copr: split copr requests by buckets #32818

youjiali1995 · 2022-03-04T01:54:19Z

Signed-off-by: youjiali1995 zlwgx1023@gmail.com

What problem does this PR solve?

Issue Number: close #32968

Problem Summary:

What is changed and how it works?

Update client-go(region_cache: support buckets tikv/client-go#439) and pd-client(client: support getting region with buckets tikv/pd#4699).
Split copr requests by buckets
If the latestBucketsVersion in copr reponse is newer than the cached one, update buckets asynchronously.
No behaviour change if bucket is not enabled.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Documentation

Release note

None

ti-chi-bot · 2022-03-04T01:54:20Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

cfzjywxk
sticnarf

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-chi-bot · 2022-03-04T01:54:25Z

@youjiali1995: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

…cket Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

youjiali1995 · 2022-03-09T09:26:57Z

/run-unit-test

sre-bot · 2022-03-09T10:35:08Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/4f86e6c5052f525f4a87d14c17e8eb669bf8f1d5

youjiali1995 · 2022-03-09T12:11:53Z

/run-all-tests

cfzjywxk · 2022-03-10T02:09:18Z

store/copr/coprocessor.go

@@ -180,7 +181,8 @@ func buildCopTasks(bo *Backoffer, cache *RegionCache, ranges *KeyRanges, req *kv

 	rangesLen := ranges.Len()

-	locs, err := cache.SplitKeyRangesByLocations(bo, ranges)
+	// TODO(youjiali1995): is there any request type that needn't be splitted by buckets?


Will the split affect some internal work such as br cheksum, data analyze or statistic update?

Yes, they should be splitted by buckets too.

Does TiFlash ignore buckets?

Yes. TiFlash has implemented server-side streaming by BatchCoprocessor which splits ranges by segments.

cfzjywxk · 2022-03-10T02:11:44Z

store/copr/coprocessor_test.go

+}
+
+func TestBuildTasksByBuckets(t *testing.T) {
+	mockClient, cluster, pdClient, err := testutils.NewMockTiKV("", nil)


Should we use the new mockstore(NewMockStore) instead?

No, NewMockStore doesn't export region schedule functions.

sticnarf

SplitKeyRangesByLocations and splitKeyRangesByBuckets share the similar structure because a bucket is just like a region in region. Is it possible to unify them?

sticnarf · 2022-03-10T07:08:36Z

store/copr/coprocessor.go

@@ -319,7 +322,6 @@ type copIteratorWorker struct {

 	replicaReadSeed uint32

-	actionOnExceed             *rateLimitAction


Is it confirmed to be never used in the future?

I don't know, but it's not used now.

sticnarf · 2022-03-10T07:10:37Z

store/copr/coprocessor.go

-	region tikv.RegionVerID
-	ranges *KeyRanges
+	region     tikv.RegionVerID
+	bucketsVer uint64


What will it be used for?

I forget it... Now it's used for reducing UpdateBucketsIfNeeded 0b24a84#diff-23eb1043649369d5956b123b173b799ded8c6b7cbf4519bb32cb3caa01928d75R935.

store/copr/region_cache.go

sticnarf · 2022-03-10T08:58:52Z

store/copr/coprocessor.go

@@ -180,7 +181,8 @@ func buildCopTasks(bo *Backoffer, cache *RegionCache, ranges *KeyRanges, req *kv

 	rangesLen := ranges.Len()

-	locs, err := cache.SplitKeyRangesByLocations(bo, ranges)
+	// TODO(youjiali1995): is there any request type that needn't be splitted by buckets?


Does TiFlash ignore buckets?

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

sticnarf

LGTM

store/copr/region_cache.go

sticnarf · 2022-03-10T09:36:10Z

store/copr/region_cache.go

+			var i int
+			for ; i < ranges.Len(); i++ {
+				r := ranges.At(i)
+				if !(bucket.Contains(r.EndKey) || bytes.Equal(bucket.EndKey, r.EndKey)) {


Why not a simple bytes.Compare(r.EndKey, bucket.EndKey) > 0?

Should be len(bucket.EndKey) != 0 && bytes.Compare(r.EndKey, bucket.EndKey) > 0. It's not simpler.

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

cfzjywxk

LGTM

youjiali1995 · 2022-03-11T07:27:07Z

/merge

ti-chi-bot · 2022-03-11T07:27:10Z

This pull request has been accepted and is ready to merge.

Commit hash: dc0981c

youjiali1995 · 2022-03-11T07:39:51Z

/run-unit-test

youjiali1995 · 2022-03-11T07:46:25Z

/merge

youjiali1995 · 2022-03-11T08:17:44Z

/merge

youjiali1995 · 2022-03-11T08:35:44Z

/run-unit-test

ti-chi-bot added do-not-merge/needs-linked-issue do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Mar 4, 2022

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 4, 2022

youjiali1995 force-pushed the copr-bucket branch from f23f8cf to dc13c53 Compare March 7, 2022 10:19

youjiali1995 mentioned this pull request Mar 8, 2022

region_cache: support buckets tikv/client-go#439

Merged

youjiali1995 added 2 commits March 9, 2022 16:09

copr: split copr requests by buckets

e8e898a

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

Merge branch 'master' of https://github.com/pingcap/tidb into copr-bu…

ced3b50

…cket Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

youjiali1995 force-pushed the copr-bucket branch from dc13c53 to ced3b50 Compare March 9, 2022 08:13

fix go.mod

340d8af

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

youjiali1995 force-pushed the copr-bucket branch from c018d47 to 340d8af Compare March 9, 2022 08:45

fix tests

374f3a2

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

youjiali1995 force-pushed the copr-bucket branch from 583f26c to 374f3a2 Compare March 9, 2022 09:14

Merge branch 'master' into copr-bucket

1ec3f08

ti-chi-bot removed the do-not-merge/needs-linked-issue label Mar 9, 2022

youjiali1995 marked this pull request as ready for review March 9, 2022 10:13

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 9, 2022

Merge branch 'master' into copr-bucket

9cc24d0

youjiali1995 requested review from sticnarf and cfzjywxk March 9, 2022 10:13

Merge branch 'master' into copr-bucket

67db774

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2022

cfzjywxk reviewed Mar 10, 2022

View reviewed changes

Merge branch 'master' into copr-bucket

861bb76

sticnarf reviewed Mar 10, 2022

View reviewed changes

youjiali1995 force-pushed the copr-bucket branch from 23a09eb to 0b24a84 Compare March 10, 2022 09:27

address comments

108221b

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

youjiali1995 force-pushed the copr-bucket branch from 0b24a84 to 108221b Compare March 10, 2022 09:54

sticnarf approved these changes Mar 10, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 10, 2022

move r out of the loop

dc0981c

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

cfzjywxk approved these changes Mar 11, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 11, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 11, 2022

Merge branch 'master' into copr-bucket

46e273a

ti-chi-bot and others added 2 commits March 11, 2022 15:46

Merge branch 'master' into copr-bucket

c4e2a99

Merge branch 'master' into copr-bucket

923f13a

Merge branch 'master' into copr-bucket

4f86e6c

ti-chi-bot merged commit 328a8bc into pingcap:master Mar 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

copr: split copr requests by buckets #32818

copr: split copr requests by buckets #32818

youjiali1995 commented Mar 4, 2022 •

edited by SpadeA-Tang

Loading

ti-chi-bot commented Mar 4, 2022 •

edited

Loading

ti-chi-bot commented Mar 4, 2022

youjiali1995 commented Mar 9, 2022

sre-bot commented Mar 9, 2022 •

edited

Loading

youjiali1995 commented Mar 9, 2022

cfzjywxk Mar 10, 2022

youjiali1995 Mar 10, 2022

sticnarf Mar 10, 2022

youjiali1995 Mar 10, 2022

cfzjywxk Mar 10, 2022

youjiali1995 Mar 10, 2022

sticnarf left a comment

sticnarf Mar 10, 2022

youjiali1995 Mar 10, 2022

sticnarf Mar 10, 2022

youjiali1995 Mar 10, 2022

sticnarf Mar 10, 2022

sticnarf left a comment

sticnarf Mar 10, 2022

youjiali1995 Mar 11, 2022

cfzjywxk left a comment

youjiali1995 commented Mar 11, 2022

ti-chi-bot commented Mar 11, 2022

youjiali1995 commented Mar 11, 2022

youjiali1995 commented Mar 11, 2022

youjiali1995 commented Mar 11, 2022

youjiali1995 commented Mar 11, 2022

		@@ -319,7 +322,6 @@ type copIteratorWorker struct {

		replicaReadSeed uint32

		actionOnExceed *rateLimitAction

copr: split copr requests by buckets #32818

copr: split copr requests by buckets #32818

Conversation

youjiali1995 commented Mar 4, 2022 • edited by SpadeA-Tang Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Mar 4, 2022 • edited Loading

ti-chi-bot commented Mar 4, 2022

youjiali1995 commented Mar 9, 2022

sre-bot commented Mar 9, 2022 • edited Loading

youjiali1995 commented Mar 9, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sticnarf left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sticnarf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfzjywxk left a comment

Choose a reason for hiding this comment

youjiali1995 commented Mar 11, 2022

ti-chi-bot commented Mar 11, 2022

youjiali1995 commented Mar 11, 2022

youjiali1995 commented Mar 11, 2022

youjiali1995 commented Mar 11, 2022

youjiali1995 commented Mar 11, 2022

youjiali1995 commented Mar 4, 2022 •

edited by SpadeA-Tang

Loading

ti-chi-bot commented Mar 4, 2022 •

edited

Loading

sre-bot commented Mar 9, 2022 •

edited

Loading