reload region cache when store is resolved from invalid status #843

you06 · 2023-06-16T11:34:54Z

close #841

Before this PR:

With this PR:

Signed-off-by: you06 <you1474600@gmail.com>

LykxSassinator · 2023-06-19T04:18:55Z

internal/locate/region_request.go

@@ -573,18 +573,22 @@ func (state *accessFollower) next(bo *retry.Backoffer, selector *replicaSelector
 	if state.option.preferLeader {
 		state.lastIdx = state.leaderIdx
 	}
+	offset := rand.Intn(replicaSize)


Under PreferLeader mode, offset should be set to 0 to make the selection starting from leaderIdx.

Yeah, I changed the code and not to use offset in the first selection.

Signed-off-by: you06 <you1474600@gmail.com>

cfzjywxk · 2023-06-19T06:32:41Z

/cc @crazycs520

LykxSassinator · 2023-06-19T07:16:56Z

internal/locate/region_request.go

-		// as candidates to serve the Read request. Meanwhile, we should make the choice of next() meet Uniform Distribution.
-		for cnt := 0; cnt < replicaSize && !state.isCandidate(idx, selector.replicas[idx]); cnt++ {
-			idx = AccessIndex((int(idx) + rand.Intn(replicaSize)) % replicaSize)
+		var idx AccessIndex


Still recommend to revert to the original one, current implementation is bit more complex and hard to read.

Signed-off-by: you06 <you1474600@gmail.com>

cfzjywxk · 2023-06-19T08:49:47Z

internal/locate/region_cache.go

+		// async reload triggered by other thread.
+		return
+	}
+	go func() {


It can be risky to spawn a reload task for each region, especially if many regions become invalid due to the related store being marked as such. To mitigate this risk, we should implement a refreshing strategy in the region cache-related workers and consider incorporating the store's "health checker."

@crazycs520 is now considering refactoring the region cache component, he could give some advice about it.

For the v6.5.x release version, a quick fix may still be needed.

cfzjywxk · 2023-06-19T08:53:23Z

internal/locate/region_request.go

@@ -573,18 +573,36 @@ func (state *accessFollower) next(bo *retry.Backoffer, selector *replicaSelector
 	if state.option.preferLeader {
 		state.lastIdx = state.leaderIdx
 	}
+	var offset int


Peer selection can become quite complex when the accessFollower is used for all follower read and stale read processing, with various configurations. To prevent any potential issues, it may be beneficial to separate them into different categories.

ReplicaSelector itself is quite complex too..

After discussing with @you06, we could do some refactoring on the master branch like

match access_type { leade_only => follower_read_random => follower_read_replica_only => follower_read_closet => stale_read_random => stale_read_closest => stale_read_retry => }

while keeping the change on release-6.5 as simple as possible to reduce the risk.

Maybe the refactor can be in another PR?

Ok let's merge this first

Signed-off-by: you06 <you1474600@gmail.com>

…master-stale-patch

#846) * reload region cache when store is resolved from invalid Signed-off-by: you06 <you1474600@gmail.com> * reload region once Signed-off-by: you06 <you1474600@gmail.com> * build in go1.18 Signed-off-by: you06 <you1474600@gmail.com> * build in go1.18 Signed-off-by: you06 <you1474600@gmail.com> * handle region reload in resolve goroutine Signed-off-by: you06 <you1474600@gmail.com> * retest Signed-off-by: you06 <you1474600@gmail.com> * fix data race (#736) Signed-off-by: Smityz <smityz@qq.com> Co-authored-by: disksing <i@disksing.com> Signed-off-by: you06 <you1474600@gmail.com> * build with go 1.18 Signed-off-by: you06 <you1474600@gmail.com> * fix integration test (#673) Signed-off-by: disksing <i@disksing.com> Signed-off-by: you06 <you1474600@gmail.com> * Update internal/locate/region_cache.go Co-authored-by: crazycs <crazycs520@gmail.com> Signed-off-by: you06 <you1474600@gmail.com> * address comment Signed-off-by: you06 <you1474600@gmail.com> * address comment Signed-off-by: you06 <you1474600@gmail.com> --------- Signed-off-by: you06 <you1474600@gmail.com> Signed-off-by: Smityz <smityz@qq.com> Signed-off-by: disksing <i@disksing.com> Co-authored-by: Smilencer <smityz@qq.com> Co-authored-by: disksing <i@disksing.com> Co-authored-by: crazycs <crazycs520@gmail.com>

internal/locate/region_cache.go

Signed-off-by: you06 <you1474600@gmail.com>

* client-go: add some key range info to error when PD returned no region (#862) Signed-off-by: Chao Wang <cclcwangchao@hotmail.com> * *: refine non-global stale-read request retry logic (#863) Signed-off-by: crazycs520 <crazycs520@gmail.com> * Fix the issue that primary pessimistic lock may be left not cleared after GC (#866) * Fix the issue that primary pessimistic lock may be left not cleared after GC Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * Fix mysteriously shown up thing that makes compilation failed Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * Fix test effectiveness (forgot to set txn2 to pessimistic txn); add more strict checks Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * Address comments Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> --------- Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> Co-authored-by: MyonKeminta <MyonKeminta@users.noreply.github.com> * add explicit request source type to label the external request like lightning/br (#868) Signed-off-by: nolouch <nolouch@gmail.com> * use '%d' instead of '%q' for some int values in error message (#875) Signed-off-by: Chao Wang <cclcwangchao@hotmail.com> * format key in error message in method `scanRegions` (#876) Signed-off-by: Chao Wang <cclcwangchao@hotmail.com> * make cop request timeout a config paramter (#865) * update Signed-off-by: Spade A <u6748471@anu.edu.au> * update Signed-off-by: Spade A <u6748471@anu.edu.au> * update Signed-off-by: Spade A <u6748471@anu.edu.au> * update Signed-off-by: Spade A <u6748471@anu.edu.au> --------- Signed-off-by: Spade A <u6748471@anu.edu.au> * region_cache: support check pending tiflash peer (#821) Signed-off-by: guo-shaoge <shaoge1994@163.com> Co-authored-by: disksing <i@disksing.com> * *: add `SnapshotIterReverse` and make `iterReverse` supports `lowerBound` (#883) Signed-off-by: Jason Mo <mohangjie1995@gmail.com> * *: fix stale read ops metric (#878) (#889) Signed-off-by: crazycs520 <crazycs520@gmail.com> Co-authored-by: disksing <i@disksing.com> * add gc options (#828) Signed-off-by: weedge <weege007@gmail.com> Co-authored-by: disksing <i@disksing.com> * reload region cache when store is resolved from invalid status (#843) Signed-off-by: you06 <you1474600@gmail.com> Co-authored-by: disksing <i@disksing.com> * ci: update setup-go action (#904) Signed-off-by: disksing <i@disksing.com> * fix unexpected slow query during GC running after stop 1 tikv-server (#899) (#909) * fix unexpected slow query during GC running after stop 1 tikv-server Signed-off-by: crazycs520 <crazycs520@gmail.com> * fix test Signed-off-by: crazycs520 <crazycs520@gmail.com> --------- Signed-off-by: crazycs520 <crazycs520@gmail.com> * resource_manager: ignore ru metrics for background request (#872) Signed-off-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: disksing <i@disksing.com> * add more log for diagnose (#915) * add more log for diagnose Signed-off-by: crazycs520 <crazycs520@gmail.com> * fix Signed-off-by: crazycs520 <crazycs520@gmail.com> * add more log for diagnose Signed-off-by: crazycs520 <crazycs520@gmail.com> * add more log Signed-off-by: crazycs520 <crazycs520@gmail.com> * address comment Signed-off-by: crazycs520 <crazycs520@gmail.com> --------- Signed-off-by: crazycs520 <crazycs520@gmail.com> * use context logger as much as possible (#908) * use context logger as much as possible Signed-off-by: crazycs520 <crazycs520@gmail.com> * refine Signed-off-by: crazycs520 <crazycs520@gmail.com> --------- Signed-off-by: crazycs520 <crazycs520@gmail.com> * 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 <lsswxrxr@163.com> * add cancel Signed-off-by: cfzjywxk <lsswxrxr@163.com> --------- Signed-off-by: cfzjywxk <lsswxrxr@163.com> * request_source: remove default label (#890) * request_source: remove default label Signed-off-by: nolouch <nolouch@gmail.com> * add a function to set request source task type (#925) * add a function to set request source task type Signed-off-by: glorv <glorvs@163.com> * ci: update go version (#936) * ci: update go version Signed-off-by: crazycs520 <crazycs520@gmail.com> * fix test Signed-off-by: crazycs520 <crazycs520@gmail.com> --------- Signed-off-by: crazycs520 <crazycs520@gmail.com> * use tidb_kv_read_timeout as first kv request timeout (#919) * support tidb_kv_read_timeout as first round kv request timeout Signed-off-by: crazycs520 <crazycs520@gmail.com> * fix ci Signed-off-by: crazycs520 <crazycs520@gmail.com> * fix ci Signed-off-by: crazycs520 <crazycs520@gmail.com> * fix ci Signed-off-by: crazycs520 <crazycs520@gmail.com> * fix ci Signed-off-by: crazycs520 <crazycs520@gmail.com> * fix ci Signed-off-by: crazycs520 <crazycs520@gmail.com> * update comment Signed-off-by: crazycs520 <crazycs520@gmail.com> * refine test Signed-off-by: crazycs520 <crazycs520@gmail.com> --------- Signed-off-by: crazycs520 <crazycs520@gmail.com> * [pick] resource_control: bypass some internal urgent request (#938) * resource_control: bypass some internal urgent request (#884) Signed-off-by: nolouch <nolouch@gmail.com> * resourcecontrol: fix nil pointer (#900) Signed-off-by: nolouch <nolouch@gmail.com> --------- Signed-off-by: nolouch <nolouch@gmail.com> --------- Signed-off-by: Chao Wang <cclcwangchao@hotmail.com> Signed-off-by: crazycs520 <crazycs520@gmail.com> Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> Signed-off-by: nolouch <nolouch@gmail.com> Signed-off-by: Spade A <u6748471@anu.edu.au> Signed-off-by: guo-shaoge <shaoge1994@163.com> Signed-off-by: Jason Mo <mohangjie1995@gmail.com> Signed-off-by: weedge <weege007@gmail.com> Signed-off-by: you06 <you1474600@gmail.com> Signed-off-by: disksing <i@disksing.com> Signed-off-by: husharp <jinhao.hu@pingcap.com> Signed-off-by: cfzjywxk <lsswxrxr@163.com> Signed-off-by: glorv <glorvs@163.com> Signed-off-by: iosmanthus <myosmanthustree@gmail.com> Co-authored-by: 王超 <cclcwangchao@hotmail.com> Co-authored-by: crazycs <crazycs520@gmail.com> Co-authored-by: MyonKeminta <9948422+MyonKeminta@users.noreply.github.com> Co-authored-by: MyonKeminta <MyonKeminta@users.noreply.github.com> Co-authored-by: ShuNing <nolouch@gmail.com> Co-authored-by: Spade A <71589810+SpadeA-Tang@users.noreply.github.com> Co-authored-by: guo-shaoge <shaoge1994@163.com> Co-authored-by: disksing <i@disksing.com> Co-authored-by: Hangjie Mo <mohangjie1995@gmail.com> Co-authored-by: weedge <weege007@gmail.com> Co-authored-by: you06 <you1474600@gmail.com> Co-authored-by: Hu# <jinhao.hu@pingcap.com> Co-authored-by: cfzjywxk <lsswxrxr@163.com> Co-authored-by: glorv <glorvs@163.com>

) Signed-off-by: you06 <you1474600@gmail.com> Co-authored-by: disksing <i@disksing.com>

* reload region cache when store is resolved from invalid status (#843) Signed-off-by: you06 <you1474600@gmail.com> Co-authored-by: disksing <i@disksing.com> * fallback to follower when leader is busy (#916) (#923) * fallback to follower when leader is busy Signed-off-by: you06 <you1474600@gmail.com> Co-authored-by: cfzjywxk <cfzjywxk@gmail.com> Co-authored-by: cfzjywxk <lsswxrxr@163.com> * 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 <lsswxrxr@163.com> * add cancel Signed-off-by: cfzjywxk <lsswxrxr@163.com> --------- Signed-off-by: cfzjywxk <lsswxrxr@163.com> * add region cache state test & fix some issues of replica selector (#910) Signed-off-by: you06 <you1474600@gmail.com> remove duplicate code Signed-off-by: you06 <you1474600@gmail.com> * enable workflow for tidb-7.1 Signed-off-by: you06 <you1474600@gmail.com> * update Signed-off-by: you06 <you1474600@gmail.com> update Signed-off-by: you06 <you1474600@gmail.com> fix test Signed-off-by: you06 <you1474600@gmail.com> fix test Signed-off-by: you06 <you1474600@gmail.com> * lint Signed-off-by: you06 <you1474600@gmail.com> * lint Signed-off-by: you06 <you1474600@gmail.com> * fix flaky test Signed-off-by: you06 <you1474600@gmail.com> --------- Signed-off-by: you06 <you1474600@gmail.com> Signed-off-by: cfzjywxk <lsswxrxr@163.com> Co-authored-by: disksing <i@disksing.com> Co-authored-by: cfzjywxk <cfzjywxk@gmail.com> Co-authored-by: cfzjywxk <lsswxrxr@163.com>

reload region cache when store is resolved from invalid

bb30b61

Signed-off-by: you06 <you1474600@gmail.com>

you06 changed the title ~~reload region cache when store is resolved from invalid~~ reload region cache when store is resolved from invalid status Jun 16, 2023

you06 added 2 commits June 19, 2023 11:49

refine code

5119e70

Signed-off-by: you06 <you1474600@gmail.com>

use offset to select replica randomly

9f798e2

Signed-off-by: you06 <you1474600@gmail.com>

LykxSassinator reviewed Jun 19, 2023

View reviewed changes

you06 added 2 commits June 19, 2023 12:46

choose leader firstly for prefer leader

ba45117

Signed-off-by: you06 <you1474600@gmail.com>

refine code

deb5a88

Signed-off-by: you06 <you1474600@gmail.com>

cfzjywxk requested review from cfzjywxk, MyonKeminta and zyguan June 19, 2023 06:32

LykxSassinator reviewed Jun 19, 2023

View reviewed changes

simplify code

696a06a

Signed-off-by: you06 <you1474600@gmail.com>

cfzjywxk reviewed Jun 19, 2023

View reviewed changes

you06 mentioned this pull request Jun 20, 2023

reload region cache when store is resolved from invalid status (#843) #846

Merged

you06 added 4 commits June 20, 2023 18:05

handle region reload in check-resovle goroutine

753346d

Signed-off-by: you06 <you1474600@gmail.com>

Merge branch 'master' into master-stale-patch

818e1c7

fix test

ba1eaef

Signed-off-by: you06 <you1474600@gmail.com>

Merge branch 'master-stale-patch' of github.com:you06/client-go into …

7703c17

…master-stale-patch

cfzjywxk reviewed Jun 29, 2023

View reviewed changes

internal/locate/region_cache.go Show resolved Hide resolved

internal/locate/region_cache.go Outdated Show resolved Hide resolved

cfzjywxk requested a review from ekexium June 29, 2023 07:18

address comment

034f36e

Signed-off-by: you06 <you1474600@gmail.com>

cfzjywxk approved these changes Jun 29, 2023

View reviewed changes

Merge branch 'master' into master-stale-patch

84e83df

you06 mentioned this pull request Jul 5, 2023

refactor accessFollower #873

Open

crazycs520 approved these changes Jul 7, 2023

View reviewed changes

disksing added 2 commits July 14, 2023 11:47

Merge branch 'master' into master-stale-patch

7a73a23

Merge branch 'master' into master-stale-patch

55915dc

disksing merged commit 85fc8f3 into tikv:master Jul 14, 2023

zyguan mentioned this pull request Jul 24, 2023

let access-follower reload region on no candidate #901

Closed

you06 mentioned this pull request Aug 9, 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 11, 2023

reload region cache when store is resolved from invalid status (tikv#843

9917a17

) Signed-off-by: you06 <you1474600@gmail.com> Co-authored-by: disksing <i@disksing.com>

you06 mentioned this pull request Aug 11, 2023

Cherry pick some patches to 7.1 #940

Merged

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

reload region cache when store is resolved from invalid status (tikv#843

00afe65

) Signed-off-by: you06 <you1474600@gmail.com> Co-authored-by: disksing <i@disksing.com>

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

reload region cache when store is resolved from invalid status (tikv#843

60bbaf1

) Signed-off-by: you06 <you1474600@gmail.com> Co-authored-by: disksing <i@disksing.com>

zyguan mentioned this pull request Dec 28, 2023

PreferLeader doesn't try followers #1097

Closed

zyguan mentioned this pull request Jan 18, 2024

improve region reload strategy #1122

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reload region cache when store is resolved from invalid status #843

reload region cache when store is resolved from invalid status #843

you06 commented Jun 16, 2023 •

edited

Loading

LykxSassinator Jun 19, 2023

you06 Jun 19, 2023

cfzjywxk commented Jun 19, 2023

LykxSassinator Jun 19, 2023

cfzjywxk Jun 19, 2023

cfzjywxk Jun 19, 2023

cfzjywxk Jun 19, 2023

cfzjywxk Jun 26, 2023 •

edited

Loading

you06 Jun 27, 2023

cfzjywxk Jun 29, 2023

reload region cache when store is resolved from invalid status #843

reload region cache when store is resolved from invalid status #843

Conversation

you06 commented Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfzjywxk commented Jun 19, 2023

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 Jun 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

you06 commented Jun 16, 2023 •

edited

Loading

cfzjywxk Jun 26, 2023 •

edited

Loading