kvdb-rocksdb: pass ReadOptions to iterators by ordian · Pull Request #277 · paritytech/parity-common

ordian · 2019-12-03T11:05:45Z

Passing prefix_same_as_start has no effect, since we're not using "Prefix Seek" mode, see https://github.com/facebook/rocksdb/blob/9befbe9b406574e8d8b687799701bd8cebf5163b/include/rocksdb/options.h#L1246-L1251 and https://github.com/facebook/rocksdb/wiki/Prefix-Seek-API-Changes.

dvdplm

This is pretty confusing, apologies if I'm misunderstanding this.

In order to use prefix search users must create a prefix extractor (rocksdb::SliceTransform::create_fixed_prefix(…)) when opening the DB and then pass it along when instantiating an iterator, is that correct?

The test_prefix_iterator() test in rust-rocksdb uses a different type though: Options (i.e. ffi::rocksdb_options_t) – do both work?

I think we have two options here:

land this PR here (with more docs/examples illustrating usage)
audit code using the prefix search and figure out if it really needs to; if, as I suspect, we're not using this, then maybe it's better to just remove support for it.

ordian · 2019-12-04T11:20:04Z

audit code using the prefix search and figure out if it really needs to; if, as I suspect, we're not using this

we are: 1, 2.

In order to use prefix search users must create a prefix extractor (rocksdb::SliceTransform::create_fixed_prefix(…)) when opening the DB and then pass it along when instantiating an iterator, is that correct?

yes

The test_prefix_iterator() test in rust-rocksdb uses a different type though: Options (i.e. ffi::rocksdb_options_t) – do both work?

set_prefix_extractor is set on Options (this is a global option, which affects the db layout, db is partitioned based on prefix),
set_prefix_same_as_start is called on ReadOptions when creating an Iterator
https://docs.rs/rocksdb/0.13.0/src/rocksdb/db.rs.html#1153

ordian · 2019-12-04T11:21:16Z

(with more docs/examples illustrating usage)

what do you mean, we haven't changed the public API here

dvdplm · 2019-12-04T11:38:42Z

what do you mean, we haven't changed the public API here

True, but the docs for the existing API are lacking and do not mention how to set up the prefix search properly. I think we want an example here, docs here and probably module level docs discussing the quirks of prefix search too?

dvdplm · 2019-12-04T11:40:57Z

audit code using the prefix search and figure out if it really needs to; if, as I suspect, we're not using this

we are: 1, 2.

I see. So that code needs to be updated after this PR then? And does it even work right now?

ordian · 2019-12-04T12:29:34Z

So that code needs to be updated after this PR then?

no logic has changed

But I just realised that we changed the logic in #257 8fb8f13#diff-4382bfb7e75959b2b8884268540abfa3R597, previously iter_from_prefix returned an iterator over all keys >= prefix, now it only returns an iterator of keys starting with the given prefix. I thought it was the expected behavior, but not sure now :/

ordian · 2019-12-04T13:06:56Z

But I just realised that we changed the logic in #257

After a quick search, in parity-ethereum we use iter_from_prefix only in epoch_transitions and then we filter it to start with prefix here. The same thing happens in substrate:
usage, filter. So even though we've changed the logic in #257, it doesn't affect parity-ethereum and substrate. The question is: do we want the new behavior or the old one?

dvdplm

lgtm modulo docs

dvdplm · 2019-12-04T14:34:26Z

 {
-	pub fn new(read_lock: RwLockReadGuard<'a, Option<T>>, col: Option<u32>) -> Self {
-		Self { inner: Self::new_inner(read_lock, |db| db.iter(col)) }
+	pub fn new(read_lock: RwLockReadGuard<'a, Option<T>>, col: Option<u32>, read_opts: &ReadOptions) -> Self {


<insert illuminating example here>. – needs some editing... ;)

…and there are other methods that now take ReadOptions that needs some docs?

Co-Authored-By: David <dvdplm@gmail.com>

dvdplm

There are still some docs that need some love, but overall lgtm.

dvdplm · 2019-12-05T10:17:13Z

 {
-	pub fn new(read_lock: RwLockReadGuard<'a, Option<T>>, col: Option<u32>) -> Self {
-		Self { inner: Self::new_inner(read_lock, |db| db.iter(col)) }
+	pub fn new(read_lock: RwLockReadGuard<'a, Option<T>>, col: Option<u32>, read_opts: &ReadOptions) -> Self {


<insert illuminating example here>. – needs some editing... ;)

…and there are other methods that now take ReadOptions that needs some docs?

dvdplm · 2019-12-10T06:47:21Z

ping @bkchr

* master: Fix a typo in error rlp error message (#283) [kvdb*] Make column type u32 instead of Option<u32> (#278) [kvdb-rocksdb] Add benchmark for point lookups (#275) Compile triehash for no_std (#280)

niklasad1

LGTM but I'm not super familiar with this code, thus would be good if @bkchr could sign off on it.

bkchr

Besides the read lock in the comments it looks good.

bkchr · 2019-12-19T11:19:37Z

 	/// Create an `Iterator` over the default DB column or over a `ColumnFamily` if a column number
 	/// is passed.
-	fn iter(&self, col: u32) -> Self::Iterator;
+	/// In addition to a read lock and a column index, it takes a ref to the same `ReadOptions` we


Not sure why it talks about the read lock here? That is not part of the interface?

Same below.

* master: kvdb-rocksdb: pass ReadOptions to iterators (#277)

kvdb-rocksdb: pass ReadOptions to iterators

eb88d20

ordian added the A0-pleasereview label Dec 3, 2019

ordian requested review from bkchr and dvdplm December 3, 2019 11:05

dvdplm reviewed Dec 4, 2019

View reviewed changes

ordian mentioned this pull request Dec 4, 2019

[kvdb*] Make column type u32 instead of Option<u32> #278

Merged

dvdplm approved these changes Dec 4, 2019

View reviewed changes

kvdb-rocksdb: add some iter docs

18811bc

dvdplm suggested changes Dec 5, 2019

View reviewed changes

Comment thread kvdb-rocksdb/src/iter.rs Outdated

Comment thread kvdb-rocksdb/src/iter.rs Outdated

Comment thread kvdb-rocksdb/src/iter.rs Outdated

Comment thread kvdb-rocksdb/src/iter.rs Outdated

Comment thread kvdb-rocksdb/src/iter.rs Outdated

ordian and others added 2 commits December 5, 2019 11:07

Apply David's suggestions from code review

b428c3c

Co-Authored-By: David <dvdplm@gmail.com>

Merge branch 'master' into ao-pass-read-option-to-prefix-iter

b2c1dff

dvdplm approved these changes Dec 5, 2019

View reviewed changes

kvdb-rocksdb: postprocess docs

429a02d

dvdplm requested a review from niklasad1 December 16, 2019 07:48

Merge branch 'master' into ao-pass-read-option-to-prefix-iter

cb94d4f

* master: Fix a typo in error rlp error message (#283) [kvdb*] Make column type u32 instead of Option<u32> (#278) [kvdb-rocksdb] Add benchmark for point lookups (#275) Compile triehash for no_std (#280)

niklasad1 reviewed Dec 17, 2019

View reviewed changes

formatting

9c59871

bkchr approved these changes Dec 19, 2019

View reviewed changes

dvdplm merged commit 5837dff into master Dec 19, 2019

dvdplm deleted the ao-pass-read-option-to-prefix-iter branch December 19, 2019 11:20

dvdplm added a commit that referenced this pull request Dec 19, 2019

Merge branch 'master' into dp/feature/add-num-entries-to-rocksdb

447e192

* master: kvdb-rocksdb: pass ReadOptions to iterators (#277)

Conversation

ordian commented Dec 3, 2019

Uh oh!

dvdplm left a comment

Choose a reason for hiding this comment

Uh oh!

ordian commented Dec 4, 2019

Uh oh!

ordian commented Dec 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dvdplm commented Dec 4, 2019

Uh oh!

dvdplm commented Dec 4, 2019

Uh oh!

ordian commented Dec 4, 2019

Uh oh!

ordian commented Dec 4, 2019

Uh oh!

dvdplm left a comment

Choose a reason for hiding this comment

Uh oh!

dvdplm Dec 4, 2019

Choose a reason for hiding this comment

Uh oh!

dvdplm Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dvdplm left a comment

Choose a reason for hiding this comment

Uh oh!

dvdplm Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

dvdplm commented Dec 10, 2019

Uh oh!

niklasad1 left a comment

Choose a reason for hiding this comment

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

bkchr Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ordian commented Dec 4, 2019 •

edited

Loading