copr: Split more accurately when buckey keys are not accurate. #34290

SpadeA-Tang · 2022-04-27T09:48:17Z

Signed-off-by: SpadeA-Tang u6748471@anu.edu.au

What problem does this PR solve?

Issue Number: close #34287

Problem Summary:

splitKeyRangesByBuckets may not split tasks accurately when there are some disagreement between region and its bucket keys.
For example, when we miss n and x in buckets which are the start and end of the region, we can inaccurately split the task ranges.
region: n------------------x
buckets: q---s---u
ranges: n--o p--s t--v w-x
Got: [n-o], [p-s], [t--u], [u-v, w-x]
Expected: [n-o, p-q], [q-s], [t--u], [u-v, w-x]
This PR fix this problem so that we can split the ranges as expected.

Related client-go changes:
tikv/client-go#486
tikv/client-go#497

Tests

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

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

ti-chi-bot · 2022-04-27T09:48:20Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

cfzjywxk
youjiali1995

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.

sre-bot · 2022-04-27T09:49:07Z

Please follow PR Title Format:

pkg [, pkg2, pkg3]: what is changed

Or if the count of mainly changed packages are more than 3, use

*: what is changed

After you have format title, you can leave a comment /run-check_title to recheck it

CLAassistant · 2022-04-27T09:49:48Z

All committers have signed the CLA.

sre-bot · 2022-04-27T09:58:29Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/6912adb6ed4ae44bc12d43d55b093f480f6535a3

store/copr/coprocessor_test.go

cfzjywxk · 2022-04-27T13:17:05Z

store/copr/region_cache.go

+	// To easy the implementation of this function, we add region startKey if it is less than the first bucket key and add region endKey if
+	// it is larger than the last bucket key. Without the addition, loc.LocateBucket may return nil which can cause troubles.
+	if len(loc.Buckets.Keys) == 0 || bytes.Compare(loc.StartKey, loc.Buckets.Keys[0]) < 0 {
+		loc.Buckets.Keys = append([][]byte{loc.StartKey}, loc.Buckets.Keys...)


The content of the location should not be changed here, it's read-only in the original implementation. A possible option is to check the end_key if LocateBucket returns nil or changing the logic of LocateBucket .
There's no mutable constraint in golang, be very careful if you decide to change the content of some input references.

cfzjywxk · 2022-04-27T13:20:27Z

store/copr/region_cache.go

 	res := []*LocationKeyRanges{}
 	for ranges.Len() > 0 {
 		bucket := loc.LocateBucket(ranges.At(0).StartKey)
-		if bucket == nil {


Why remove the nil check, seems the loc.LocateBucket could still return nil results.

Ditto, this should not be removed directly.

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

cfzjywxk · 2022-04-28T07:58:20Z

store/copr/region_cache.go

+	if len(loc.Buckets.Keys) == 0 || (bytes.Compare(loc.StartKey, loc.Buckets.Keys[0]) < 0 ||
+		bytes.Compare(loc.Buckets.Keys[len(loc.Buckets.Keys)-1], loc.EndKey) < 0) {
+		// If we need to modify the location, we copy and modify it.
+		locCopy := *loc


This may not use deep copy, it's necessary to use a clone or deep copy method if you want to deep copy or clone an object.
BTW, the bucket key meta has its specific meaning, it's better to change the split or task generation algorithm instead of changing meta info.

You could discuss the split and bucket task split algorithm and implementation with @youjiali1995 first I think.

cfzjywxk · 2022-04-28T07:58:50Z

store/copr/region_cache.go

 	res := []*LocationKeyRanges{}
 	for ranges.Len() > 0 {
 		bucket := loc.LocateBucket(ranges.At(0).StartKey)
-		if bucket == nil {


Ditto, this should not be removed directly.

cfzjywxk · 2022-04-28T08:00:28Z

store/copr/region_cache.go

+func getKeyLocation(l *LocationKeyRanges) *tikv.KeyLocation {
+	// Sometimes, there may be some delay between region information and buckets information within the region.
+	// It may cause some gaps between the region startKey and first buckets key and region endKey and the last buckets key.
+	// To easy the implementation of splitKeyRangeByBuckets, we copy the l.Location (with original value untouched) and add region startKey


To ease the implementation? Some tools like Grammarly could help reduce grammatical mistakes.

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

store/copr/region_cache.go

store/copr/coprocessor_test.go

youjiali1995 · 2022-05-12T05:29:27Z

store/copr/coprocessor_test.go

+		taskEqual(t, task, regionIDs[1], regionIDs[1], expectedTaskRanges[i]...)
+	}
+
+	// check edge


What's the difference between it and 'cover the whole region'?

store/copr/coprocessor_test.go

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

…t-split

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

cfzjywxk · 2022-05-12T14:29:16Z

@SpadeA-Tang
Please link the related client-go change pr and add it in the description.

cfzjywxk · 2022-05-12T14:30:32Z

store/copr/region_cache.go

-			if i == ranges.Len() {
-				res = append(res, &LocationKeyRanges{l.Location, ranges})
+		// ranges must be in loc.region, so the bucket returned by loc.LocateBucketV2 is guaranteed to be not nil
+		bucket := loc.LocateBucketV2(ranges.At(0).StartKey)


What does the V2 mean? Do we need to use V1 somewhere else? If not we could remove the version in the name and try to avoid misunderstranding.

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

cfzjywxk · 2022-05-16T04:17:06Z

/merge

ti-chi-bot · 2022-05-16T04:17:09Z

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

Commit hash: 305c75a

SpadeA-Tang · 2022-05-16T04:40:51Z

/run-unit-test

SpadeA-Tang · 2022-05-16T05:21:27Z

/run-mysql-test

sre-bot · 2022-05-16T05:57:32Z

TiDB MergeCI notify

🔴 Bad News! New failing [2] after this pr merged.
These new failed integration tests seem to be caused by the current PR, please try to fix these new failed integration tests, thanks!

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/integration-ddl-test	🟥 failed 1, success 5, total 6	10 min	New failing
idc-jenkins-ci-tidb/common-test	🟥 failed 1, success 11, total 12	5 min 22 sec	New failing
idc-jenkins-ci/integration-cdc-test	🟢 all 34 tests passed	25 min	Existing passed
idc-jenkins-ci-tidb/integration-common-test	🟢 all 11 tests passed	17 min	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	6 min 44 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	5 min 47 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	5 min 37 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	3 min 19 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	3 min 1 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

SpadeA-Tang added 2 commits April 27, 2022 17:35

Add some tests. Supplyment bucket keys if needed.

bdb618e

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

Modify the comment

8dea883

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

ti-chi-bot added release-note-none size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 27, 2022

SpadeA-Tang changed the title ~~Add some tests. Supplyment bucket keys if needed.~~ Fix splitKeyRangesByBuckets when bucket keys are not accurate. Apr 27, 2022

SpadeA-Tang changed the title ~~Fix splitKeyRangesByBuckets when bucket keys are not accurate.~~ Split more accurately when buckey keys are not accurate. Apr 27, 2022

SpadeA-Tang changed the title ~~Split more accurately when buckey keys are not accurate.~~ copr: Split more accurately when buckey keys are not accurate. Apr 27, 2022

Merge branch 'master' into bucket-split

26e840a

SpadeA-Tang requested review from sticnarf and youjiali1995 April 27, 2022 10:54

cfzjywxk reviewed Apr 27, 2022

View reviewed changes

SpadeA-Tang added 2 commits April 28, 2022 13:00

fix the problem of changing read only LocationKeyRanges

f680e49

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

Merge remote-tracking branch 'origin/bucket-split' into bucket-split

6d6b87d

SpadeA-Tang requested a review from cfzjywxk April 28, 2022 05:13

cfzjywxk reviewed Apr 28, 2022

View reviewed changes

SpadeA-Tang and others added 4 commits May 9, 2022 13:02

update

91160de

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

modify it according to the change of client-go

8b353e6

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

add comment

8278326

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

Merge branch 'master' into bucket-split

8705218

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

youjiali1995 reviewed May 12, 2022

View reviewed changes

SpadeA-Tang and others added 5 commits May 12, 2022 13:57

update

0cb369c

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

Merge branch 'bucket-split' of github.com:SpadeA-Tang/tidb into bucke…

f245489

…t-split

update

d818c51

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

Merge branch 'master' into bucket-split

4c3b78b

update

df71671

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

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

update

5dab061

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

youjiali1995 approved these changes May 12, 2022

View reviewed changes

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

cfzjywxk reviewed May 12, 2022

View reviewed changes

SpadeA-Tang added 2 commits May 13, 2022 17:31

update

18e9d51

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

update go mod

305c75a

Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>

cfzjywxk approved these changes May 16, 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 May 16, 2022

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

Merge branch 'master' into bucket-split

6912adb

ti-chi-bot merged commit 4b00ee2 into pingcap:master May 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

copr: Split more accurately when buckey keys are not accurate. #34290

copr: Split more accurately when buckey keys are not accurate. #34290

SpadeA-Tang commented Apr 27, 2022 •

edited

Loading

ti-chi-bot commented Apr 27, 2022 •

edited

Loading

sre-bot commented Apr 27, 2022

CLAassistant commented Apr 27, 2022 •

edited by sre-bot

Loading

sre-bot commented Apr 27, 2022 •

edited

Loading

cfzjywxk Apr 27, 2022

cfzjywxk Apr 27, 2022

cfzjywxk Apr 28, 2022

cfzjywxk Apr 28, 2022

cfzjywxk Apr 28, 2022

cfzjywxk Apr 28, 2022

cfzjywxk Apr 28, 2022

youjiali1995 May 12, 2022

cfzjywxk commented May 12, 2022

cfzjywxk May 12, 2022

cfzjywxk commented May 16, 2022

ti-chi-bot commented May 16, 2022

SpadeA-Tang commented May 16, 2022

SpadeA-Tang commented May 16, 2022

sre-bot commented May 16, 2022

copr: Split more accurately when buckey keys are not accurate. #34290

copr: Split more accurately when buckey keys are not accurate. #34290

Conversation

SpadeA-Tang commented Apr 27, 2022 • edited Loading

What problem does this PR solve?

Release note

ti-chi-bot commented Apr 27, 2022 • edited Loading

sre-bot commented Apr 27, 2022

CLAassistant commented Apr 27, 2022 • edited by sre-bot Loading

sre-bot commented Apr 27, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfzjywxk commented May 12, 2022

Choose a reason for hiding this comment

cfzjywxk commented May 16, 2022

ti-chi-bot commented May 16, 2022

SpadeA-Tang commented May 16, 2022

SpadeA-Tang commented May 16, 2022

sre-bot commented May 16, 2022

TiDB MergeCI notify

SpadeA-Tang commented Apr 27, 2022 •

edited

Loading

ti-chi-bot commented Apr 27, 2022 •

edited

Loading

CLAassistant commented Apr 27, 2022 •

edited by sre-bot

Loading

sre-bot commented Apr 27, 2022 •

edited

Loading