add region cache state test & fix some issues of replica selector #910

you06 · 2023-07-26T02:39:34Z

This PR add the selector replica test for stale read, cover the most possible errors, and fix the bug the test finds:

Fixes:

when fallback to followers from leader, stale-read flag should be removed.
in some cases, the fallback to follower not work.
when using stale read, allow async reload region when leader is exhausted, because leader may just be unavailable for a while, and the late update of region info won't affect stale read processing.

Signed-off-by: you06 <[email protected]>

cfzjywxk · 2023-08-04T15:36:57Z

/cc @crazycs520

cfzjywxk · 2023-08-04T15:48:36Z

internal/locate/region_request.go

+			// In stale-read, the request will fallback to leader after the local follower failure.
+			// If the leader is also unavailable, we can fallback to the follower and use replica-read flag again,
+			// The remote follower not tried yet, and the local follower can retry without stale-read flag.
+			if state.isStaleRead {


This could be printed in the above log so we know if tryFollower happens in current turn, so is the fallbackFromLeader state.

cfzjywxk · 2023-08-04T15:52:30Z

internal/locate/region_request.go

@@ -609,14 +615,30 @@ func (state *accessFollower) next(bo *retry.Backoffer, selector *replicaSelector
 	// If there is no candidate, fallback to the leader.
 	if selector.targetIdx < 0 {
 		leader := selector.replicas[state.leaderIdx]
-		leaderInvalid := leader.isEpochStale() || state.IsLeaderExhausted(leader)
+		leaderEpochStale := leader.isEpochStale()
+		leaderInvalid := leaderEpochStale || state.IsLeaderExhausted(leader)


Maybe we could refactor the implememtation of state.IsLeaderExhausted, cheking if the leader is exausted by the current states instead of leader.isExhausted(2).

cfzjywxk · 2023-08-07T01:59:57Z

internal/locate/region_request_state_test.go

+
+	resp, _, err := s.regionRequestSender.SendReqCtx(bo, req, regionLoc.Region, time.Second, tikvrpc.TiKV, ops...)
+	if !available {
+		if err != nil {


Is this err not checked if it's expected or not?

…kv#910) Signed-off-by: you06 <[email protected]>

…kv#910) Signed-off-by: you06 <[email protected]> remove duplicate code Signed-off-by: you06 <[email protected]>

…) (#942) * add region cache state test & fix some issues of replica selector (#910) Signed-off-by: you06 <[email protected]> remove duplicate code Signed-off-by: you06 <[email protected]> * remove comment Signed-off-by: you06 <[email protected]> * lint Signed-off-by: you06 <[email protected]> * fix flaky test Signed-off-by: you06 <[email protected]> --------- Signed-off-by: you06 <[email protected]>

* reload region cache when store is resolved from invalid status (#843) Signed-off-by: you06 <[email protected]> Co-authored-by: disksing <[email protected]> * fallback to follower when leader is busy (#916) (#923) * fallback to follower when leader is busy Signed-off-by: you06 <[email protected]> Co-authored-by: cfzjywxk <[email protected]> Co-authored-by: cfzjywxk <[email protected]> * Resume max retry time check for stale read retry with leader option(#903) (#911) * Resume max retry time check for stale read retry with leader option Signed-off-by: cfzjywxk <[email protected]> * add cancel Signed-off-by: cfzjywxk <[email protected]> --------- Signed-off-by: cfzjywxk <[email protected]> * add region cache state test & fix some issues of replica selector (#910) Signed-off-by: you06 <[email protected]> remove duplicate code Signed-off-by: you06 <[email protected]> * enable workflow for tidb-7.1 Signed-off-by: you06 <[email protected]> * update Signed-off-by: you06 <[email protected]> update Signed-off-by: you06 <[email protected]> fix test Signed-off-by: you06 <[email protected]> fix test Signed-off-by: you06 <[email protected]> * lint Signed-off-by: you06 <[email protected]> * lint Signed-off-by: you06 <[email protected]> * fix flaky test Signed-off-by: you06 <[email protected]> --------- Signed-off-by: you06 <[email protected]> Signed-off-by: cfzjywxk <[email protected]> Co-authored-by: disksing <[email protected]> Co-authored-by: cfzjywxk <[email protected]> Co-authored-by: cfzjywxk <[email protected]>

Co-authored-by: cfzjywxk <[email protected]> Co-authored-by: cfzjywxk <[email protected]> Co-authored-by: disksing <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: zzm <[email protected]> Co-authored-by: husharp <[email protected]> Co-authored-by: you06 <[email protected]> Co-authored-by: buffer <[email protected]> Co-authored-by: 3pointer <[email protected]> Co-authored-by: buffer <[email protected]> Co-authored-by: husharp <[email protected]> Co-authored-by: crazycs520 <[email protected]> Co-authored-by: Smilencer <[email protected]> Co-authored-by: ShuNing <[email protected]> Co-authored-by: zyguan <[email protected]> Co-authored-by: Jack Yu <[email protected]> Co-authored-by: Weizhen Wang <[email protected]> Co-authored-by: lucasliang <[email protected]> Co-authored-by: healthwaite <[email protected]> Co-authored-by: xufei <[email protected]> Co-authored-by: JmPotato <[email protected]> Co-authored-by: ekexium <[email protected]> Co-authored-by: 山岚 <[email protected]> Co-authored-by: glorv <[email protected]> Co-authored-by: Yongbo Jiang <[email protected]> resolve locks interface for tidb gc_worker (#945) fix some issues of replica selector (#910) (#942) fix some issues of replica selector (#910) fix issue of configure kv timeout not work when disable batch client (#980) fix batch-client wait too long and add some metrics (#973) fix batch-client wait too long and add some metrics (#973)" (#984) fix data race at the aggressiveLockingDirty (#913) fix MinSafeTS might be set to MaxUint64 permanently (#994) fix: fix invalid nil pointer when trying to record Store.SlownessStat. (#1017) Fix batch client batchSendLoop panic (#1021) fix request source tag unset (#1025) Fix comment of `SuspendTime` (#1057)

you06 force-pushed the region-cache-test branch 2 times, most recently from 9bbea85 to 533d7eb Compare August 2, 2023 05:34

Add region cache test for stale read

5efef85

Signed-off-by: you06 <[email protected]>

you06 force-pushed the region-cache-test branch from e607df2 to 5efef85 Compare August 2, 2023 07:06

you06 marked this pull request as ready for review August 2, 2023 07:06

you06 changed the title ~~Add region cache state test~~ add region cache state test & fix some issues of replica selector Aug 2, 2023

you06 added 2 commits August 2, 2023 15:15

add can fallback follower condition

709e6d9

Signed-off-by: you06 <[email protected]>

update pr

979c489

Signed-off-by: you06 <[email protected]>

you06 mentioned this pull request Aug 4, 2023

When leader is selected as target peer at first, no other replicas could be retied for stale read #906

Closed

cfzjywxk requested review from cfzjywxk, zyguan, MyonKeminta and ekexium August 4, 2023 15:36

cfzjywxk reviewed Aug 4, 2023

View reviewed changes

you06 mentioned this pull request Aug 4, 2023

Only fallback to leader read with data-is-not-ready error #907

Closed

cfzjywxk reviewed Aug 7, 2023

View reviewed changes

cfzjywxk approved these changes Aug 7, 2023

View reviewed changes

disksing merged commit 5968ce9 into tikv:tidb-6.5 Aug 7, 2023
7 of 9 checks passed

cfzjywxk mentioned this pull request Aug 7, 2023

storage: backport the stale read enhancement and bug fix to release 6.5 pingcap/tidb#43481

Closed

you06 added a commit to you06/client-go that referenced this pull request Aug 14, 2023

add region cache state test & fix some issues of replica selector (ti…

d89c4b3

…kv#910) Signed-off-by: you06 <[email protected]>

you06 mentioned this pull request Aug 14, 2023

add region cache state test & fix some issues of replica selector (#910) #942

Merged

you06 added a commit to you06/client-go that referenced this pull request Aug 14, 2023

add region cache state test & fix some issues of replica selector (ti…

e2455c1

…kv#910) Signed-off-by: you06 <[email protected]> remove duplicate code Signed-off-by: you06 <[email protected]>

you06 added a commit to you06/client-go that referenced this pull request Aug 14, 2023

add region cache state test & fix some issues of replica selector (ti…

b032778

…kv#910) Signed-off-by: you06 <[email protected]> remove duplicate code Signed-off-by: you06 <[email protected]>

you06 added a commit to you06/client-go that referenced this pull request Aug 15, 2023

add region cache state test & fix some issues of replica selector (ti…

c572cc3

…kv#910) Signed-off-by: you06 <[email protected]> remove duplicate code Signed-off-by: you06 <[email protected]>

you06 added a commit to you06/client-go that referenced this pull request Aug 15, 2023

add region cache state test & fix some issues of replica selector (ti…

318f84d

…kv#910) Signed-off-by: you06 <[email protected]> remove duplicate code Signed-off-by: you06 <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add region cache state test & fix some issues of replica selector #910

add region cache state test & fix some issues of replica selector #910

you06 commented Jul 26, 2023 •

edited

Loading

cfzjywxk commented Aug 4, 2023

cfzjywxk Aug 4, 2023

cfzjywxk Aug 4, 2023 •

edited

Loading

cfzjywxk Aug 7, 2023

add region cache state test & fix some issues of replica selector #910

add region cache state test & fix some issues of replica selector #910

Conversation

you06 commented Jul 26, 2023 • edited Loading

cfzjywxk commented Aug 4, 2023

cfzjywxk Aug 4, 2023

Choose a reason for hiding this comment

cfzjywxk Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

cfzjywxk Aug 7, 2023

Choose a reason for hiding this comment

you06 commented Jul 26, 2023 •

edited

Loading

cfzjywxk Aug 4, 2023 •

edited

Loading