raftstore: init half split region #2790

AndreMouche · 2018-03-05T13:33:40Z

dev
test

overvenus · 2018-03-12T03:33:12Z

The test failed.

test raftstore_cases::test_split_region::test_node_half_split_region ... key:[122, 48, 48, 48, 48, 48, 48, 48, 48, 49],current_size:74,region_size:1480
key:[122, 48, 48, 48, 48, 48, 48, 48, 48, 50],current_size:148,region_size:1480
key:[122, 48, 48, 48, 48, 48, 48, 48, 48, 51],current_size:222,region_size:1480
key:[122, 48, 48, 48, 48, 48, 48, 48, 48, 52],current_size:296,region_size:1480
key:[122, 48, 48, 48, 48, 48, 48, 48, 48, 53],current_size:370,region_size:1480
key:[122, 48, 48, 48, 48, 48, 48, 48, 48, 54],current_size:444,region_size:1480
key:[122, 48, 48, 48, 48, 48, 48, 48, 48, 55],current_size:518,region_size:1480
key:[122, 48, 48, 48, 48, 48, 48, 48, 48, 56],current_size:592,region_size:1480
key:[122, 48, 48, 48, 48, 48, 48, 48, 48, 57],current_size:666,region_size:1480
key:[122, 48, 48, 48, 48, 48, 48, 48, 49, 48],current_size:740,region_size:1480
key:[122, 48, 48, 48, 48, 48, 48, 48, 49, 49],current_size:814,region_size:1480
key:[122, 48, 48, 48, 48, 48, 48, 48, 49, 49],current_size:74,region_size:666
key:[122, 48, 48, 48, 48, 48, 48, 48, 49, 50],current_size:148,region_size:666
key:[122, 48, 48, 48, 48, 48, 48, 48, 49, 51],current_size:222,region_size:666
key:[122, 48, 48, 48, 48, 48, 48, 48, 49, 52],current_size:296,region_size:666
key:[122, 48, 48, 48, 48, 48, 48, 48, 49, 53],current_size:370,region_size:666
thread 'raftstore_cases::test_split_region::test_node_half_split_region' panicked at 'assertion failed: `(left == right)`
  left: `[48, 48, 48, 48, 48, 48, 48, 49, 53]`,
 right: `[48, 48, 48, 48, 48, 48, 48, 49, 49]`', tests/raftstore_cases/test_split_region.rs:766:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:68
             at libstd/sys_common/backtrace.rs:57
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:380
   3: std::panicking::default_hook
             at libstd/panicking.rs:396
   4: tikv::util::panic_hook::track_hook::{{closure}}
             at src/util/panic_hook.rs:59
   5: <std::thread::local::LocalKey<T>>::try_with
             at /checkout/src/libstd/thread/local.rs:377
   6: <std::thread::local::LocalKey<T>>::with
             at /checkout/src/libstd/thread/local.rs:288
   7: tikv::util::panic_hook::track_hook
             at src/util/panic_hook.rs:53
   8: core::ops::function::Fn::call
             at /checkout/src/libcore/ops/function.rs:73
   9: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:577
  10: std::panicking::begin_panic
             at libstd/panicking.rs:537
  11: std::panicking::begin_panic_fmt
             at libstd/panicking.rs:521
  12: integrations::raftstore_cases::test_split_region::test_half_split_region
             at tests/raftstore_cases/test_split_region.rs:766
  13: integrations::raftstore_cases::test_split_region::test_node_half_split_region
             at tests/raftstore_cases/test_split_region.rs:738
  14: <F as alloc::boxed::FnBox<A>>::call_box
             at libtest/lib.rs:1440
             at /checkout/src/libcore/ops/function.rs:223
             at /checkout/src/liballoc/boxed.rs:817
  15: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:102
FAILED

https://circleci.com/gh/pingcap/tikv/8580?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

huachaohuang · 2018-03-12T09:48:06Z

src/raftstore/coprocessor/split_check/half.rs

+}
+
+impl HalfStatus {
+    pub fn on_split_check(&mut self, key: &[u8], value_size: u64) -> bool {


should_split seems better.

huachaohuang · 2018-03-12T09:54:18Z

src/raftstore/store/worker/split_check.rs

@@ -123,19 +123,26 @@ impl<'a> MergedIterator<'a> {
 /// Split checking task.
 pub struct Task {
    region: Region,
+    auto_check: bool,


auto_check = false can mean a lot of things, I prefer to define an enum explicitly like:

enum SplitType { AutoSplit, HalfSplit, }

huachaohuang · 2018-03-13T03:27:20Z

src/raftstore/coprocessor/split_check/half.rs

+                others => panic!("expect split check result, but got {:?}", others),
+            }
+        }
+        drop(rx);


Seems it will be dropped automatically?

huachaohuang · 2018-03-13T03:31:25Z

src/raftstore/store/worker/split_check.rs

+        let region = &task.region;
+        let mut split_ctx =
+            self.coprocessor
+                .new_split_check_status(region, &self.engine, task.is_auto_split());


Why not just pass the split type? It seems pointless to define an enum and convert it to bool again.

We'd like to use the enum instead of bool until we need to support more split algorithms

huachaohuang · 2018-03-13T03:39:09Z

tests/raftstore_cases/test_split_region.rs

+    assert_eq!(region.get_start_key(), left.get_start_key());
+    assert_eq!(right.get_start_key(), left.get_end_key());
+    assert_eq!(region.get_end_key(), right.get_end_key());
+    assert_eq!(pd_client.get_region(&max_key).unwrap(), right);


It's not clear what you are trying to test in this two lines, why not just test the start/end keys of left and right one by one?

… of approximate size

…tikv into raftstore/half_split_region

AndreMouche · 2018-03-14T09:07:10Z

@overvenus @BusyJay @huachaohuang PTAL

siddontang · 2018-03-15T01:34:09Z

src/raftstore/coprocessor/config.rs

@@ -27,10 +27,12 @@ pub struct Config {
    /// be region_split_size (or a little bit smaller).
    pub region_max_size: ReadableSize,
    pub region_split_size: ReadableSize,
+    pub half_split_bucket_size: ReadableSize,


what is this config used for?

It's used for computing region's mid_key(split_key), this value has a direct influence on split_key

AndreMouche · 2018-03-15T09:59:30Z

We will merge master && fix conflicts after pingcap/kvproto#229 merged

AndreMouche · 2018-03-15T09:59:59Z

@BusyJay @overvenus @huachaohuang PTAL

huachaohuang · 2018-03-15T11:54:35Z

src/raftstore/store/store.rs

+                region_id,
+                region_epoch,
+            } => {
+                let peer = match self.region_peers.get(&region_id) {


Don't handle message here directly, this is a dispatch function.

huachaohuang · 2018-03-15T11:55:41Z

src/raftstore/store/store.rs

+
+                if !peer.is_leader() {
+                    // region on this store is no longer leader, skipped.
+                    info!(


s/info/warn

huachaohuang · 2018-03-15T11:55:52Z

src/raftstore/store/store.rs

+
+                let region = peer.region();
+                if util::is_epoch_stale(&region_epoch, region.get_region_epoch()) {
+                    info!("[region {}] receive a stale halfsplit message", region_id);


s/info/warn

huachaohuang · 2018-03-15T12:03:33Z

tests/raftstore_cases/test_split_region.rs

+}
+
+fn test_half_split_region<T: Simulator>(cluster: &mut Cluster<T>) {
+    let item_len = 74;


Please add a comment for this magic number.

huachaohuang · 2018-03-15T12:04:26Z

src/raftstore/coprocessor/split_check/mod.rs

+        let mut status = Status::default();
+        status.auto_split = auto_split;
+        status
+    }


Add a new line below.

huachaohuang · 2018-03-15T12:04:43Z

src/raftstore/coprocessor/split_check/mod.rs

 #[derive(Default)]
 pub struct Status {
    // For TableCheckObserver
    table: Option<TableStatus>,
    // For SizeCheckObserver
    size: Option<SizeStatus>,
+    // For HalfCheckObserver TODO:


What is the TODO?

huachaohuang · 2018-03-15T12:23:50Z

src/raftstore/coprocessor/split_check/half.rs

+        self.cur_bucket_size += current_len;
+    }
+
+    fn check_and_adjust_buckets_num(&mut self) {


It takes me a while to figure out what does this do, maybe a simpler way is to increase the bucket size exponentially.

huachaohuang · 2018-03-15T12:24:25Z

src/raftstore/coprocessor/split_check/half.rs

+
+    pub fn split_key(self) -> Option<Vec<u8>> {
+        let mid = self.buckets.len() / 2;
+        if mid == 0 {


buckets.get() will take care of this.

It's used to check if self.buckets.len() <= 1 here

AndreMouche · 2018-03-18T09:41:35Z

PTAL

AndreMouche · 2018-03-21T01:35:58Z

friendly ping @overvenus @BusyJay @huachaohuang

overvenus · 2018-03-21T02:43:25Z

src/raftstore/coprocessor/split_check/half.rs

+        if mid == 0 {
+            None
+        } else {
+            self.buckets.get(mid).cloned()


How about remove? So we don't need the cloned()

It seems to cost more since remove will shift all elements after it to the left?

How about swap_remove ?

overvenus · 2018-03-21T02:49:47Z

src/raftstore/store/worker/split_check.rs

+}
+
+#[derive(Debug)]
+pub enum SplitType {


How about making the Task an enum?

pub enum Task { Auto(Region), Manual(Region), }

I don't think so since all tasks will share the same Region struct.

Then we only need a bool.

overvenus · 2018-03-21T03:03:18Z

src/raftstore/coprocessor/config.rs

@@ -31,6 +31,7 @@ pub struct Config {

 /// Default region split size.
 pub const SPLIT_SIZE_MB: u64 = 96;
+pub const HALF_SPLIT_BUCKET_SIZE_MB: u64 = 1;


Looks like an unused const.

overvenus · 2018-03-21T03:23:45Z

tests/raftstore/pd.rs

@@ -522,6 +522,24 @@ impl TestPdClient {
        });
    }

+    pub fn half_split_region(&self, src_region: metapb::Region) {
+        self.set_rule(box move |region: &metapb::Region, _: &metapb::Peer| {


Please fix merge conflicts.

It's blocked by pingcap/kvproto#229 @disksing

Why is this conflict blocked by kvproto?

huachaohuang · 2018-03-21T07:55:52Z

tests/raftstore/pd.rs

@@ -522,6 +522,24 @@ impl TestPdClient {
        });
    }

+    pub fn half_split_region(&self, src_region: metapb::Region) {
+        self.set_rule(box move |region: &metapb::Region, _: &metapb::Peer| {


Why is this conflict blocked by kvproto?

huachaohuang · 2018-03-21T09:37:31Z

src/raftstore/coprocessor/split_check/half.rs

+        if half_split_bucket_size == 0 {
+            half_split_bucket_size = 1;
+        } else if half_split_bucket_size > bucket_size_limit {
+            half_split_bucket_size = bucket_size_limit;


It doesn't seem possible that the bucket size will exceed 512MB here.

yes. we do check here in case someday the region's max size increase too much.

AndreMouche · 2018-03-23T02:21:16Z

@overvenus @BusyJay @huachaohuang PTAL

overvenus · 2018-03-23T03:54:40Z

src/raftstore/coprocessor/split_check/half.rs

+impl SplitCheckObserver for HalfCheckObserver {
+    fn new_split_check_status(
+        &self,
+        _ctx: &mut ObserverContext,


I think we can get_region_approximate_size and then we can find a split key faster.

In our last test, get_region_approximate_size may not be as accurate as expected.

siddontang · 2018-03-23T08:28:01Z

src/raftstore/coprocessor/split_check/half.rs

+        if mid == 0 {
+            None
+        } else {
+            Some(self.buckets.swap_remove(mid))


why use swap_remove here?

avoid get(mid).clone()

siddontang · 2018-03-23T08:30:04Z

src/raftstore/coprocessor/split_check/half.rs

+        let mut half_split_bucket_size = region_size_limit / BUCKET_NUMBER_LIMIT as u64;
+        let bucket_size_limit = ReadableSize::mb(BUCKET_SIZE_LIMIT_MB).0;
+        if half_split_bucket_size == 0 {
+            half_split_bucket_size = 1;


bucket size is 1 byte here? too small?

I think it is mainly for tests.

It only happens when max_region_size is smaller than 1K

siddontang · 2018-03-23T09:04:47Z

can you unify auto_split, auto_check? are they different?

huachaohuang · 2018-03-23T08:48:47Z

src/raftstore/coprocessor/split_check/half.rs

+            self.buckets.push(key.to_vec());
+            self.cur_bucket_size = 0;
+        }
+        self.cur_bucket_size += current_len;


current_len is a bit confused here, I think you can just add the length of the key and value here.

huachaohuang · 2018-03-23T08:52:07Z

src/raftstore/coprocessor/split_check/half.rs

+        let mut half_split_bucket_size = region_size_limit / BUCKET_NUMBER_LIMIT as u64;
+        let bucket_size_limit = ReadableSize::mb(BUCKET_SIZE_LIMIT_MB).0;
+        if half_split_bucket_size == 0 {
+            half_split_bucket_size = 1;


I think it is mainly for tests.

huachaohuang · 2018-03-23T08:56:19Z

src/raftstore/coprocessor/split_check/half.rs

+        }
+        // Approximate size of memtable is inaccurate for small data,
+        // we flush it to SST so we can use the size properties instead.
+        engine.flush(true).unwrap();


But you don't use approximate size now.

huachaohuang · 2018-03-23T09:03:12Z

tests/raftstore/pd.rs

 }

 impl Operator {
    fn make_region_heartbeat_response(
        &self,
-        region_id: u64,
+        region: &metapb::Region,


Why do you need to change this?

huachaohuang · 2018-03-23T09:03:46Z

tests/raftstore/pd.rs

@@ -165,6 +170,9 @@ impl Operator {
                }
                unreachable!()
            }
+            Operator::HalfSplitRegion { ref region_epoch } => {
+                region.get_region_epoch() != region_epoch


What if it is a stale epoch?

do not send split command @huachaohuang

huachaohuang · 2018-03-23T09:05:06Z

src/raftstore/store/worker/split_check.rs

@@ -123,19 +123,26 @@ impl<'a> MergedIterator<'a> {
 /// Split checking task.
 pub struct Task {
    region: Region,
+    auto: bool,


auto_split is more clear.

overvenus

LGTM

huachaohuang

LGTM

AndreMouche added 4 commits March 5, 2018 21:32

raftstore: init half split region

0b30368

Merge branch 'master' into raftstore/half_split_region

11c84f6

split_check/half: init test_split_check for HalfCheckObserver

9c38e87

tests: init test half split region with heartbeat

7d1f6a3

AndreMouche changed the title ~~[WIP] raftstore: init half split region~~ raftstore: init half split region Mar 11, 2018

split_check/half: init debuginfo for test

0343338

AndreMouche added 2 commits March 12, 2018 11:54

store/store.rs: check epoch before half split

9f94ec2

store/store.rs: check whether epoch is stale before half split

5890743

huachaohuang reviewed Mar 12, 2018

View reviewed changes

merge master && address comments

75c17c5

huachaohuang reviewed Mar 13, 2018

View reviewed changes

AndreMouche added 5 commits March 13, 2018 19:33

coprocessor/split_check: iterator region to compute split key instead…

7175f4f

… of approximate size

Merge branch 'master' into raftstore/half_split_region

dd6bd81

Merge branch 'master' into raftstore/half_split_region

4bad506

split_check/half: process case when bucket's number meets upper limit

bfd6378

Merge branch 'raftstore/half_split_region' of github.com:AndreMouche/…

a0186c6

…tikv into raftstore/half_split_region

siddontang reviewed Mar 15, 2018

View reviewed changes

huachaohuang reviewed Mar 15, 2018

View reviewed changes

AndreMouche added 2 commits March 16, 2018 20:09

address comments

b2aab91

address comments

46c3fcb

overvenus reviewed Mar 21, 2018

View reviewed changes

address comments

793eebe

huachaohuang reviewed Mar 21, 2018

View reviewed changes

merge master && fix conflicts

9b6e8f8

AndreMouche added 5 commits March 22, 2018 17:58

Merge branch 'master' into raftstore/half_split_region

5b8875b

address comments

4643d14

merge master

d4f03d1

merge master && fix conflicts

1881404

address comments

5d53579

overvenus reviewed Mar 23, 2018

View reviewed changes

Merge branch 'master' into raftstore/half_split_region

0134382

siddontang reviewed Mar 23, 2018

View reviewed changes

huachaohuang reviewed Mar 23, 2018

View reviewed changes

AndreMouche added 2 commits March 26, 2018 10:25

address comments

392a1b5

Merge branch 'master' into raftstore/half_split_region

78326ca

overvenus reviewed Mar 26, 2018

View reviewed changes

huachaohuang reviewed Mar 26, 2018

View reviewed changes

overvenus previously approved these changes Mar 26, 2018

View reviewed changes

update kvproto

b6d1b0f

AndreMouche dismissed overvenus’s stale review via b6d1b0f March 26, 2018 10:11

Merge branch 'master' into raftstore/half_split_region

efb19ff

overvenus approved these changes Mar 27, 2018

View reviewed changes

overvenus merged commit 6784cba into tikv:master Mar 27, 2018

sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019

raftstore: init half split region (tikv#2790)

71a83cc

raftstore: init half split region #2790

raftstore: init half split region #2790

Conversation

AndreMouche commented Mar 5, 2018 • edited

overvenus commented Mar 12, 2018

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

AndreMouche commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreMouche commented Mar 15, 2018

AndreMouche commented Mar 15, 2018

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

AndreMouche commented Mar 18, 2018

AndreMouche commented Mar 21, 2018

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

AndreMouche commented Mar 23, 2018

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

siddontang commented Mar 23, 2018

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

overvenus left a comment

Choose a reason for hiding this comment

huachaohuang left a comment

Choose a reason for hiding this comment

AndreMouche commented Mar 5, 2018 •

edited