region_cache: check epoch before insert#1079
Merged
disksing merged 10 commits intotikv:masterfrom Dec 20, 2023
Merged
Conversation
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
rleungx
reviewed
Dec 18, 2023
internal/locate/region_cache.go
Outdated
| // The second case is that the region may be obtained from the PD follower, | ||
| // and there is the synchronization time between the pd follower and the leader. | ||
| // So we should check the epoch. | ||
| if ok && (latest.GetVer() > newVer.GetVer() || latest.GetConfVer() > newVer.GetConfVer()) { |
Member
There was a problem hiding this comment.
The naming is confused, which one is newer?
| "github.com/tikv/client-go/v2/internal/mockstore/mocktikv" | ||
| "github.com/tikv/client-go/v2/kv" | ||
| pd "github.com/tikv/pd/client" | ||
| uatomic "go.uber.org/atomic" |
Member
Author
There was a problem hiding this comment.
Because sync/atomic is also imported.
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
rleungx
reviewed
Dec 18, 2023
internal/locate/region_cache.go
Outdated
| return true | ||
| } | ||
| // Also check the intersecting regions. | ||
| intersectedRegions, stale := mu.sorted.removeIntersecting(cachedRegion, &newVer) |
Member
There was a problem hiding this comment.
If the region is not stale, will the intersecting regions be removed first?
Member
Author
There was a problem hiding this comment.
Yes, but we will use lock first. So it seems no affect.
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Member
Author
|
cc @disksing |
disksing
reviewed
Dec 20, 2023
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> address comment Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
9010f6d to
713e6ec
Compare
disksing
approved these changes
Dec 20, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There are two or more situations in which the region we got is stale. The first case is that the process of getting a region is concurrent. The stale region may be returned later due to network reasons.
The second case is that the region may be obtained from the PD follower in the future, and there is the synchronization time between the pd follower and the leader.
So we should check the epoch to avoid stale region.
Some details of the
insertRegionToCacheare described as follows.Firstly,
mu.latestVersions[cachedRegion.VerID().id]is used to check if there is an old version with the same ID. If it exists, the code compares the version numbers (newVer.GetVer()andoldVer.GetVer()) as well as the conf version numbers (newVer.GetConfVer()andoldVer.GetConfVer()). If the old version number is higher, it considers the newly obtained Region as stale.Next,
mu.sorted.removeIntersectingis called to check and remove any old Regions that overlap with the new Region, and to determine if the new Region is stale.Afterward, the new Region is inserted into the cache using the
mu.sorted.ReplaceOrInsert(cachedRegion)method.If there are existing old Regions that overlap with the new Region, it inherits some information from the first overlapping Region, such as working indexes for TiKV and TiFlash, as well as bucket information.
Finally, it iterates over the overlapping old Regions, removes them from the cache, and marks them as expired. If the
invalidateOldRegionparameter is true, the old Regions are marked as invalid to prevent certain requests from using expired Region information.Lastly, relevant variables are updated, including adding the new Region and its corresponding version to mu.regions and
mu.latestVersions.Throughout this process, by comparing version numbers and cluster change version numbers, as well as removing overlapping old Regions, it ensures that the Region information in the cache is up-to-date and valid.
Benchmark test result (count = 20)
InsertRegionToCachetests the case in which the key range and epoch change and delay appear.InsertRegionToCache2tests the case in which the key range does not change.master compares this PR
We can see that the results of the two tests are significantly different. There is no problem in terms of speed, mainly in terms of memory allocation.
To find out why this PR allocates more bytes, I remove the code that checks the intersected region(in
sorted_btree.go).And master compares it.
From this, we can see that the additional overhead is mainly due to checking intersecting regions. IMO, this is reasonable.