debug: add scan_mvcc support. #2335

hicqu · 2017-09-25T11:06:05Z

This PR add support for debug ScanMvcc API.

hicqu · 2017-09-25T11:08:07Z

@overvenus , PTAL, thanks.

overvenus · 2017-09-25T12:16:28Z

src/server/service/debug.rs

+                Ok(_) => Ok(dealer.0.unwrap()),
+                Err(e) => Err((dealer.0.unwrap(), e)),
+            };
+            future::result(result)


I think result is enough.

pool.spawn_fn requires a closure returning Future, so I believe we need future::result(...).

overvenus · 2017-09-25T12:22:06Z

src/raftstore/store/debug.rs

+    fn mvcc_kvs(&self, cf: CfName, from: Vec<u8>, to: Vec<u8>) -> Receiver<Kv> {
+        let (tx, rx) = channel::<Kv>();
+        let db = self.engines.kv_engine.clone();
+        thread::spawn(move || -> Result<()> {


Please spawn threads with names.

overvenus · 2017-09-25T12:23:00Z

src/raftstore/store/debug.rs

+        let db = self.engines.kv_engine.clone();
+        thread::spawn(move || -> Result<()> {
+            for kv in &mut try!(gen_mvcc_iter(db.as_ref(), cf, &from, &to)) {
+                box_try!(tx.send(kv));


How about if let Err(_) = tx.send() { return; }?

We also need to call gen_mvcc_iter in the thread closure, which also returns a Result.
So let the closure return a Result is better?

overvenus · 2017-09-25T16:36:35Z

src/server/service/debug.rs

+        mvcc_resp.set_info(mvcc);
+
+        let sink = self.0.take().unwrap();
+        match sink.send((mvcc_resp, WriteFlags::default())).wait() {


Attach an RpcContext to MvccKVToSink and spawn the sink.send(), so that we dont need to call wait().

Because sink.send is called in other threads in pool and RpcContext can't Send, I think we can't do this.

overvenus · 2017-09-25T16:43:36Z

src/server/service/debug.rs

+            Ok(mut sink) => {
+                let f = future::poll_fn(move || sink.close())
+                    .map_err(|e| Service::on_grpc_error("scan_mvcc", e));
+                Box::new(f) as Box<Future<Item = (), Error = ()> + Send>


I think spawning f here is fine.

I tried but the RpcContext is not Send, so I can't call ctx.spawn(f) here.

I mean call ctx.spwan() here, so f does not need to cast a BoxFuture .

overvenus · 2017-09-26T12:15:00Z

src/raftstore/store/debug.rs

+        let (tx, rx) = channel::<Kv>();
+        let db = self.engines.kv_engine.clone();
+        thread::Builder::new()
+            .name(format!("scan_mvcc_{}", cf))


I think you can use thd_name!().

overvenus · 2017-09-26T12:23:48Z

src/raftstore/store/debug.rs

@@ -151,6 +166,206 @@ impl Debugger {
            Err(e) => Err(box_err!(e)),
        }
    }
+
+    pub fn scan_mvcc<D, E>(&self, req: ScanMvccRequest, dealer: &mut D) -> Result<()>


How about passing a futures::sync::mpsc::UnboundedSender<MvccInfo>?

let (tx, rx) = unbounded(); self .pool .spwan( ok(self.debugger.clone()) .and_then(|debugger| debugger.scan_mvcc(req, tx) ) .forgot(); let f = sink.send_all(rx).and_then(|sink| poll_fn(sink.close())); ctx.spawn(f);

overvenus · 2017-09-26T12:32:19Z

src/raftstore/store/debug.rs

+    #[test]
+    fn test_scan_mvcc() {
+        struct MvccCounter(usize);
+        impl MvccKVDealer for MvccCounter {


Please also add a test in test_servics.rs .

hicqu · 2017-09-27T10:06:37Z

@overvenus , Now we don't need useless MvccKVDealer trait. PTAL again, thanks.

hicqu · 2017-09-27T10:07:09Z

@BusyJay , PTAL, thanks.

siddontang · 2017-09-28T07:12:23Z

PTAL @BusyJay @overvenus

overvenus · 2017-09-28T09:11:06Z

src/raftstore/store/debug.rs

+                    if limit != 0 && count >= limit {
+                        break;
+                    }
+                    if count >= limit {}


overvenus · 2017-09-28T09:12:11Z

src/raftstore/store/debug.rs

+
+        let mut mvcc_infos: BTreeMap<Vec<u8>, (MvccInfo, bool, bool, bool)> = BTreeMap::new();
+        let (mut want_lock, mut want_default, mut want_write) = (true, true, true);
+        let (mut m_l, mut m_d, mut m_w) = (None, None, None);


I think we need a better name.

overvenus · 2017-09-28T09:17:08Z

src/raftstore/store/debug.rs

+                break;
+            }
+            if want_lock {
+                if let Ok(kv) = lock_rx.recv() {


Maybe we can use select!() [1].

[1] https://doc.rust-lang.org/std/macro.select.html

overvenus · 2017-09-28T09:20:59Z

src/raftstore/store/debug.rs

+                }
+                want_write = false;
+            }
+            for key in mvcc_infos.keys().cloned().collect::<Vec<_>>() {


How about entry()[1]?

[1] https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.entry

overvenus · 2017-09-28T09:22:24Z

src/raftstore/store/debug.rs

+                want_write = false;
+            }
+            for key in mvcc_infos.keys().cloned().collect::<Vec<_>>() {
+                let mvcc_and_flags = mvcc_infos.remove(&key).unwrap();


See also OccupiedEntry::remove_entry() [1].

[1] https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.entry

overvenus · 2017-09-28T09:30:02Z

src/raftstore/store/debug.rs

+                    }
+                    m_w = Some(key);
+                } else {
+                    m_w = Some(vec![0xffu8]);


What does '0xffu8' mean?

hicqu · 2017-09-29T06:11:05Z

/run-all-test

hicqu · 2017-09-29T06:11:18Z

@BusyJay , PTAL, thanks.

overvenus

LGTM

hicqu · 2017-10-05T07:10:55Z

@BusyJay PTAL thanks.

siddontang · 2017-10-09T03:17:53Z

PTAL @BusyJay

BusyJay · 2017-10-09T06:59:26Z

src/raftstore/store/debug.rs

-    /// Compact the cf[start..end) in the db **by manual**.
+    pub fn scan_mvcc(&self, mut req: ScanMvccRequest) -> Result<MvccInfoIterator> {
+        let from_key = req.take_from_key();
+        let to_key = Some(req.take_to_key()).into_iter().find(|k| !k.is_empty());


Use if else instead.

BusyJay · 2017-10-09T07:04:20Z

src/server/service/debug.rs

+                        .map(|_| ())
+                })
+                .map_err(|e| Service::on_grpc_error("scan_mvcc", &e))
+                .wait();


Because DBIterator is !Send so that we can't use it directly in spawn.

BusyJay · 2017-10-09T07:05:12Z

src/server/service/debug.rs

+                .wait();
+            result
+        };
+        self.pool.spawn_fn(future).forget();


You can use spawn instead of spawn_fn.

Use spawn needs a Future instead of Closure returns Future. But if the future contains any DBIterator, it can't be Send, so that it can't spawn.

hicqu · 2017-10-11T04:14:18Z

@BusyJay , PTAL thanks.

hicqu · 2017-10-11T09:26:33Z

@BusyJay , PTAL, thanks.

BusyJay · 2017-10-11T09:58:33Z

LGTM, though I think MvccInfoIterator can still be improved.

BusyJay · 2017-10-11T09:58:44Z

/run-integration-tests

siddontang · 2017-10-11T10:04:35Z

PTAL @overvenus

… qupeng/debug-scan-mvcc

BusyJay

rest LGTM

BusyJay · 2017-10-11T12:27:38Z

src/raftstore/store/debug.rs

+                    mvcc_info.set_values(values);
+                },
+                Ordering::Greater => {}
+                _ => unreachable!(),


If there is a bug, it's reachable. So should return all those keys instead of panic.

BusyJay · 2017-10-11T12:28:39Z

src/raftstore/store/keys.rs

@@ -199,6 +200,11 @@ pub fn origin_key(key: &[u8]) -> &[u8] {
    &key[DATA_PREFIX_KEY.len()..]
 }

+/// The caller should ensure the key is a timestamped key.
+pub fn truncate_ts(key: &[u8]) -> &[u8] {


The function should be put in types.rs. All function in this file has nothing related to MVCC.

hicqu · 2017-10-12T03:53:48Z

@overvenus , PTAL again, thanks.

BusyJay · 2017-10-12T03:58:28Z

src/raftstore/store/debug.rs

+                },
+                Ordering::Greater => {}
+                _ => {
+                    let err_msg = format!(


/cc @disksing, is an error message or listing all the related kvs better help with debuging?

overvenus · 2017-10-12T04:42:04Z

/run-all-test

debug: add scan_mvcc support.

f5b8e0d

hicqu requested a review from overvenus September 25, 2017 11:07

fix.

24de483

overvenus reviewed Sep 25, 2017

View reviewed changes

hicqu added 4 commits September 26, 2017 11:43

Merge branch 'master' into qupeng/debug-scan-mvcc

788177e

address comments.

0170c20

add test cases.

32a30aa

address comments.

3607d11

overvenus reviewed Sep 26, 2017

View reviewed changes

hicqu added 5 commits September 27, 2017 15:00

address comments.

ee3d7cc

address comments.

15b645d

remove MvccKVDealer.

23d7d15

add limit support.

281d292

Merge branch 'master' into qupeng/debug-scan-mvcc

5343e95

hicqu requested review from BusyJay and overvenus September 27, 2017 10:06

fix ci.

271cd43

hicqu added 2 commits September 28, 2017 15:22

Merge branch 'master' into qupeng/debug-scan-mvcc

5bdc11a

fix bug.

8841d63

overvenus reviewed Sep 28, 2017

View reviewed changes

hicqu added 3 commits September 28, 2017 21:44

address comments.

d825c1c

address comments.

ab8752b

Merge branch 'master' into qupeng/debug-scan-mvcc

6e1f6e3

overvenus reviewed Sep 29, 2017

View reviewed changes

hicqu added 3 commits September 30, 2017 19:02

address comments.

73f7f4a

Merge branch 'master' into qupeng/debug-scan-mvcc

ff68046

address comments.

98ee00f

BusyJay reviewed Oct 9, 2017

View reviewed changes

hicqu added 6 commits October 9, 2017 15:12

address comments.

8004a7b

add more tests.

346bd7d

Merge branch 'master' into qupeng/debug-scan-mvcc

e25f0d3

fix

f74d231

Merge branch 'master' into qupeng/debug-scan-mvcc

83cb4c8

send future with DBIterator.

d829163

hicqu added 3 commits October 11, 2017 12:15

remove useless comments.

e0c3b18

address comments.

3c89f28

Merge branch 'master' into qupeng/debug-scan-mvcc

82c3c66

hicqu added 2 commits October 11, 2017 20:16

address comments.

4051158

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

a461ad3

… qupeng/debug-scan-mvcc

BusyJay reviewed Oct 11, 2017

View reviewed changes

address comments.

e88e791

BusyJay reviewed Oct 12, 2017

View reviewed changes

overvenus approved these changes Oct 12, 2017

View reviewed changes

hicqu merged commit 464c161 into master Oct 12, 2017

hicqu deleted the qupeng/debug-scan-mvcc branch October 12, 2017 05:10

debug: add scan_mvcc support. #2335

debug: add scan_mvcc support. #2335

Conversation

hicqu commented Sep 25, 2017

hicqu commented Sep 25, 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

hicqu commented Sep 27, 2017

hicqu commented Sep 27, 2017

siddontang commented Sep 28, 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

hicqu commented Sep 29, 2017

hicqu commented Sep 29, 2017

overvenus left a comment

Choose a reason for hiding this comment

hicqu commented Oct 5, 2017

siddontang commented Oct 9, 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

hicqu commented Oct 11, 2017

hicqu commented Oct 11, 2017

BusyJay commented Oct 11, 2017

BusyJay commented Oct 11, 2017

siddontang commented Oct 11, 2017

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

hicqu commented Oct 12, 2017

Choose a reason for hiding this comment

overvenus commented Oct 12, 2017