raftstore: region initial size depends on the split resource . #15456

bufferflies · 2023-08-28T09:11:52Z

What is changed and how it works?

Issue Number: Close #15457
depends: pingcap/kvproto#1177
What's Changed:

there are three triggers will split the regions:
1. load split include sizekeys, load etc. In this cases, the new region should contains the data  after split.
2. tidb split tables or partition table, such like `create table test.t1(id int,b int) shard_row_id_bits=4 partition by hash(id) partitions 2000`. 
In this cases , the new region shouldn't contains any data  after split.

Related changes

Check List

Tests

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

Side effects

Release note

None

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

ti-chi-bot · 2023-08-28T09:11:54Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

SpadeA-Tang
tonyxuqqi

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.

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

Cargo.toml

components/raftstore/src/store/worker/pd.rs

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

components/raftstore-v2/src/operation/command/admin/split.rs

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

Connor1996 · 2023-09-04T07:40:26Z

components/raftstore-v2/src/operation/command/admin/split.rs

@@ -75,6 +75,9 @@ pub struct SplitResult {
    // The index of the derived region in `regions`
    pub derived_index: usize,
    pub tablet_index: u64,
+    // new regions will amortize the region size if it's true.
+    // otherwise, the new region's size will be 0.
+    pub amortize_source_region_size: bool,


amortize means gradually write off the initial cost of (an asset) over a period. It's not proper here.

I think the name can be size_based_split: bool

has renamed share_source_region_size .

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

…-size

tonyxuqqi · 2023-09-04T08:42:37Z

components/raftstore-v2/src/operation/command/admin/split.rs

-        let estimated_keys = control.approximate_keys.map(|v| v / new_region_count);
+        // if share_source_region_size is true, it means the new region contains any
+        // data from the origin region.
+        let mut amortize_size = None;


rename these amortize_xxx as well

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

bufferflies · 2023-09-04T09:46:41Z

/merge

ti-chi-bot · 2023-09-04T09:46:43Z

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

/run-all-tests

You only need to trigger /merge once, and 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.

If you have any questions about the PR merge process, please refer to pr process.

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 · 2023-09-04T09:46:46Z

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

Commit hash: 47ea9f3

close tikv#15457 Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2023-09-05T03:04:02Z

In response to a cherrypick label: new pull request created to branch release-6.5: #15509.

ti-chi-bot · 2023-09-05T03:04:19Z

In response to a cherrypick label: new pull request created to branch release-7.1: #15510.

close tikv#15457 Signed-off-by: ti-chi-bot <[email protected]>

… (#15509) close #15457 there are three triggers will split the regions: 1. load split include sizekeys, load etc. In this cases, the new region should contains the data after split. 2. tidb split tables or partition table, such like `create table test.t1(id int,b int) shard_row_id_bits=4 partition by hash(id) partitions 2000`. In this cases , the new region shouldn't contains any data after split. 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]>

…15456) (tikv#15509) close tikv#15457 there are three triggers will split the regions: 1. load split include sizekeys, load etc. In this cases, the new region should contains the data after split. 2. tidb split tables or partition table, such like `create table test.t1(id int,b int) shard_row_id_bits=4 partition by hash(id) partitions 2000`. In this cases , the new region shouldn't contains any data after split. 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]>

… (#15510) close #15457 there are three triggers will split the regions: 1. load split include sizekeys, load etc. In this cases, the new region should contains the data after split. 2. tidb split tables or partition table, such like `create table test.t1(id int,b int) shard_row_id_bits=4 partition by hash(id) partitions 2000`. In this cases , the new region shouldn't contains any data after split. 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>

region size

4493549

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

lint

c8c3b34

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

bufferflies force-pushed the region-size branch from 3552d5b to c8c3b34 Compare August 28, 2023 09:49

add unit test

870a30a

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

bufferflies marked this pull request as ready for review August 29, 2023 05:07

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

lint

038b63b

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

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 29, 2023

bufferflies changed the title ~~raftstore: region size depends on the split resource .~~ raftstore: region initial size depends on the split resource . Aug 29, 2023

bufferflies requested review from rleungx, overvenus and SpadeA-Tang August 29, 2023 07:32

overvenus reviewed Aug 31, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

components/raftstore/src/store/worker/pd.rs Outdated Show resolved Hide resolved

bufferflies added 4 commits August 31, 2023 17:35

lint

a361c82

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

region size

82a504e

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

update kvproto

6635990

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

resolve conflict

b6f6ccf

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

bufferflies force-pushed the region-size branch from 82f958a to b6f6ccf Compare August 31, 2023 09:56

SpadeA-Tang reviewed Aug 31, 2023

View reviewed changes

components/raftstore-v2/src/operation/command/admin/split.rs Outdated Show resolved Hide resolved

bufferflies added 2 commits September 4, 2023 11:35

update kvproto

2f7eca4

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

update kvproto

37e7e7e

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

SpadeA-Tang approved these changes Sep 4, 2023

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 4, 2023

bufferflies requested a review from overvenus September 4, 2023 06:57

Connor1996 reviewed Sep 4, 2023

View reviewed changes

bufferflies added 3 commits September 4, 2023 16:22

size

703d69c

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

rename amortize_source_region_size to share_source_region_size

142bcd8

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

Merge branch 'region-size' of github.com:bufferflies/tikv into region…

3ddf48c

…-size

tonyxuqqi reviewed Sep 4, 2023

View reviewed changes

tonyxuqqi approved these changes Sep 4, 2023

View reviewed changes

ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 4, 2023

bufferflies added 2 commits September 4, 2023 16:50

address comment

fee7e4d

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

Merge branch 'master' into region-size

47ea9f3

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 4, 2023

ti-chi-bot bot merged commit 640143a into tikv:master Sep 4, 2023

ti-chi-bot bot added this to the Pool milestone Sep 4, 2023

bufferflies added needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. labels Sep 5, 2023

ti-chi-bot pushed a commit to ti-chi-bot/tikv that referenced this pull request Sep 5, 2023

This is an automated cherry-pick of tikv#15456

64538c3

close tikv#15457 Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Sep 5, 2023

raftstore: region initial size depends on the split resource . (#15456) #15509

Merged

ti-chi-bot mentioned this pull request Sep 5, 2023

raftstore: region initial size depends on the split resource . (#15456) #15510

Merged

ti-chi-bot pushed a commit to ti-chi-bot/tikv that referenced this pull request Sep 5, 2023

This is an automated cherry-pick of tikv#15456

73ce8b5

close tikv#15457 Signed-off-by: ti-chi-bot <[email protected]>

bufferflies mentioned this pull request Sep 6, 2023

raftstore: region initial size depends on the split policy. #15536

Merged

tonyxuqqi mentioned this pull request Jan 29, 2024

After spliting regions, the size of the new regions are not updated to zero but None. #15696

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raftstore: region initial size depends on the split resource . #15456

raftstore: region initial size depends on the split resource . #15456

bufferflies commented Aug 28, 2023 •

edited

Loading

ti-chi-bot bot commented Aug 28, 2023 •

edited

Loading

Connor1996 Sep 4, 2023

tonyxuqqi Sep 4, 2023

tonyxuqqi Sep 4, 2023

bufferflies Sep 4, 2023

tonyxuqqi Sep 4, 2023

bufferflies commented Sep 4, 2023

ti-chi-bot bot commented Sep 4, 2023

ti-chi-bot bot commented Sep 4, 2023

ti-chi-bot commented Sep 5, 2023

ti-chi-bot commented Sep 5, 2023

raftstore: region initial size depends on the split resource . #15456

raftstore: region initial size depends on the split resource . #15456

Conversation

bufferflies commented Aug 28, 2023 • edited Loading

What is changed and how it works?

Related changes

Check List

Release note

ti-chi-bot bot commented Aug 28, 2023 • edited Loading

Connor1996 Sep 4, 2023

Choose a reason for hiding this comment

tonyxuqqi Sep 4, 2023

Choose a reason for hiding this comment

tonyxuqqi Sep 4, 2023

Choose a reason for hiding this comment

bufferflies Sep 4, 2023

Choose a reason for hiding this comment

tonyxuqqi Sep 4, 2023

Choose a reason for hiding this comment

bufferflies commented Sep 4, 2023

ti-chi-bot bot commented Sep 4, 2023

ti-chi-bot bot commented Sep 4, 2023

ti-chi-bot commented Sep 5, 2023

ti-chi-bot commented Sep 5, 2023

bufferflies commented Aug 28, 2023 •

edited

Loading

ti-chi-bot bot commented Aug 28, 2023 •

edited

Loading