Support receving health feedback #1153

MyonKeminta · 2024-02-08T07:45:25Z

Ref: tikv/tikv#16297

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

zyguan · 2024-02-23T08:12:45Z

internal/locate/region_cache.go

 	c.forEachStore(func(store *Store) {
-		store.updateSlowScoreStat()
-		slowScoreMetrics[store.storeID] = float64(store.getSlowScore())
+		store.healthStatus.update()


We need to make sure the update run as fast as possible, otherwise storeMu will be locked for a long time. Expensive operations need to be processed outside (maybe iterate stores by batch in the future). It's OK by now.

zyguan · 2024-02-23T08:12:52Z

internal/locate/region_cache.go

 	})
-	for store, score := range slowScoreMetrics {
-		metrics.TiKVStoreSlowScoreGauge.WithLabelValues(strconv.FormatUint(store, 10)).Set(score)
+	logutil.BgLogger().Info("checkAndUpdateStoreHealthStats: get health details", zap.Reflect("details", healthDetails))


Would this log be annoying?

Sorry, I added this for debugging.

zyguan · 2024-02-23T08:27:16Z

internal/client/client.go

@@ -108,6 +108,13 @@ type Client interface {
 	CloseAddr(addr string) error
 	// SendRequest sends Request.
 	SendRequest(ctx context.Context, addr string, req *tikvrpc.Request, timeout time.Duration) (*tikvrpc.Response, error)
+	// SetEventListener registers an event listener for the Client instance. If called more than once, the previously
+	// set one will be replaced.
+	SetEventListener(listener ClientEventListener)


Can we just declare that the method is not thread safe and should be called before SendRequest so that we can get rid of atomic.Pointer and make related code simpler?

Yes, but I'm afraid that the assumption might be broken easily by mistake in the future...

zyguan · 2024-02-23T08:41:27Z

internal/locate/region_cache.go

+	store.recordHealthFeedback(feedback)
+}
+
+func (c *RegionCache) GetClientEventListener() client.ClientEventListener {


Maybe document that it will be registered to the internal client of KVStore automatically when calling tikv.NewKVStore.

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

…eive-health-feedback

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

cfzjywxk · 2024-02-27T05:43:04Z

@crazycs520 PTAL

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta · 2024-02-28T05:41:30Z

As I noticed that the interface compatibility problem of unistore can be workarounded by unistoreClientWrapper, It's actually not necessary to split into multiple PRs. cc @zyguan

crazycs520 · 2024-02-28T08:06:52Z

internal/locate/region_cache.go

+	tikvSlowScoreDecayRate     = 20. / 60. // s^(-1), linear decaying
+	tikvSlowScoreSlowThreshold = 80.


Suggested change

tikvSlowScoreDecayRate = 20. / 60. // s^(-1), linear decaying

tikvSlowScoreSlowThreshold = 80.

tikvSlowScoreDecayRate float64 = 20.0 / 60.0 // s^(-1), linear decaying

tikvSlowScoreSlowThreshold float64 = 80.0

crazycs520 · 2024-02-28T08:27:50Z

internal/locate/region_cache.go

@@ -3164,6 +3339,10 @@ func (s *Store) recordReplicaFlowsStats(destType replicaFlowsType) {
 	atomic.AddUint64(&s.replicaFlowsStats[destType], 1)
 }

+func (s *Store) recordHealthFeedback(feedback *tikvpb.HealthFeedback) {
+	s.healthStatus.updateTiKVServerSideSlowScore(int64(feedback.GetSlowScore()), time.Now())


feedback.FeedbackSeqNo is never used now?

Yes, it's not checked for now. Maybe I'd better add a comment to note it.

crazycs520

REST LGTM

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

cfzjywxk · 2024-02-28T10:09:14Z

internal/locate/region_cache.go

+		return
+	}
+
+	// TODO: Try to get store status from PD here. But it's not mandatory.


The function would be executed inside the batchRecvLoop, would it affect the performance if we call pdClient.func here?

No. This function is not expected to be run in batchRecvLoop, but should be a periodic task executed in the background.
The function that might be called inside batchRecvLoop is updateTiKVServerSideSlowScore.

Here what we need to be careful about is the mutex.

Got it, it's better to add comments to functions that would be used in the critical performance path.

MyonKeminta added 5 commits February 6, 2024 16:55

Register health feedback handler in batch client

e714b6a

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

update protocol

12166fa

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Collect the health status and record to the stores

419649a

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Remove the filter which looks not necessary

9b9d0d0

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Add metrics

c381ab5

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta mentioned this pull request Feb 9, 2024

Performance/health feedback to client and the Unified Health Controller tikv/tikv#16297

Closed

8 tasks

MyonKeminta force-pushed the m/receive-health-feedback branch from fbdbfd9 to 55e9683 Compare February 18, 2024 11:36

Fix checkAndUpdateStoreHealthStatus panicking

cce241a

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta force-pushed the m/receive-health-feedback branch from 55e9683 to cce241a Compare February 18, 2024 11:37

Add logs

f769b67

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta force-pushed the m/receive-health-feedback branch from 8dc5d28 to f769b67 Compare February 18, 2024 11:56

MyonKeminta added 3 commits February 19, 2024 14:24

Merge SetHealthFeedbackHandler to Client

3d92daf

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Try another pattern: separated callback registry

a29a129

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Try region cache eventListener pattern

1ddc60d

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta force-pushed the m/receive-health-feedback branch from e6a2673 to 1ddc60d Compare February 19, 2024 12:38

MyonKeminta added 2 commits February 21, 2024 21:48

Add tests

1af7e4a

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Test receiving froim callback

3d685e7

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

zyguan reviewed Feb 23, 2024

View reviewed changes

MyonKeminta added 6 commits February 26, 2024 16:04

Add tests to region cache

91c7bbe

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Remove unnecessary debug log

903ff59

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Add comments

daa3dd0

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

remove replace of tidb repo

f800ae1

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Merge branch 'master' of https://github.com/tikv/client-go into m/rec…

c2509b4

…eive-health-feedback

fix build

f99235a

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta marked this pull request as ready for review February 27, 2024 03:41

cfzjywxk requested review from you06, ekexium and cfzjywxk February 27, 2024 05:42

Update comments; fix lint

0be8fd6

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta added 2 commits February 27, 2024 18:28

Add the isSlow method of Store back

cf5330d

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

remove unused method for now to make lint happy

c6535a8

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta requested a review from zyguan February 28, 2024 05:37

zyguan approved these changes Feb 28, 2024

View reviewed changes

crazycs520 reviewed Feb 28, 2024

View reviewed changes

crazycs520 approved these changes Feb 28, 2024

View reviewed changes

zyguan mentioned this pull request Feb 28, 2024

RegionCache Enhancement #1104

Open

8 tasks

MyonKeminta and others added 2 commits February 28, 2024 16:56

Address comments

eb1421f

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Merge branch 'master' into m/receive-health-feedback

64f11b1

cfzjywxk reviewed Feb 28, 2024

View reviewed changes

cfzjywxk merged commit 03bbadb into tikv:master Feb 28, 2024
9 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support receving health feedback #1153

Support receving health feedback #1153

MyonKeminta commented Feb 8, 2024

zyguan Feb 23, 2024

zyguan Feb 23, 2024

MyonKeminta Feb 26, 2024

zyguan Feb 23, 2024

MyonKeminta Feb 26, 2024

zyguan Feb 23, 2024

cfzjywxk commented Feb 27, 2024

MyonKeminta commented Feb 28, 2024

crazycs520 Feb 28, 2024

crazycs520 Feb 28, 2024

MyonKeminta Feb 28, 2024

crazycs520 left a comment

cfzjywxk Feb 28, 2024

MyonKeminta Feb 28, 2024

cfzjywxk Feb 28, 2024

		tikvSlowScoreDecayRate = 20. / 60. // s^(-1), linear decaying
		tikvSlowScoreSlowThreshold = 80.

Support receving health feedback #1153

Support receving health feedback #1153

Conversation

MyonKeminta commented Feb 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfzjywxk commented Feb 27, 2024

MyonKeminta commented Feb 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazycs520 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment