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

region_cache: support buckets #439

Merged
merged 4 commits into from
Mar 9, 2022

Conversation

youjiali1995
Copy link
Contributor

@youjiali1995 youjiali1995 commented Mar 4, 2022

Signed-off-by: youjiali1995 [email protected]

Region cache now saves buckets information which is fetched from PD by GetRegion() or GetRegionByID()(tikv/pd#4699):

There are some TODOs left, I will file PRs for them:

  • BatchLoadRegionsWithKeyRange() which is used by GC worker populates the region cache now.
  • Reduce GetRegion() traffic flow by singleflight.
  • Optimize some update policy, e.g., on epoch-not-match error.
  • Replace the TiDB dependency. Now it depends on a develop branch.

@youjiali1995 youjiali1995 force-pushed the region-cache-with-bucket branch 2 times, most recently from 7839d1c to 423dc8b Compare March 7, 2022 10:15
Signed-off-by: youjiali1995 <[email protected]>
@youjiali1995 youjiali1995 force-pushed the region-cache-with-bucket branch from 423dc8b to e0ebd26 Compare March 7, 2022 10:26
@youjiali1995 youjiali1995 force-pushed the region-cache-with-bucket branch 3 times, most recently from e88ef0f to 30cc87e Compare March 8, 2022 06:44
Signed-off-by: youjiali1995 <[email protected]>
@youjiali1995 youjiali1995 force-pushed the region-cache-with-bucket branch from 30cc87e to 66ae954 Compare March 8, 2022 06:49
@youjiali1995 youjiali1995 requested a review from sticnarf March 8, 2022 06:59
@youjiali1995
Copy link
Contributor Author

@cfzjywxk PTAL too, if you have time.

@@ -1027,6 +1084,8 @@ func (c *RegionCache) BatchLoadRegionsWithKeyRange(bo *retry.Backoffer, startKey
c.mu.Lock()
defer c.mu.Unlock()

// TODO(youjiali1995): scanRegions always fetch regions from PD and these regions don't contain buckets information
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it mean that we are going to add option to the ScanRegions interface, so it can return buckets in the same response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, too much network traffic. A 10GB region can contains 100 buckets, so the payload of a single region can be several KBs. I will make BatchLoadRegionsWithKeyRange() use scanRegionsFromCache() or not insert regions to cache.

if meta, err = decodeRegionMetaKeyWithShallowCopy(meta); err != nil {
return false, errors.Errorf("newRegion's range key is not encoded: %v, %v", meta, err)
}
}
region, err := newRegion(bo, c, &pd.Region{Meta: meta})
// TODO(youjiali1995): new regions inherit old region's buckets now. Maybe we should make EpochNotMatch error
// carry buckets information. Can it bring much overhead?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is is true that bucket information usually doesn't change greatly just after a region merge or split? Then, an asynchronous UpdateBucketsIfNeeded from PD may be ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Buckets is generated asynchronously after region leader or range changed. Querying PD immediately may not get the newer buckets, so not handling it and depending on https://github.com/pingcap/tidb/pull/32818/files#diff-23eb1043649369d5956b123b173b799ded8c6b7cbf4519bb32cb3caa01928d75R936 is also a choice.

Signed-off-by: youjiali1995 <[email protected]>
Copy link
Collaborator

@sticnarf sticnarf left a comment

Choose a reason for hiding this comment

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

LGTM

@youjiali1995 youjiali1995 merged commit 7e34d88 into tikv:master Mar 9, 2022
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.

2 participants