Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from
Jun 28, 2023

Conversation

you06
Copy link
Contributor

@you06 you06 commented Jun 20, 2023

Cherry-pick #843 to tidb-6.5.

@disksing
Copy link
Collaborator

cannot build.

@you06
Copy link
Contributor Author

you06 commented Jun 20, 2023

Hold this PR because it may spawns too many goroutines.

@cfzjywxk
Copy link
Contributor

See the discussions in #843.
TODOS for the release-v6.5 branch:

  • Make the change about acccessFollower.next simple to reduce risk, verify the correctness.
  • Verify there's no extra or hidden cache reloading logic in the RegionCache, the background async reload worker is the only entry to resolve the region actively.

you06 and others added 9 commits June 27, 2023 21:08
Signed-off-by: you06 <[email protected]>
Signed-off-by: you06 <[email protected]>
Signed-off-by: you06 <[email protected]>
Signed-off-by: you06 <[email protected]>
Signed-off-by: Smityz <[email protected]>
Co-authored-by: disksing <[email protected]>
Signed-off-by: you06 <[email protected]>
Signed-off-by: you06 <[email protected]>
Signed-off-by: disksing <[email protected]>
Signed-off-by: you06 <[email protected]>
mockRequestLiveness atomic.Value
}

regionsNeedReload struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using a channel so Mutex could be saved and operations on the RegionCache are already synchronized?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A channel is bounded, if it's full when trying scheduling a region to it, it'll wait(maybe the asyncCheckAndResolveLoop is doing something and cannot pull the channel immediately).

Comment on lines 485 to 488
for regionID := range c.regionsNeedReload.toReload {
c.reloadRegion(regionID)
delete(c.regionsNeedReload.toReload, regionID)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look like those codes should be put after line #498 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delay the reload to next loop which avoids some errors and backoffs, see the comment of Line491 to Line494

you06 and others added 2 commits June 27, 2023 22:47
Co-authored-by: crazycs <[email protected]>
Signed-off-by: you06 <[email protected]>
Signed-off-by: you06 <[email protected]>
Signed-off-by: you06 <[email protected]>
@@ -1300,7 +1359,7 @@ func (c *RegionCache) removeVersionFromCache(oldVer RegionVerID, regionID uint64

// insertRegionToCache tries to insert the Region to cache.
// It should be protected by c.mu.Lock().
func (c *RegionCache) insertRegionToCache(cachedRegion *Region) {
func (c *RegionCache) insertRegionToCache(cachedRegion *Region, invalidateOldRegion bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to comment about the new parameter.

@you06
Copy link
Contributor Author

you06 commented Jun 28, 2023

What's wrong with golangci-lint 🤔️

run golangci-lint
  Running [/home/runner/golangci-lint-1.47.3-linux-amd64/golangci-lint run --out-format=github-actions] in [] ...
  panic: load embedded ruleguard rules: rules/rules.go:13: can't load fmt
  
  goroutine 1 [running]:
  github.com/go-critic/go-critic/checkers.init.22()
  	github.com/go-critic/[email protected]/checkers/embedded_rules.go:47 +0x4b4
  
  Error: golangci-lint exit with code 2
  Ran golangci-lint in 12405ms

@cfzjywxk
Copy link
Contributor

@crazycs520 @ekexium @zyguan
PTAL

Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cfzjywxk cfzjywxk merged commit b113831 into tikv:tidb-6.5 Jun 28, 2023
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants