tikv-ctl: set a region to tombstone. #2394

hicqu · 2017-10-18T12:56:39Z

No description provided.

siddontang · 2017-10-18T14:19:32Z

src/raftstore/store/debug.rs

+        epoch: RegionEpoch,
+        peers: RepeatedField<Peer>,
+    ) -> Result<()> {
+        let db = &self.engines.kv_engine;


we must guarantee that the peers can't contain the local peer now.

siddontang · 2017-10-18T14:20:30Z

src/raftstore/store/debug.rs

@@ -219,6 +220,31 @@ impl Debugger {
        compact_range(db, handle, start, end, false);
        Ok(())
    }
+
+    /// Set a region to tombstone by manual.
+    pub fn set_region_tombstone(


why not using a meta::Region directly as the argument.

How about the region info in PD is updated? For example, if the region split it will have a new [start_key, end_key).

It's also weird the range doesn't match the epoch.

hicqu · 2017-10-19T06:11:04Z

@BusyJay PTAL, thanks.

BusyJay · 2017-10-19T06:17:07Z

src/bin/tikv-ctl.rs

+        {
+            Some(mut meta_region) => {
+                let epoch = meta_region.take_region_epoch();
+                let peers = meta_region.take_peers();


Should check if peers contains target peer.

This logic is in Debugger::set_region_tombstone, because here we can't get self store_id.

we can get store id with store_ident key

see https://github.com/pingcap/tikv/blob/master/src/raftstore/store/keys.rs#L37

Yes, we do this in Debugger::set_region_tombstone.

siddontang · 2017-10-19T14:07:46Z

src/raftstore/store/debug.rs

+            Some(mut region_state) => {
+                let conf_ver = region.get_region_epoch().get_conf_ver();
+                if conf_ver == region_state.get_region().get_region_epoch().get_conf_ver() {
+                    return Err(box_err!("The conf_ver hasn't changed."));


can we meet this case?

remove-peer in pd-ctl is asynchonrous so that we may run tikv-ctl tombstone command before the config really change. So I think add this check is better.

If this is happening then it can't pass the check at L231.

BusyJay · 2017-10-20T05:55:18Z

src/bin/tikv-ctl.rs

@@ -323,6 +324,29 @@ trait DebugExecutor {
        self.do_compact(db, cf, from, to);
    }

+    fn set_region_tombstone(&self, region: u64, endpoints: Vec<String>) {


region_id is better.

BusyJay · 2017-10-20T05:57:13Z

src/bin/tikv-ctl.rs

@@ -323,6 +324,29 @@ trait DebugExecutor {
        self.do_compact(db, cf, from, to);
    }

+    fn set_region_tombstone(&self, region: u64, endpoints: Vec<String>) {
+        let debugger = self.get_local_debugger().unwrap_or_else(|| {


Why not add set_region_tombstone to the trait?

BusyJay · 2017-10-20T05:59:23Z

src/bin/tikv-ctl.rs

+                        .use_delimiter(true)
+                        .require_delimiter(true)
+                        .value_delimiter(",")
+                        .help("the pd url"),


I don't think it's a url.

BusyJay · 2017-10-20T06:04:40Z

src/raftstore/store/debug.rs

+            .iter()
+            .any(|peer| peer.get_store_id() == store_id)
+        {
+            return Err(box_err!("The peer is still in new peers list"));


What if there is a new replica (same store id but different region id) on this store?

We compare this peer's store_id with peers having the specified region. So I think if remove-peer really finished, we can got false here, which is expected.

BusyJay · 2017-10-20T06:05:45Z

src/raftstore/store/debug.rs

+            Some(mut region_state) => {
+                let conf_ver = region.get_region_epoch().get_conf_ver();
+                if conf_ver == region_state.get_region().get_region_epoch().get_conf_ver() {
+                    return Err(box_err!("The conf_ver hasn't changed."));


If this is happening then it can't pass the check at L231.

BusyJay · 2017-10-20T06:08:10Z

src/raftstore/store/debug.rs

+                if conf_ver == region_state.get_region().get_region_epoch().get_conf_ver() {
+                    return Err(box_err!("The conf_ver hasn't changed."));
+                }
+                region_state.set_state(PeerState::Tombstone);


Can you reuse write_peer_state instead?

hicqu · 2017-10-20T06:42:53Z

@BusyJay , PTAL again, thanks.

hicqu · 2017-10-20T11:50:56Z

@BusyJay PTAL again thanks.

BusyJay

rest LGTM

BusyJay · 2017-10-20T12:09:38Z

src/raftstore/store/debug.rs

+
+        if new_conf_ver > old_conf_ver && scheduled {
+            let wb = WriteBatch::new();
+            box_try!(write_peer_state(db, &wb, &region, PeerState::Tombstone));


Use old_region just as GC message does.

siddontang

LGTM

hicqu · 2017-10-22T14:12:53Z

/run-all-test

tikv-ctl: set a region to tombstone.

4a384e5

hicqu requested review from BusyJay, siddontang and overvenus October 18, 2017 12:56

siddontang reviewed Oct 18, 2017

View reviewed changes

hicqu changed the title ~~tikv-ctl: set a region to tombstone.~~ jobstikv-ctl: set a region to tombstone. Oct 19, 2017

address comments.

5f96194

BusyJay reviewed Oct 19, 2017

View reviewed changes

hicqu changed the title ~~jobstikv-ctl: set a region to tombstone.~~ tikv-ctl: set a region to tombstone. Oct 19, 2017

hicqu added 2 commits October 19, 2017 15:38

address comments.

0b32861

address comments.

ad30e20

hicqu mentioned this pull request Oct 19, 2017

tikv-ctl: add command log-behind. #2395

Closed

Merge branch 'master' into qupeng/debug-tombstone

4792437

siddontang reviewed Oct 19, 2017

View reviewed changes

BusyJay reviewed Oct 20, 2017

View reviewed changes

address comments.

8e8dd8d

hicqu added 3 commits October 20, 2017 15:29

fix CI.

b6d6dec

address comments.

5749c0c

address comments.

67bad50

BusyJay reviewed Oct 20, 2017

View reviewed changes

siddontang approved these changes Oct 20, 2017

View reviewed changes

hicqu added 2 commits October 22, 2017 22:01

Merge branch 'master' into qupeng/debug-tombstone

614e78d

address comments.

36666c0

hicqu merged commit 7763bca into master Oct 22, 2017

hicqu deleted the qupeng/debug-tombstone branch October 22, 2017 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tikv-ctl: set a region to tombstone. #2394

tikv-ctl: set a region to tombstone. #2394

hicqu commented Oct 18, 2017

siddontang Oct 18, 2017

siddontang Oct 18, 2017

hicqu Oct 19, 2017

BusyJay Oct 19, 2017

hicqu commented Oct 19, 2017

BusyJay Oct 19, 2017

hicqu Oct 19, 2017

siddontang Oct 19, 2017

siddontang Oct 19, 2017

hicqu Oct 19, 2017

siddontang Oct 19, 2017

hicqu Oct 20, 2017

BusyJay Oct 20, 2017

BusyJay Oct 20, 2017

BusyJay Oct 20, 2017

hicqu Oct 20, 2017

BusyJay Oct 20, 2017

BusyJay Oct 20, 2017

hicqu Oct 20, 2017

BusyJay Oct 20, 2017

BusyJay Oct 20, 2017

hicqu commented Oct 20, 2017

hicqu commented Oct 20, 2017

BusyJay left a comment

BusyJay Oct 20, 2017

siddontang left a comment

hicqu commented Oct 22, 2017

tikv-ctl: set a region to tombstone. #2394

tikv-ctl: set a region to tombstone. #2394

Conversation

hicqu commented Oct 18, 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

hicqu commented Oct 19, 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

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

hicqu commented Oct 20, 2017

hicqu commented Oct 20, 2017

BusyJay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddontang left a comment

Choose a reason for hiding this comment

hicqu commented Oct 22, 2017