schedule: revise region_scatter distribution for multi groups #3422

Yisaer · 2021-02-18T12:22:38Z

Signed-off-by: Song Gao [email protected]

What problem does this PR solve?

Currently region-scatter didn't work well for multi regions in different groups.

For example, there are 50 regions and 6 stores in the cluster. Each region belong to a dedicated group.

region scatter may locate them in the store1/store2/store3. Thus from each region in the group, the distribution is well. And for the whole cluster, the distribution is not well.

What is changed and how it works?

During region scatter, we should consider whether the target store already have the most peer or leader, if it is, we should skip it.

Check List

Tests

Unit test

I also tested with the testcases in this document: https://docs.google.com/document/d/1cGZU2DCm0UuIueqAQP8TGqTivyavCvB_QiwIzqvP_rA/edit

All the test case results didn't change.

Release note

No release note

Signed-off-by: Song Gao <[email protected]>

codecov · 2021-02-18T12:50:55Z

Codecov Report

Merging #3422 (3eb838b) into master (f204416) will decrease coverage by 0.18%.
The diff coverage is 94.28%.

@@            Coverage Diff             @@
##           master    #3422      +/-   ##
==========================================
- Coverage   74.75%   74.57%   -0.19%     
==========================================
  Files         245      245              
  Lines       24107    24136      +29     
==========================================
- Hits        18022    17999      -23     
- Misses       4480     4516      +36     
- Partials     1605     1621      +16

Flag	Coverage Δ
unittests	`74.57% <94.28%> (-0.19%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
server/schedule/operator/builder.go	`87.98% <ø> (ø)`
server/schedule/region_scatterer.go	`88.37% <92.85%> (+1.32%)`	⬆️
pkg/cache/ttl.go	`97.77% <100.00%> (+0.15%)`	⬆️
server/schedule/operator/create_operator.go	`89.76% <100.00%> (+0.08%)`	⬆️
server/id/id.go	`76.19% <0.00%> (-4.77%)`	⬇️
server/tso/allocator_manager.go	`73.92% <0.00%> (-3.99%)`	⬇️
server/schedulers/random_merge.go	`60.00% <0.00%> (-3.34%)`	⬇️
server/member/member.go	`65.59% <0.00%> (-2.69%)`	⬇️
client/base_client.go	`83.24% <0.00%> (-2.17%)`	⬇️
server/server.go	`71.42% <0.00%> (-2.16%)`	⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f204416...3eb838b. Read the comment docs.

server/schedule/region_scatterer_test.go

Signed-off-by: Song Gao <[email protected]>

nolouch · 2021-02-25T10:18:09Z

What might become worse if there is concurrency? I remember that there was a situation where there was a concurrent split of different tables.

rleungx · 2021-02-26T07:06:52Z

server/schedule/region_scatterer.go

-				targetPeers[peer.GetStoreId()] = peer
-				continue
+			storeCount := context.selectedPeer.storeTotalCount(peer.GetStoreId())
+			if storeCount < maxStoreTotalCount || storeCount == minStoreTotalCount {


What is the difference between storeCount < maxStoreTotalCount and storeCount == minStoreTotalCount?

If the storeCounts are all the same( maxStoreTotalCount == minStoreTotalCount), we can pick this store as candidate.

server/schedule/region_scatterer_test.go

Signed-off-by: Song Gao <[email protected]>

Yisaer · 2021-04-01T09:30:23Z

/run-all-tests

nolouch

lgtm

ti-chi-bot · 2021-04-21T04:02:19Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato
nolouch

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 writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

nolouch · 2021-04-21T04:02:36Z

/merge

ti-chi-bot · 2021-04-21T04:02:36Z

@nolouch: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

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 ti-community-infra/tichi repository.

ti-chi-bot · 2021-04-21T04:02:38Z

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

Commit hash: e91faed

ti-chi-bot · 2021-04-21T04:02:45Z

@Yisaer: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

ti-chi-bot · 2021-04-21T04:21:24Z

Merge canceled because a new commit is pushed.

Signed-off-by: yisaer <[email protected]>

sonarcloud · 2021-04-21T04:25:51Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

Signed-off-by: ti-srebot <[email protected]>

ti-srebot · 2021-04-21T04:46:28Z

cherry pick to release-4.0 in PR #3620

Signed-off-by: ti-srebot <[email protected]>

ti-srebot · 2021-04-21T04:47:03Z

cherry pick to release-5.0 in PR #3621

…#3620) * cherry pick #3422 to release-4.0 Signed-off-by: ti-srebot <[email protected]> * fix conflict Signed-off-by: yisaer <[email protected]> * remove test Signed-off-by: yisaer <[email protected]> * remove test Signed-off-by: yisaer <[email protected]> Co-authored-by: Song Gao <[email protected]>

…#3621) Signed-off-by: ti-srebot <[email protected]> Co-authored-by: Song Gao <[email protected]> Co-authored-by: Ti Chi Robot <[email protected]>

close #6962 In past, PD conside peer distribution in the different group influenece by using `TotalCountByStore` , but not include the leader distribution. The max used situation is partition table. After this pr, TIDB call scatter api will use same group not different. ref: #3422 pingcap/tidb#46156 Signed-off-by: bufferflies <[email protected]> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

close #6962 In past, PD conside peer distribution in the different group influenece by using `TotalCountByStore` , but not include the leader distribution. The max used situation is partition table. After this pr, TIDB call scatter api will use same group not different. ref: #3422 pingcap/tidb#46156 Signed-off-by: ti-chi-bot <[email protected]> Signed-off-by: bufferflies <[email protected]> Co-authored-by: buffer <[email protected]> Co-authored-by: bufferflies <[email protected]> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

fix scatter region in multi groups

eb3e66f

Signed-off-by: Song Gao <[email protected]>

ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 18, 2021

ti-chi-bot requested a review from rleungx February 18, 2021 12:22

Yisaer added 2 commits February 18, 2021 20:31

fix test

37ceec2

Signed-off-by: Song Gao <[email protected]>

fix test

5e2e2ae

Signed-off-by: Song Gao <[email protected]>

Yisaer marked this pull request as ready for review February 19, 2021 04:48

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 Feb 19, 2021

lhy1024 self-requested a review February 19, 2021 08:39

HunDunDM added the component/cluster Cluster logic. label Feb 20, 2021

Yisaer requested a review from disksing February 23, 2021 08:40

lhy1024 reviewed Feb 23, 2021

View reviewed changes

server/schedule/region_scatterer_test.go Outdated Show resolved Hide resolved

server/schedule/region_scatterer_test.go Show resolved Hide resolved

Yisaer added 2 commits February 24, 2021 18:24

add comment

0b9e93d

Signed-off-by: Song Gao <[email protected]>

improve test

a11927f

Signed-off-by: Song Gao <[email protected]>

rleungx reviewed Feb 26, 2021

View reviewed changes

server/schedule/region_scatterer_test.go Outdated Show resolved Hide resolved

Yisaer added 2 commits March 2, 2021 13:29

Merge remote-tracking branch 'upstream/master' into fix_scatter

4dfa057

fix test

c6d532d

Signed-off-by: Song Gao <[email protected]>

Yisaer marked this pull request as draft March 5, 2021 03:20

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

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

Yisaer closed this Mar 17, 2021

Yisaer reopened this Mar 17, 2021

Yisaer mentioned this pull request Apr 1, 2021

store/tikv: pipe the split and scatter task pingcap/tidb#23357

Closed

Merge remote-tracking branch 'upstream/master' into fix_scatter

344ceb5

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

Yisaer marked this pull request as ready for review April 1, 2021 05:22

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 Apr 1, 2021

nolouch approved these changes Apr 21, 2021

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 Apr 21, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 21, 2021

Merge branch 'master' into fix_scatter

6b33f0a

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Apr 21, 2021

address the comment

3eb838b

Signed-off-by: yisaer <[email protected]>

Yisaer force-pushed the fix_scatter branch from db784ff to 3eb838b Compare April 21, 2021 04:25

Yisaer added the status/can-merge Indicates a PR has been approved by a committer. label Apr 21, 2021

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Apr 21, 2021

Yisaer merged commit 9e60f4d into tikv:master Apr 21, 2021

ti-srebot pushed a commit to ti-srebot/pd that referenced this pull request Apr 21, 2021

cherry pick tikv#3422 to release-4.0

fa2e920

Signed-off-by: ti-srebot <[email protected]>

ti-srebot mentioned this pull request Apr 21, 2021

schedule: revise region_scatter distribution for multi groups (#3422) #3620

Merged

ti-srebot pushed a commit to ti-srebot/pd that referenced this pull request Apr 21, 2021

cherry pick tikv#3422 to release-5.0

1ff15bc

Signed-off-by: ti-srebot <[email protected]>

ti-srebot mentioned this pull request Apr 21, 2021

schedule: revise region_scatter distribution for multi groups (#3422) #3621

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schedule: revise region_scatter distribution for multi groups #3422

schedule: revise region_scatter distribution for multi groups #3422

Yisaer commented Feb 18, 2021 •

edited

Loading

codecov bot commented Feb 18, 2021 •

edited

Loading

nolouch commented Feb 25, 2021

rleungx Feb 26, 2021

Yisaer Apr 7, 2021

Yisaer commented Apr 1, 2021

nolouch left a comment

ti-chi-bot commented Apr 21, 2021

nolouch commented Apr 21, 2021

ti-chi-bot commented Apr 21, 2021

ti-chi-bot commented Apr 21, 2021

ti-chi-bot commented Apr 21, 2021

ti-chi-bot commented Apr 21, 2021

sonarcloud bot commented Apr 21, 2021

ti-srebot commented Apr 21, 2021

ti-srebot commented Apr 21, 2021

schedule: revise region_scatter distribution for multi groups #3422

schedule: revise region_scatter distribution for multi groups #3422

Conversation

Yisaer commented Feb 18, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

codecov bot commented Feb 18, 2021 • edited Loading

Codecov Report

nolouch commented Feb 25, 2021

rleungx Feb 26, 2021

Choose a reason for hiding this comment

Yisaer Apr 7, 2021

Choose a reason for hiding this comment

Yisaer commented Apr 1, 2021

nolouch left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Apr 21, 2021

nolouch commented Apr 21, 2021

ti-chi-bot commented Apr 21, 2021

ti-chi-bot commented Apr 21, 2021

ti-chi-bot commented Apr 21, 2021

ti-chi-bot commented Apr 21, 2021

sonarcloud bot commented Apr 21, 2021

ti-srebot commented Apr 21, 2021

ti-srebot commented Apr 21, 2021

Yisaer commented Feb 18, 2021 •

edited

Loading

codecov bot commented Feb 18, 2021 •

edited

Loading