support split v2 #1747

zhangjinpeng87 · 2017-04-08T03:52:47Z

siddontang · 2017-04-08T09:01:38Z

src/raftstore/store/store.rs

-        for peer in right.get_peers() {
+                             right: metapb::Region,
+                             left_derive: bool) {
+        let new_region = if left_derive {


let (new_region, origin_region) = if left_derive { (right.clone(), left.clone()) } else { (left.clone(), right.clone()) }

siddontang · 2017-04-08T09:03:50Z

seem that we should also handle https://github.com/pingcap/tikv/blob/614b7e083c4cee7e27d960436c5207cd6908e265/src/raftstore/store/store.rs#L1274 carefully

/cc @BusyJay @disksing

zhangjinpeng87 · 2017-04-10T03:16:00Z

src/raftstore/store/store.rs

-                if let Some(left) = self.region_peers.get(&region_id) {
-                    campaigned = new_peer.maybe_campaign(left, &mut self.pending_raft_groups);
+                if let Some(origin_peer) = self.region_peers.get(&region_id) {
+                    campaigned =


@BusyJay Can we remove this mechanism after we implement split v2?

The left region may still need to campaign fastly.

zhangjinpeng87 · 2017-04-10T03:21:02Z

@siddontang Yes, so we modify Excluded(enc_end_key(parent_region)) to Included(enc_start_key(parent_region)) to find the two new regions and add them to new_regions.

siddontang · 2017-04-11T02:29:46Z

src/raftstore/store/store.rs

            // received by the TiKV driver is newer than the meta cached in the driver, the meta is
            // updated.
            if let Some((_, &next_region_id)) = self.region_ranges
-                .range((Excluded(enc_end_key(peer.region())), Unbounded::<Key>))
+                .range((Included(enc_start_key(peer.region())), Unbounded::<Key>))


do we have any test to cover the tow cases for 1 -> 1 + 2 and 1 -> 2 + 1?

siddontang · 2017-04-13T08:35:18Z

any update @zhangjinpeng1987

siddontang · 2017-04-14T01:20:54Z

src/raftstore/store/peer.rs

@@ -1337,13 +1337,17 @@ impl Peer {

 pub fn check_epoch(region: &metapb::Region, req: &RaftCmdRequest) -> Result<()> {
    let (mut check_ver, mut check_conf_ver) = (false, false);
+    let mut left_derive = false;


I guess this can only check Admin Split command, for other commands, we also can meet stale problems.

Please refresh change code.

siddontang · 2017-04-14T05:05:50Z

src/raftstore/store/config.rs

@@ -130,6 +130,9 @@ pub struct Config {
    pub raft_store_max_leader_lease: TimeDuration,

    pub use_sst_file_snapshot: bool,
+
+    // Left region derive origin region id when split.
+    pub left_derive_when_split: bool,


is it better to name right_derive here because the request uses right_derive.

BusyJay · 2017-04-14T05:15:25Z

src/raftstore/store/worker/split_check.rs

@@ -147,19 +147,22 @@ pub struct Runner<C> {
    ch: RetryableSendCh<Msg, C>,
    region_max_size: u64,
    split_size: u64,
+    left_derive: bool,


I don't think it's necessary here.

BusyJay · 2017-04-14T05:17:07Z

src/raftstore/coprocessor/split_observer.rs

@@ -133,6 +133,7 @@ mod test {
        req.set_cmd_type(AdminCmdType::Split);
        let mut split_req = SplitRequest::new();
        split_req.set_split_key(key.to_vec());
+        split_req.set_right_derive(true);


Unnecessary.

zhangjinpeng87 · 2017-04-15T01:06:39Z

PTAL

BusyJay · 2017-04-17T09:03:05Z

Please fix CI.

siddontang · 2017-04-18T01:25:52Z

src/raftstore/store/store.rs

            // received by the TiKV driver is newer than the meta cached in the driver, the meta is
            // updated.
-            if let Some((_, &next_region_id)) = self.region_ranges
+            let new_region_id = if self.cfg.right_derive_when_split {


can we extract the logic and check individually? I am concerned the correctness here.

test_split_stale_epoch do that thing exactly.

siddontang · 2017-04-19T11:17:01Z

src/raftstore/store/store.rs

            }
            return Err(Error::StaleEpoch(msg, new_regions));
        }
        res
    }

+    pub fn find_sibling_region(&self, region: &metapb::Region) -> Option<u64> {
+        if self.cfg.right_derive_when_split {


/cc @disksing

Please review the logic here.

let start = if self.cfg.right_derive_when_split { Included(enc_start_key(region) } else { Excluded(enc_end_key(region) }; self.region_ranges.range(start, Unbounded::<Key>).next().map(|(_, &region_id)| region_id)

any update?

siddontang · 2017-04-20T06:11:35Z

Rest LGTM

PTAL @BusyJay @disksing

zhangjinpeng87 · 2017-04-21T06:15:31Z

@disksing PTAL

disksing · 2017-04-21T09:35:35Z

LGTM.

siddontang · 2017-04-22T06:35:10Z

src/raftstore/store/store.rs

+                // To prevent from big region, the right region need run split
+                // check again after split.
+                if right_derive {
+                    if let Some(peer) = self.region_peers.get_mut(&region_id) {


seem must exist here?

siddontang · 2017-04-22T06:42:00Z

Rest LGTM
PTAL @hhkbp2

hhkbp2 · 2017-04-24T03:06:09Z

src/raftstore/store/store.rs

+                if right_derive {
+                    self.region_peers.get_mut(&region_id).unwrap().size_diff_hint = self.cfg
+                        .region_check_size_diff;
+


Remove empty line.

hhkbp2 · 2017-04-24T03:16:59Z

src/raftstore/store/store.rs

+        } else {
+            Excluded(enc_end_key(region))
+        };
+        self.region_ranges.range((start, Unbounded::<Key>)).next().map(|(_, &region_id)| region_id)


Should it be range(Unbound::<key>, start).rev() for right_derive_when_split == true?

hhkbp2 · 2017-04-24T03:30:18Z

src/raftstore/store/config.rs

@@ -130,6 +130,9 @@ pub struct Config {
    pub raft_store_max_leader_lease: TimeDuration,

    pub use_sst_file_snapshot: bool,
+
+    // Right region derive origin region id when split.
+    pub right_derive_when_split: bool,


s/right_derive_when_split/derive_right_part_when_split/

right_derive_when_split is ok

hhkbp2 · 2017-04-24T06:55:06Z

Rest LGTM

siddontang · 2017-04-24T09:00:39Z

Any update? @zhangjinpeng1987

…zhangjinpeng/split-v2

Signed-off-by: Ping Yu <[email protected]>

zhangjinpeng87 added 3 commits April 7, 2017 19:24

support split v2

acd5780

fix test

4810f52

use flag instead of SplitV2

614b7e0

siddontang reviewed Apr 8, 2017

View reviewed changes

zhangjinpeng87 added 2 commits April 8, 2017 18:15

address comments

dcfab29

Merge branch 'master' into zhangjinpeng/split-v2

05005fc

zhangjinpeng87 commented Apr 10, 2017

View reviewed changes

merge master

72de920

siddontang reviewed Apr 11, 2017

View reviewed changes

zhangjinpeng87 added 4 commits April 13, 2017 16:57

Merge branch 'master' into zhangjinpeng/split-v2

92b51e2

add left_derive for stale-epoch-error type

9dd9bef

add test for left derive and right derive when split

6b92806

remove duplicated code

66695cc

siddontang reviewed Apr 14, 2017

View reviewed changes

merge master

32664ce

siddontang reviewed Apr 14, 2017

View reviewed changes

BusyJay reviewed Apr 14, 2017

View reviewed changes

zhangjinpeng87 added 4 commits April 14, 2017 21:10

Merge branch 'master' into zhangjinpeng/split-v2

17e582a

address comments

ff491f8

Merge branch 'master' into zhangjinpeng/split-v2

1835ed9

update kvproto

67a0b0d

siddontang reviewed Apr 18, 2017

View reviewed changes

zhangjinpeng87 added 3 commits April 18, 2017 17:54

merge master

c40d333

fix ci

648c3b6

address comments

9f28b2e

siddontang reviewed Apr 19, 2017

View reviewed changes

address comments

7cf2875

siddontang reviewed Apr 22, 2017

View reviewed changes

siddontang approved these changes Apr 22, 2017

View reviewed changes

zhangjinpeng87 added 2 commits April 23, 2017 08:51

address comments

914593d

Merge branch 'master' into zhangjinpeng/split-v2

8565df4

hhkbp2 reviewed Apr 24, 2017

View reviewed changes

zhangjinpeng87 added 2 commits April 24, 2017 17:12

address comments

26f4bec

Merge branch 'zhangjinpeng/split-v2' of github.com:pingcap/tikv into …

a965f36

…zhangjinpeng/split-v2

zhangjinpeng87 merged commit a273d36 into master Apr 24, 2017

zhangjinpeng87 deleted the zhangjinpeng/split-v2 branch April 24, 2017 09:53

iosmanthus pushed a commit to iosmanthus/tikv that referenced this pull request Oct 17, 2024

random-test: Improve stability of check cluster (tikv#1747)

ab817b8

Signed-off-by: Ping Yu <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support split v2 #1747

support split v2 #1747

zhangjinpeng87 commented Apr 8, 2017

siddontang Apr 8, 2017

siddontang commented Apr 8, 2017

zhangjinpeng87 Apr 10, 2017

siddontang Apr 10, 2017 •

edited

Loading

zhangjinpeng87 commented Apr 10, 2017

siddontang Apr 11, 2017

siddontang commented Apr 13, 2017

siddontang Apr 14, 2017

zhangjinpeng87 Apr 14, 2017

siddontang Apr 14, 2017

BusyJay Apr 14, 2017

BusyJay Apr 14, 2017

zhangjinpeng87 commented Apr 15, 2017

BusyJay commented Apr 17, 2017

siddontang Apr 18, 2017

zhangjinpeng87 Apr 19, 2017

siddontang Apr 19, 2017

disksing Apr 19, 2017

siddontang Apr 20, 2017

siddontang commented Apr 20, 2017

zhangjinpeng87 commented Apr 21, 2017

disksing commented Apr 21, 2017

siddontang Apr 22, 2017

siddontang commented Apr 22, 2017

hhkbp2 Apr 24, 2017

hhkbp2 Apr 24, 2017

zhangjinpeng87 Apr 24, 2017

hhkbp2 Apr 24, 2017

zhangjinpeng87 Apr 24, 2017

hhkbp2 commented Apr 24, 2017

siddontang commented Apr 24, 2017

support split v2 #1747

support split v2 #1747

Conversation

zhangjinpeng87 commented Apr 8, 2017

Choose a reason for hiding this comment

siddontang commented Apr 8, 2017

Choose a reason for hiding this comment

siddontang Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

zhangjinpeng87 commented Apr 10, 2017

Choose a reason for hiding this comment

siddontang commented Apr 13, 2017

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

zhangjinpeng87 commented Apr 15, 2017

BusyJay commented Apr 17, 2017

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

siddontang commented Apr 20, 2017

zhangjinpeng87 commented Apr 21, 2017

disksing commented Apr 21, 2017

Choose a reason for hiding this comment

siddontang commented Apr 22, 2017

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

hhkbp2 commented Apr 24, 2017

siddontang commented Apr 24, 2017

siddontang Apr 10, 2017 •

edited

Loading