tikv-ctl: refactor with debug API. #2377

hicqu · 2017-10-12T05:25:31Z

Now tikv-ctl uses debug API so that it can work in both remote mode and local mode.

BusyJay · 2017-10-12T05:30:50Z

src/raftstore/store/debug.rs

@@ -53,6 +55,13 @@ quick_error!{
    }
 }

+#[derive(PartialEq, Debug)]
+pub struct RegionInfo(


Use struct instead of tuple struct.

BusyJay · 2017-10-12T05:36:47Z

src/raftstore/store/debug.rs

@@ -186,6 +209,38 @@ impl Debugger {
        compact_range(db, handle, start, end, false);
        Ok(())
    }
+
+    /// Set a region to tombstone by manual.
+    pub fn tombstone_region(&self, region: u64, conf_ver: u64) -> Result<()> {


Mark a region as tombstone doesn't (and shouldn't) require a running TiKV instance, otherwise the updated region will probably be overwritten by the running instance.

Yes, so there is no tombstone_region API in server/service/debug.rs, and tombstone command in tikv-ctl.rs doesn't support remote mode either. I just put the code here. If it's confuse, which place to place this function is better?

The function name confused me, which looks like a get function, but here is a set function, a bad name.

Maybe we should get the current region epoch from PD and then set the tombstone with it.

can you use another PR to support this feature?

This PR is only for refactor.

Got it. I will raise this in next PR, pass a PDClient into this function, and move this code to tikv-ctl.rs.

BusyJay · 2017-10-12T05:39:36Z

src/raftstore/store/debug.rs

@@ -63,6 +72,27 @@ impl Debugger {
        Debugger { engines }
    }

+    pub fn get_all_regions(&self, db: DBType, cf: &str) -> Result<Vec<u64>> {


In what case will db be DBType::Raft?

BusyJay · 2017-10-12T05:41:30Z

src/bin/tikv-ctl.rs

+                let raft_db = if let Some(raftdb_path) = matches.value_of("raftdb") {
+                    Arc::new(util::rocksdb::open(raftdb_path, &[CF_DEFAULT]).unwrap())
+                } else {
+                    let raftdb_path = path.to_string() + "../raft";


Use Path::join instead.

…peng/debug-client

hicqu · 2017-10-12T09:43:46Z

@BusyJay @overvenus , PTAL again, thanks.

siddontang · 2017-10-12T11:47:15Z

src/bin/tikv-ctl.rs

+use tikv::raftstore::store::debug::{Debugger, RegionInfo};
+use tikv::storage::{ALL_CFS, CF_DEFAULT, CF_LOCK, CF_RAFT, CF_WRITE};
+
+enum DebugExecutor {


is it better to define an Executor trait so we can avoid following match in many functions?

BusyJay · 2017-10-13T05:30:27Z

src/raftstore/store/debug.rs

@@ -63,6 +85,23 @@ impl Debugger {
        Debugger { engines }
    }

+    /// Get all region id in the specified CF, only used for KV rocksdb.
+    pub fn get_all_regions(&self, cf: &str) -> Result<Vec<u64>> {


In what case cf is not CF_RAFT?

For now there is not because we only have get_region_info using it. However, I think maybe we will need to get regions from other CFs in future. So keep the CF as an argument is better?

BusyJay · 2017-10-13T05:37:16Z

src/bin/tikv-ctl.rs

-            None => dump_all_region_size(&db, cf_name),
+        let cfs = matches
+            .value_of("cf")
+            .map_or(ALL_CFS.to_vec(), |cf| vec![cf]);


map_or_else

BusyJay · 2017-10-13T05:39:03Z

src/bin/tikv-ctl.rs

+        println!("value: {}", value);
+    }
+
+    fn region_size(&self, region_id: u64, cfs: Vec<&str>) {


Weird name. It feels like it's going to return the region size. Maybe verb+noun is a better choice.

BusyJay · 2017-10-13T05:40:13Z

src/bin/tikv-ctl.rs

+
+    fn region_info(&self, region_id: u64, skip_tombstone: bool) {
+        let region_info = self.get_region_info(region_id);
+        Self::show_region_info(region_id, region_info, skip_tombstone);


Why do it in a separate function?

siddontang · 2017-10-13T09:31:17Z

PTAL @overvenus @BusyJay

I use some long options, for avoid name conflict.

…peng/debug-client

hicqu · 2017-10-13T10:05:54Z

@BusyJay @overvenus , PTAL thanks.

BusyJay · 2017-10-13T10:52:21Z

src/bin/tikv-ctl.rs

            Arg::with_name("hex-to-escaped")
-                .short("h")
+                .long("from-hex")


I think "to-escaped" is more appropriate.

BusyJay · 2017-10-13T10:52:55Z

src/bin/tikv-ctl.rs

@@ -496,39 +503,44 @@ fn main() {
                .about("print the range db range")
                .arg(
                    Arg::with_name("from")
-                        .short("f")
+                        .long("from")


Please also keep the short flag.

BusyJay · 2017-10-13T10:55:15Z

src/bin/tikv-ctl.rs

+    host: Option<&str>,
+) -> Box<DebugExecutor> {
+    match (host, db) {
+        (Some(_), Some(_)) => {


I think L67~L74 is unreachable.

BusyJay · 2017-10-13T10:56:22Z

src/bin/tikv-ctl.rs

+}
+
+trait DebugExecutor {
+    fn dump_key_value(&self, cf: &str, key: Vec<u8>) {


dump_value is more appropriate for key is not printed out.

BusyJay · 2017-10-13T10:57:12Z

src/bin/tikv-ctl.rs

+        let sizes = self.get_region_size(region, cfs);
+        if sizes.len() > 1 {
+            let total_size = sizes.iter().map(|t| t.1).sum::<usize>() as u64;
+            println!("total region number: {}", sizes.len());


What does it mean by "region number"?

BusyJay · 2017-10-13T11:00:27Z

src/bin/tikv-ctl.rs

+
+    fn dump_all_region_info(&self, skip_tombstone: bool) {
+        for region in self.get_all_meta_regions() {
+            self.dump_region_info(region, skip_tombstone);


I think you need to print region id before display its information.

hicqu · 2017-10-16T03:47:18Z

@BusyJay @overvenus PTAL, thanks.

overvenus · 2017-10-16T10:57:21Z

src/bin/tikv-ctl.rs

+use tikv::raftstore::store::debug::{Debugger, RegionInfo};
+use tikv::storage::{ALL_CFS, CF_DEFAULT, CF_LOCK, CF_WRITE};
+
+fn perror_and_exit<E: Error, T>(e: E) -> T {


Why not -> !?

So unwrap_or_else can't accept it as argument.

overvenus

LGTM

BusyJay · 2017-10-16T04:28:46Z

src/bin/tikv-ctl.rs

+use tikv::storage::{ALL_CFS, CF_DEFAULT, CF_LOCK, CF_WRITE};
+
+fn perror_and_exit<E: Error, T>(e: E) -> T {
+    eprintln!("{}", e);


The error may be confusing because it lacks context information.

BusyJay · 2017-10-17T11:45:34Z

src/bin/tikv-ctl.rs

+                        if start_ts.map_or(true, |ts| write_info.get_start_ts() == ts) &&
+                            commit_ts.map_or(true, |ts| write_info.get_commit_ts() == ts)
+                        {
+                            // FIXME: short_value is lost in kvproto.


Why is it a problem?

The pb WriteInfo has not short_value field, so we can't show that.

BusyJay · 2017-10-17T11:47:16Z

src/bin/tikv-ctl.rs

+                    let mut lock_info = mvcc.take_lock();
+                    if start_ts.map_or(true, |ts| lock_info.get_lock_version() == ts) {
+                        // FIXME: "lock type" is lost in kvproto.
+                        let pk = escape(lock_info.get_primary_lock()).into_bytes();


Why escape?

I think primary key points another valid data key in rocksdb? So we escape it in order to we can easily pass it into another tikv-ctl command, such as print.

Output protobuf message in debug format will do that automatically.

BusyJay · 2017-10-17T11:47:36Z

src/bin/tikv-ctl.rs

+                        let k = escape(lock_info.get_key()).into_bytes();
+                        lock_info.set_primary_lock(pk);
+                        lock_info.set_key(k);
+                        println!("\tlock cf value: {}", print_to_string(&lock_info));


Why not use "{:?}"?

BusyJay · 2017-10-17T11:50:07Z

src/bin/tikv-ctl.rs

+                return;
+            }
+            (Some(region_local_1), Some(region_local_2)) => {
+                let start_key = keys::data_key(region_local_1.get_region().get_start_key());


Can it be checked by ==?

Compare two metapb.Region will also compare them peers, but here we only need to compare data in them.

I think they should be same if two regions is going to be the same.

BusyJay · 2017-10-17T11:51:07Z

src/bin/tikv-ctl.rs

+                let mvcc_infos_1 = self.get_mvcc_infos(start_key.clone(), end_key.clone(), 0);
+                let mvcc_infos_2 = rhs_debug_executor.get_mvcc_infos(start_key, end_key, 0);
+                let cmp_future = mvcc_infos_1.zip(mvcc_infos_2).for_each(|(item_1, item_2)| {
+                    let (key1, mvcc1) = (escape(&item_1.0), item_1.1);


escape is needed only when they are not equal.

BusyJay · 2017-10-17T11:51:32Z

src/bin/tikv-ctl.rs

+                    let (key1, mvcc1) = (escape(&item_1.0), item_1.1);
+                    let (key2, mvcc2) = (escape(&item_2.0), item_2.1);
+                    if key1 != key2 {
+                        let err_msg = format!("db1 cur key: {}, db2 cur key: {}", key1, key2);


Is it OK to return early here? /cc @zhangjinpeng1987

Do you mean we return a future::err here but not a future::ok, which causes the steping on the DBIterators stop? I think it's OK if the future lib can deconstruct those DBIterators correctly.

We need to print all different keys.

BusyJay · 2017-10-17T11:58:15Z

src/bin/tikv-ctl.rs

@@ -248,131 +705,78 @@ fn main() {
            println!("{}", &unescape(escaped).to_hex().to_uppercase());
            return;
        }
+        _ => {}


Should use unreachable!().

BusyJay

rest LGTM

BusyJay · 2017-10-17T13:05:13Z

src/bin/tikv-ctl.rs

+                    }
+                }
+                if cfs.contains(&CF_DEFAULT) {
+                    for value_info in mvcc.take_values().into_iter() {


Does for value_info in mvcc.take_values() work?

No, IntoIterator is implemented for &RepeatedField but not RepeatedField.

How about for value_info in mvcc.get_values()?

BusyJay · 2017-10-17T13:06:07Z

src/bin/tikv-ctl.rs

+use tikv::raftstore::store::debug::{Debugger, RegionInfo};
+use tikv::storage::{ALL_CFS, CF_DEFAULT, CF_LOCK, CF_WRITE};
+
+fn perror_and_exit<E: Error, T>(prefix: &str, e: E) -> T {


It's OK to use ! now.

BusyJay · 2017-10-17T14:07:32Z

LGTM, @zhangjinpeng1987 PTAL, you may have different thoughts on command diff_region.

hicqu · 2017-10-18T06:40:27Z

@BusyJay @zhangjinpeng1987 , PTAL thanks.

hicqu · 2017-10-18T08:51:38Z

/run-all-test

zhangjinpeng87

LGTM

hicqu · 2017-10-18T09:11:46Z

/run-all-test

hicqu · 2017-10-18T10:38:26Z

/run-all-test

tikv-ctl: refactor with debug API.

9fea2a4

hicqu requested a review from overvenus October 12, 2017 05:25

fix.

f3ab43e

BusyJay reviewed Oct 12, 2017

View reviewed changes

hicqu added 4 commits October 12, 2017 14:24

address comments.

18ae6f8

Merge branch 'master' into qupeng/debug-client

7bd3797

address comments.

db53976

Merge branch 'qupeng/debug-client' of github.com:pingcap/tikv into qu…

8ebb6da

…peng/debug-client

siddontang reviewed Oct 12, 2017

View reviewed changes

BusyJay reviewed Oct 13, 2017

View reviewed changes

hicqu added 2 commits October 13, 2017 16:50

address comments.

8a6db84

Merge branch 'master' into qupeng/debug-client

6425581

hicqu added 2 commits October 13, 2017 18:04

adjust command-line arguments.

8cf7a0c

I use some long options, for avoid name conflict.

Merge branch 'qupeng/debug-client' of github.com:pingcap/tikv into qu…

43e5b86

…peng/debug-client

BusyJay reviewed Oct 13, 2017

View reviewed changes

hicqu added 3 commits October 13, 2017 19:46

address comments.

62e637f

Merge branch 'master' into qupeng/debug-client

53ce25c

Merge branch 'master' into qupeng/debug-client

bb683db

hicqu added 3 commits October 16, 2017 12:32

Merge branch 'master' into qupeng/debug-client

33a0dae

improve command-line parse.

d2f2c9b

improve command-line parse.

82eb209

hicqu force-pushed the qupeng/debug-client branch from c9e3916 to 82eb209 Compare October 16, 2017 07:04

diff two regions should also compare their data, not only meta.

a47f8a4

overvenus reviewed Oct 16, 2017

View reviewed changes

overvenus reviewed Oct 17, 2017

View reviewed changes

BusyJay reviewed Oct 17, 2017

View reviewed changes

address comments.

382e556

BusyJay reviewed Oct 17, 2017

View reviewed changes

hicqu added 2 commits October 17, 2017 21:39

address comments.

cbe5db3

address comments.

a6d43c4

address comments.

cac3377

hicqu added 4 commits October 18, 2017 14:53

address comments.

42d427c

address comments.

32f8800

fix bug.

a4e09d7

fix bug

fd03258

zhangjinpeng87 reviewed Oct 18, 2017

View reviewed changes

zhangjinpeng87 approved these changes Oct 18, 2017

View reviewed changes

hicqu merged commit e57a49c into master Oct 18, 2017

hicqu deleted the qupeng/debug-client branch October 18, 2017 12:49

hicqu added a commit to hicqu/tikv that referenced this pull request Jan 10, 2018

tikv-ctl: refactor with debug API. (tikv#2377)

ba2163d

ngaut pushed a commit that referenced this pull request Jan 12, 2018

tikv-ctl: refactor with debug API. (#2377) (#2668)

4681e37

tikv-ctl: refactor with debug API. #2377

tikv-ctl: refactor with debug API. #2377

Conversation

hicqu commented Oct 12, 2017 • edited Loading

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 12, 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

siddontang commented Oct 13, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hicqu commented Oct 16, 2017

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

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

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

BusyJay commented Oct 17, 2017

hicqu commented Oct 18, 2017

hicqu commented Oct 18, 2017

zhangjinpeng87 left a comment

Choose a reason for hiding this comment

hicqu commented Oct 18, 2017

hicqu commented Oct 18, 2017

hicqu commented Oct 12, 2017 •

edited

Loading