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

*: support a region divided into multiple regions #11739

Merged
merged 15 commits into from
Sep 3, 2019

Conversation

zimulala
Copy link
Contributor

@zimulala zimulala commented Aug 14, 2019

What problem does this PR solve?

Serial splitting and scattering regions take a long time.
Related to pingcap/kvproto#440, tikv/tikv#5268

What is changed and how it works?

If some split keys in a region, we can batch split these keys. Then we can split a region into several regions in a request. And we split of different regions in parallel.

Check List

Tests

  • No code

Code changes

  • Has exported function/method change
  • Has interface methods change

@zimulala zimulala added status/DNM sig/execution SIG execution require-LGT3 Indicates that the PR requires three LGTM. and removed status/DNM labels Aug 14, 2019
@codecov
Copy link

codecov bot commented Aug 22, 2019

Codecov Report

Merging #11739 into master will increase coverage by 0.0045%.
The diff coverage is 72.8915%.

@@               Coverage Diff                @@
##             master     #11739        +/-   ##
================================================
+ Coverage   81.3475%   81.3521%   +0.0045%     
================================================
  Files           444        444                
  Lines         95103      95180        +77     
================================================
+ Hits          77364      77431        +67     
- Misses        12233      12246        +13     
+ Partials       5506       5503         -3

@zimulala
Copy link
Contributor Author

PTAL @crazycs520 @bb7133

@zimulala
Copy link
Contributor Author

PTAL @crazycs520 @bb7133

@winkyao
Copy link
Contributor

winkyao commented Aug 24, 2019

@zimulala Please give some results of performance improvement

if len(regionIDs) == 1 {
return regionIDs[0]
}
return 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we return an error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the original code and several functions on the upper layer also did not return an error message.

store/tikv/split_region.go Outdated Show resolved Hide resolved
@zimulala zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 30, 2019
@zimulala
Copy link
Contributor Author

PTAL @winkyao @nolouch

Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

}
regionIDs = append(regionIDs, regionID)

regionIDs, err := s.SplitRegions(context.Background(), splitIdxKeys, true)
Copy link
Member

Choose a reason for hiding this comment

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

Why not using ctxWithTimeout here? IMHO a timeout is still needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original code hasn't and SplitRegions has itself timeout, so I don't add it.
If you feel the need, is there a suggested value?

Copy link
Member

Choose a reason for hiding this comment

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

OK, never mind

store/tikv/split_region.go Outdated Show resolved Hide resolved
store/tikv/split_region.go Outdated Show resolved Hide resolved
store/tikv/split_region.go Outdated Show resolved Hide resolved
store/tikv/split_region.go Outdated Show resolved Hide resolved
store/tikv/split_region.go Outdated Show resolved Hide resolved
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 2, 2019
bb7133
bb7133 previously approved these changes Sep 2, 2019
Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

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

LGTM

@bb7133 bb7133 added status/LGT3 The PR has already had 3 LGTM. status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Sep 2, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 2, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Sep 2, 2019

@zimulala merge failed.

@zimulala
Copy link
Contributor Author

zimulala commented Sep 2, 2019

/run-integration-common-test

@zimulala
Copy link
Contributor Author

zimulala commented Sep 3, 2019

/run-all-tests

@zimulala zimulala merged commit f9d8541 into pingcap:master Sep 3, 2019
@zimulala zimulala deleted the split-resions branch September 3, 2019 05:30
@sre-bot
Copy link
Contributor

sre-bot commented Sep 3, 2019

cherry pick to release-3.0 failed

@sre-bot
Copy link
Contributor

sre-bot commented Sep 3, 2019

cherry pick to release-2.1 failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
require-LGT3 Indicates that the PR requires three LGTM. sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants