add upper bound for iterator #1060

zhangjinpeng87 · 2016-09-12T08:53:26Z

Add upper bound for iterator to speed up region scan when there are a lot of deleted keys.

siddontang · 2016-09-12T09:22:02Z

Any benchmark?

siddontang · 2016-09-12T09:31:26Z

Please merge master to fix CI first.

zhangjinpeng87 · 2016-09-12T09:38:33Z

In theory, with upper bound key, iterator will return invalid as soon as it reach first key greater than or equal to the upper bound key, regardless of it is a normal key/value pair or deleted key. And I will bench it.

BusyJay · 2016-09-15T03:16:43Z

LGTM

siddontang · 2016-09-17T08:38:19Z

src/raftstore/coprocessor/region_snapshot.rs

@@ -324,7 +316,7 @@ mod tests {
        assert_eq!(data.len(), 2);
        assert_eq!(data, &base_data[1..3]);

-        let mut iter = snap.iter();
+        let mut iter = snap.iter(None);


any test to cover not None key?

siddontang · 2016-09-19T00:46:49Z

Ping @zhangjinpeng1987

…deleted keys

zhangjinpeng87 · 2016-09-19T07:31:10Z

@BusyJay @disksing @siddontang PTAL

siddontang · 2016-09-19T07:48:40Z

src/raftstore/coprocessor/region_snapshot.rs

-    pub fn iter(&self) -> RegionIterator {
-        RegionIterator::new(self.snap.new_iterator(), self.region.clone())
+    pub fn iter(&self, upper_bound: Option<&[u8]>) -> RegionIterator {
+        if upper_bound.is_none() {


can use upper_bound.or_else() to simplify the code.

siddontang · 2016-09-19T07:49:37Z

src/raftstore/coprocessor/region_snapshot.rs

-                    end_key: &[u8],
-                    f: &mut F)
-                    -> Result<()>
+    fn scan_impl<F>(&self, mut it: RegionIterator, start_key: &[u8], f: &mut F) -> Result<()>


do we have a test to check that we can't get the key >= upper bound key.

BusyJay · 2016-09-19T07:52:52Z

src/storage/engine/raftkv.rs

-        Ok(box RegionSnapshot::iter(self))
+        if upper_bound.is_none() {
+            Ok(box RegionSnapshot::iter(self,
+                                        Some(keys::enc_end_key(self.get_region()).as_slice())))


Why enc_end_key here?

BusyJay · 2016-09-19T07:56:07Z

src/storage/mvcc/reader.rs

@@ -179,7 +176,7 @@ impl<'a> MvccReader<'a> {
        where F: Fn(&Lock) -> bool
    {
        if self.lock_cursor.is_none() {
-            self.lock_cursor = Some(try!(self.snapshot.iter_cf(CF_LOCK)));
+            self.lock_cursor = Some(try!(self.snapshot.iter_cf(CF_LOCK, None)));


Do we need to add upper bound here?

No, RegionIterator will add region's end_key as upper bound.

disksing · 2016-09-19T08:00:46Z

src/raftstore/coprocessor/region_snapshot.rs

+                                    .as_slice())),
+                                self.region.clone())
+        } else {
+            RegionIterator::new(self.snap.new_iterator(upper_bound), self.region.clone())


I think upper_bound need to be encoded here.

It's better to add a new function in RegionIterator to handle the encode/decode thing.

BusyJay · 2016-09-19T09:39:04Z

src/raftstore/coprocessor/region_snapshot.rs

+               cf: &str)
+               -> RegionIterator<'a> {
+        let encoded_upper =
+            upper_bound.map_or_else(|| keys::enc_end_key(&region), |v| keys::data_key(v));


Just keys::data_key should be ok.

BusyJay · 2016-09-19T09:40:55Z

src/raftstore/coprocessor/region_snapshot.rs

@@ -140,7 +126,19 @@ pub struct RegionIterator<'a> {
 // we use rocksdb's style iterator, so omit the warning.
 #[allow(should_implement_trait)]
 impl<'a> RegionIterator<'a> {
-    pub fn new(iter: DBIterator<'a>, region: Region) -> RegionIterator<'a> {
+    pub fn new(snap: &'a Snapshot,


Can we use two function new and new_cf here?

disksing · 2016-09-19T09:50:04Z

src/raftstore/coprocessor/region_snapshot.rs

+                break;
+            }
+        }
+        let expected_res = vec![(b"a1".to_vec(), b"v1".to_vec()), (b"a3".to_vec(), b"v3".to_vec())];


You can use base_data like L373.

No, we can't, we add upper_bound for iterator and can't get all datas.

You can slice it like L328 :)

BusyJay · 2016-09-19T10:25:32Z

src/raftstore/coprocessor/region_snapshot.rs

+               region: Region,
+               upper_bound: Option<&[u8]>)
+               -> RegionIterator<'a> {
+        let encoded_upper = upper_bound.map_or_else(|| keys::enc_end_key(&region), keys::data_key);


What if upper_bound is ""?

disksing · 2016-09-19T11:08:11Z

LGTM

BusyJay

rest LGTM

BusyJay · 2016-09-19T11:08:33Z

src/raftstore/coprocessor/region_snapshot.rs

@@ -157,6 +159,14 @@ impl<'a> RegionIterator<'a> {
        }
    }

+    fn adjust_upper_bound(upper_bound: Option<&[u8]>) -> Option<&[u8]> {


Define as a local function instead.

BusyJay · 2016-09-19T11:16:45Z

LGTM

Signed-off-by: hslam <[email protected]>

…by default (tikv#1060) * set override_priority with resource group priority Signed-off-by: glorv <[email protected]> * update go.mod Signed-off-by: glorv <[email protected]> * fix typo Signed-off-by: glorv <[email protected]> * fix Signed-off-by: glorv <[email protected]> --------- Signed-off-by: glorv <[email protected]>

siddontang reviewed Sep 17, 2016

View reviewed changes

zhangjinpeng87 added 4 commits September 19, 2016 13:01

add upper_bound for iterator to speed up scan when there is a lot of …

7969083

…deleted keys

use upper bound to instead of comparing with end_key

d0971f7

fix ci

81660cc

update rust-rocksdb version

d5b5cf1

zhangjinpeng87 force-pushed the zhangjinpeng/iterator_with_upper_bound branch from 87fd424 to d5b5cf1 Compare September 19, 2016 05:23

add upper bound for region iterator

edd215a

siddontang reviewed Sep 19, 2016

View reviewed changes

BusyJay reviewed Sep 19, 2016

View reviewed changes

disksing reviewed Sep 19, 2016

View reviewed changes

zhangjinpeng87 added 2 commits September 19, 2016 16:53

address comments

0cb8976

add test for iterator with upper bound

6fd109d

BusyJay reviewed Sep 19, 2016

View reviewed changes

disksing reviewed Sep 19, 2016

View reviewed changes

zhangjinpeng87 added 2 commits September 19, 2016 17:56

address comments

347dc37

address comments

9d7444b

BusyJay reviewed Sep 19, 2016

View reviewed changes

address comments

fbb3a47

BusyJay reviewed Sep 19, 2016

View reviewed changes

address comments

e3f7265

zhangjinpeng87 merged commit 8d5d2c0 into master Sep 19, 2016

zhangjinpeng87 deleted the zhangjinpeng/iterator_with_upper_bound branch September 19, 2016 11:22

iosmanthus pushed a commit to iosmanthus/tikv that referenced this pull request Oct 30, 2023

set the time to live of the remote analysis cache (tikv#1060)

49062d4

Signed-off-by: hslam <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add upper bound for iterator #1060

add upper bound for iterator #1060

zhangjinpeng87 commented Sep 12, 2016

siddontang commented Sep 12, 2016

siddontang commented Sep 12, 2016

zhangjinpeng87 commented Sep 12, 2016

BusyJay commented Sep 15, 2016

siddontang Sep 17, 2016

siddontang commented Sep 19, 2016

zhangjinpeng87 commented Sep 19, 2016

siddontang Sep 19, 2016

siddontang Sep 19, 2016

BusyJay Sep 19, 2016

BusyJay Sep 19, 2016

zhangjinpeng87 Sep 19, 2016

disksing Sep 19, 2016

BusyJay Sep 19, 2016

BusyJay Sep 19, 2016

BusyJay Sep 19, 2016

disksing Sep 19, 2016

zhangjinpeng87 Sep 19, 2016

disksing Sep 19, 2016

BusyJay Sep 19, 2016

disksing commented Sep 19, 2016

BusyJay left a comment

BusyJay Sep 19, 2016

BusyJay commented Sep 19, 2016

add upper bound for iterator #1060

add upper bound for iterator #1060

Conversation

zhangjinpeng87 commented Sep 12, 2016

siddontang commented Sep 12, 2016

siddontang commented Sep 12, 2016

zhangjinpeng87 commented Sep 12, 2016

BusyJay commented Sep 15, 2016

Choose a reason for hiding this comment

siddontang commented Sep 19, 2016

zhangjinpeng87 commented Sep 19, 2016

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

disksing commented Sep 19, 2016

BusyJay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BusyJay commented Sep 19, 2016