raftstore-v2: support tablet split #13709

SpadeA-Tang · 2022-11-01T08:45:40Z

What is changed and how it works?

Issue Number: Ref #12842

What's Changed:

Subsequent PR of #13689 which implements propose and execute tablet split

This PR implements on apply result as well as create/initialize split peers.
In v1, split create and register PeerFsm directly which causes some race condition problems as the 
PeerFsm can also be created by raft message.

Now in v2, the PeerFsm creation entrance has been unified which means split does not create it directly. 
Instread, it first sends an AcrossPeerMsg to the store where the store first creates an unintialized PeerFsm 
and then sends the msg to the relevant peer. It includes the information to intialize the peer.

Todo:

Implement FreezeAndClone to substitute the CreateCheckpoint API.
Only when parent receives all the responses can the apply index be updated.
Consider more when the split peers is already initialized.
Use DeleteFilesInRange and compaction filter to delete data not in the region.

Check List

Tests

Unit test
Integration test

Release note

None

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

ti-chi-bot · 2022-11-01T08:45:42Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

tabokie
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: SpadeA-Tang <u6748471@anu.edu.au>

BusyJay · 2022-11-10T08:16:38Z

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

-//! - todo
+//! On Apply Result:
+//! - on_ready_split_region: Update the relevant in memory meta info of the
+//!   parent peer, and wrap and send to the store the relevant info needed to


Suggested change

//! parent peer, and wrap and send to the store the relevant info needed to

//! parent peer, then send to the store the relevant info needed to

BusyJay · 2022-11-10T08:17:27Z

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

@@ -63,14 +72,23 @@ pub struct SplitResult {
    pub derived_index: usize,
    pub tablet_index: u64,
 }
+pub struct SplitInit {
+    /// Split region
+    pub region: metapb::Region,


Why need to be public?

It's accessed in life.rs

BusyJay · 2022-11-10T08:22:13Z

components/raftstore-v2/src/operation/life.rs

+        ER: RaftEngine,
+    {
+        let region_id = msg.as_ref().region.id;
+        let mut raft_msg = RaftMessage::default();


You can put it into Box directly.

Any difference?

Avoid copy.

BusyJay · 2022-11-10T09:01:16Z

components/raftstore-v2/src/raft/peer.rs

+            // difference is due to v2 only performs read by local reader and
+            // raftstore is just a place to renew the lease. If the lease in raftstore is
+            // not expired, it may not renew lease.
+            self.leader_lease.expire();


No, v2 should be the same as v1. expire is just used to forbid on-going read. All local reader should use the new lease as the epoch change.

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

components/raftstore-v2/src/batch/store.rs

components/raftstore-v2/src/fsm/store.rs

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

components/raftstore-v2/src/raft/peer.rs

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

BusyJay

Overall workflow LGTM

BusyJay · 2022-11-11T07:07:38Z

components/raftstore-v2/src/router/message.rs

@@ -195,6 +202,7 @@ impl fmt::Debug for StoreMsg {
    fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
        match *self {
            StoreMsg::RaftMessage(_) => write!(fmt, "Raft Message"),
+            StoreMsg::SplitInit(_) => write!(fmt, "Peer Creation"),


Wrong message.

Good catch.

BusyJay · 2022-11-11T07:07:53Z

components/raftstore-v2/src/router/message.rs

@@ -167,6 +170,9 @@ impl fmt::Debug for PeerMsg {
            },
            PeerMsg::ApplyRes(res) => write!(fmt, "ApplyRes {:?}", res),
            PeerMsg::Start => write!(fmt, "Startup"),
+            PeerMsg::SplitInit(_) => {
+                write!(fmt, "Across peer msg")


Wrong message.

Good catch.

BusyJay · 2022-11-11T07:11:45Z

components/raftstore-v2/src/operation/life.rs

+        raft_msg.set_region_epoch(msg.region.get_region_epoch().clone());
+        // Set the peer_id be INVALID_ID so that the message will not be sent to the
+        // peer after the creation.
+        raft_msg.mut_from_peer().set_id(raft::INVALID_ID);


Why not just raft_msg.take_from_peer();?

BusyJay · 2022-11-11T07:12:51Z

components/raftstore-v2/src/operation/life.rs

+        ctx.router
+            .force_send(region_id, PeerMsg::SplitInit(msg))
+            .unwrap_or_else(|e| {
+                panic!(


It's OK to return error. It's possible the peer is destroyed right before force send.

Good catch.

BusyJay · 2022-11-11T07:15:06Z

components/raftstore-v2/tests/integrations/test_split.rs

+    region: &metapb::Region,
+    key: &[u8],
+) -> RaftCmdResponse {
+    let mut req = RaftCmdRequest::default();


Why not router.new_request_for?

BusyJay · 2022-11-11T07:16:19Z

components/raftstore-v2/tests/integrations/test_split.rs

+    assert!(!resp.get_header().has_error(), "{:?}", resp);
+}
+
+fn must_put_error(store_id: u64, router: &mut TestRouter, region: &metapb::Region, key: &[u8]) {


Better implement these inline, it's hard to read to jump back and forth.

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

tabokie · 2022-11-14T05:02:48Z

components/raftstore-v2/src/raft/peer.rs

+    ///
+    /// This will update the region of the peer, caller must ensure the region
+    /// has been preserved in a durable device.
+    pub fn set_region(


Is this supposed to be put here? Seems only simple getter/setters are placed in this file.

So, which file you think is better?

I don't know really, just a thought.

components/raftstore-v2/src/raft/peer.rs

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

tabokie · 2022-11-14T06:21:01Z

components/raftstore-v2/src/raft/peer.rs

+    pub fn reset_region_buckets(&mut self) {
+        if self.region_buckets.is_some() {
+            self.last_region_buckets = self.region_buckets.take();
+            self.region_buckets = None;


unnecessary.

Good catch.

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

tabokie · 2022-11-14T06:40:28Z

/merge

ti-chi-bot · 2022-11-14T06:40:29Z

@tabokie: 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 · 2022-11-14T06:40:31Z

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

Commit hash: 2bc0a98

SpadeA-Tang and others added 27 commits October 27, 2022 13:25

init

8b49353

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

update

97f1506

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

add a test

8891983

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

refactor engine factory

070c910

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

update

9afc87c

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

update

c4bfc02

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

update

17d42b4

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

address comment:

b9a2c9e

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

address comment

dc35a2c

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

save

445a721

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

Merge branch 'split-path' into split-exec

ee0e104

save

0f0971f

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

Merge branch 'split-path' into split-exec

c8d5e63

pass test

16ecbe1

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

update

11bb7f3

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

solve conflict

01f6bcf

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

update

7558019

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

add single split

ca2d1bc

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

update

38ec916

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

update

766093f

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

update

f78c219

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

update

d884c01

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

update

ca98abb

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

fmt

f9ccbb9

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

fmt

cefa5de

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

Merge branch 'master' into split-exec

c749bd4

update

1e212d5

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

ti-chi-bot added do-not-merge/needs-linked-issue release-note-none labels Nov 1, 2022

SpadeA-Tang added 2 commits November 10, 2022 13:06

address comment

792ada3

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

address comment

0eb8f73

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

tonyxuqqi approved these changes Nov 10, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Status: PR - There is already 1 approval label Nov 10, 2022

BusyJay reviewed Nov 10, 2022

View reviewed changes

update

e97f1ad

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

tabokie reviewed Nov 11, 2022

View reviewed changes

components/raftstore-v2/src/batch/store.rs Outdated Show resolved Hide resolved

tabokie reviewed Nov 11, 2022

View reviewed changes

SpadeA-Tang added 3 commits November 11, 2022 12:45

address comment

0b4c717

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

update

ea40391

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

address comment

7e892c2

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

BusyJay reviewed Nov 11, 2022

View reviewed changes

SpadeA-Tang added 2 commits November 11, 2022 15:40

address comment

dbdf1ad

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

resolve merge

d7cde7e

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

ti-chi-bot added the needs-rebase label Nov 12, 2022

tabokie reviewed Nov 14, 2022

View reviewed changes

SpadeA-Tang added 2 commits November 14, 2022 13:29

address comment

400cf05

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

merge

7a8a31e

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

ti-chi-bot removed the needs-rebase label Nov 14, 2022

tabokie reviewed Nov 14, 2022

View reviewed changes

tabokie approved these changes Nov 14, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Status: PR - There are already 2 approvals and removed status/LGT1 Status: PR - There is already 1 approval labels Nov 14, 2022

address comment

2bc0a98

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

ti-chi-bot added the status/can-merge Status: Can merge to base branch label Nov 14, 2022

ti-chi-bot merged commit 23dba4f into tikv:master Nov 14, 2022

ti-chi-bot added this to the Pool milestone Nov 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raftstore-v2: support tablet split #13709

raftstore-v2: support tablet split #13709

SpadeA-Tang commented Nov 1, 2022 •

edited

ti-chi-bot commented Nov 1, 2022 •

edited

BusyJay Nov 10, 2022

BusyJay Nov 10, 2022

SpadeA-Tang Nov 10, 2022

BusyJay Nov 10, 2022

SpadeA-Tang Nov 10, 2022

BusyJay Nov 10, 2022

BusyJay Nov 10, 2022

BusyJay left a comment

BusyJay Nov 11, 2022

SpadeA-Tang Nov 11, 2022

BusyJay Nov 11, 2022

SpadeA-Tang Nov 11, 2022

BusyJay Nov 11, 2022

BusyJay Nov 11, 2022

SpadeA-Tang Nov 11, 2022

BusyJay Nov 11, 2022

SpadeA-Tang Nov 11, 2022

BusyJay Nov 11, 2022

SpadeA-Tang Nov 11, 2022

tabokie Nov 14, 2022

SpadeA-Tang Nov 14, 2022

tabokie Nov 14, 2022

tabokie Nov 14, 2022

SpadeA-Tang Nov 14, 2022

tabokie commented Nov 14, 2022

ti-chi-bot commented Nov 14, 2022

ti-chi-bot commented Nov 14, 2022

	//! parent peer, and wrap and send to the store the relevant info needed to
	//! parent peer, then send to the store the relevant info needed to

raftstore-v2: support tablet split #13709

raftstore-v2: support tablet split #13709

Conversation

SpadeA-Tang commented Nov 1, 2022 • edited

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Nov 1, 2022 • edited

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

BusyJay 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tabokie commented Nov 14, 2022

ti-chi-bot commented Nov 14, 2022

ti-chi-bot commented Nov 14, 2022

SpadeA-Tang commented Nov 1, 2022 •

edited

ti-chi-bot commented Nov 1, 2022 •

edited