Refactor into store-centric API #101

rrichardson · 2018-12-21T20:12:43Z

This depends on the 2018 edition PR #100

The Store types are now

SingleStore,
MultiStore,
IntegerStore,
MultiIntegerStore

The Reader/Writer types have been completely removed.. this now uses

lmdb::{ 
  Transaction, 
   RwTransaction, 
   RoTransaction, 
}

all *Store::get methods take a T: Transaction instance.

1 additional interface change:

When creating from Rkv::open... if we actually made a method for every permutation of store-type, flags, and create/not we'd have 16 create* functions.. So I created 4 open* functions which all take a open : bool and a flags : DatabaseFlags which all do what you'd expect.

…tting

rrichardson · 2018-12-22T08:35:13Z

OK.. @ncloudioj @mykmelez - This should be good to go. When fixing up the tests, I'm pretty happy how the types and lifetimes got cleaned up a little.

IntegerStore now will only work against a single type of PrimitiveInt (you can still open it with the wrong type)

Also, check out the Rkv::open* methods, I don't having to pass the extra params.. I guess the other option is to create a StoreConfig to house the flags and create-vs-open option.

mykmelez · 2019-01-04T18:19:19Z

@rrichardson, this branch could use a merge from master as well. Currently it still shows changes that have landed on master in #100.

…i-refactor

rrichardson · 2019-01-04T19:32:34Z

@mykmelez - huh.. git tells me that my store-api-refactor branch is up to date with master.

rrichardson · 2019-01-04T20:00:49Z

also says its up to date with rrichardson:2018-edition which merged in your changes..

mykmelez · 2019-01-04T20:48:27Z

Hmm, strange. The "Files changed" tab of this pull request is still showing the changes from #100. For example, examples/iterator.rs shows replacement of extern crate rkv; with use rkv;, even though those changes have landed on master.

mykmelez · 2019-01-04T21:25:11Z

Strangely, GitHub displays an up-to-date diff when I compare mozilla/master to rrichardson/store-api-refactor:

mozilla:master...rrichardson:store-api-refactor

It's just this PR's "Files changed" view that is out-of-date.

@rrichardson What happens when you merge the upstream master branch into this branch? When I do so, Git tells me that this branch is "Already up to date!" But then it creates a merge commit anyway:

01-04 13:17 > git merge upstream/master
Already up to date!
Merge made by the 'recursive' strategy.
01-04 13:17 > git log
commit 8d0834386aa3be8124ad37d83fa44668a938ed83 (HEAD -> rrichardson-store-api-refactor)
Merge: 25316f3 11442e2
Author: Myk Melez <myk@mykzilla.org>
Date:   Fri Jan 4 13:17:39 2019

    Merge remote-tracking branch 'upstream/master' into rrichardson-store-api-refactor

I wonder if pushing such a commit to this branch would convince GitHub to update "Files changed."

…i-refactor

mykmelez · 2019-01-07T23:59:20Z

Merge branch 'master' of https://github.com/mozilla/rkv into store-api-refactor

Thanks, that seems to have worked! The "Files changed" tab now only shows the changes on this branch, which makes it easier to review!

mykmelez

Also, check out the Rkv::open* methods, I don't having to pass the extra params.. I guess the other option is to create a StoreConfig to house the flags and create-vs-open option.

I don't know that there's a way to eliminate all extra arguments without multiplexing the open* functions, but a StoreConfig struct would halve them from two to one, and its value could be None if the StoreConfig parameter was optional (and the consumer wants the default config):

let store = env.open_single("foo", None);

Reducing configuration to a single argument would also allow us to retain open_or_create()_ variants without ending up with 16 permutations, as we'd only need a single _create_ variant for each open* function, for a total of 8:

pub fn open_single(…) {}
pub fn open_integer(…) {}
pub fn open_multi(…) {}
pub fn open_multi_integer(…) {}

pub fn open_or_create_single(…) {}
pub fn open_or_create_integer(…) {}
pub fn open_or_create_multi(…) {}
pub fn open_or_create_multi_integer(…) {}

And that would be useful, given that create: true without flags seems to be one of the more popular configurations of the open* calls.

Either way, given a StoreConfig struct that derives Default, Clone, and Copy:

#[derive(Default, Copy, Clone)]
pub struct StoreConfig {
    create: bool,
    flags: Option<DatabaseFlags>,
}

Consumers could pass StoreConfig::default() explicitly to indicate they want the default configuration, which is more self-documenting than the true, None argument pair (although it's also longer):

let store = env.open_single("foo", StoreConfig::default());

Consumers could also create static variants for popular configurations:

static CREATE: StoreConfig = StoreConfig { create: true, flags: None };
…
let store = env.open_single("foo", CREATE);

(I might bikeshed the name of the struct a bit. The leveldb crate calls its equivalent simply Options, which seems simpler and can be disambiguated if necessary via declarations like use Store::Options as StoreOptions.)

examples/simple-store.rs

src/env.rs

src/lib.rs

src/store/integermulti.rs

src/store/mod.rs

rrichardson · 2019-01-14T21:54:26Z

Arg.. I just ran cargo +nightly fmt and it still finds differences..

mykmelez

Thanks, @rrichardson, this looks great. I went through it again and noticed a few more nits, but no blockers; so this can land as-is, with the nits addressed in followups as we see fit. Thanks for all your hard work on this significant enhancement!

mykmelez · 2019-01-15T21:32:42Z

src/store/multi.rs

+        })
+    }
+
+    /// Provides a cursor to all of the values for the duplicate entries that match this key


Nit: this should say that the function provides the first value that matches this key.

mykmelez · 2019-01-15T21:46:15Z

src/store/multi.rs

+        match self.iter.next() {
+            None => None,
+            Some(Ok((key, bytes))) => Some((key, read_transform(Ok(bytes)))),
+            Some(Err(_)) => None,


Note: I'm going to change what Iter.next() returns in the Some(Err) case over in #105. But I'll merge this PR first, so that PR doesn't break it, and then update that PR as needed.

mykmelez · 2019-01-15T21:57:39Z

src/store/multi.rs

+    }
+}
+
+fn read_transform_owned(val: Result<&[u8], lmdb::Error>) -> Result<Option<OwnedValue>, StoreError> {


Nit: I don't see any uses of this in the codebase. Is it needed (or will soon be needed)?

I think this was related to the MultiIter impl that I mostly removed because I failed to implement it.

mykmelez · 2019-01-15T22:02:39Z

src/store/multi.rs

+
+    /// Insert a value at the specified key.
+    /// This put will allow duplicate entries.  If you wish to have duplicate entries
+    /// rejected, use the `put_with_flags` function and specify NO_DUP_DATA


Nit: we should add tests for the put_with_flags() functions as well, although we can do that in a followup.

mykmelez · 2019-01-15T22:05:42Z

src/store/multi.rs

+}
+
+/*
+impl<'env> Iterator for MultiIter<'env> {


Nit: this comment block (and the next one) references a MultiIter type, but that type isn't defined anywhere (not even in another comment block). It'd be helpful to include a declaration of that type along with this partial implementation, even if it's just a skeletal one. Or, perhaps better, move this code into an issue that tracks the problem and its resolution.

I would like to support a MultiIter. I have not been able to make it work re: lifetimes, it would need to copy over the cursor on every iteration (I think?) since it is an iterator of iterators.
I'd say leave it there, but ad a TODO or somesuch to remind us to do that in the future.

mykmelez · 2019-01-15T22:09:36Z

src/store/single.rs

+
+use crate::value::Value;
+
+fn read_transform(val: Result<&[u8], lmdb::Error>) -> Result<Option<Value>, StoreError> {


Nit: the two implementations of read_transform() in single.rs and multi.rs are identical and could be refactored into their super module store.rs.

mykmelez · 2019-01-15T22:11:40Z

Cargo.toml

 lazy_static = "1.0.2"
-lmdb-rkv = "0.9"


Nit: I'd preserve the alphabetical ordering here.

rrichardson added 12 commits November 26, 2018 16:30

added aspirational test for putting a value retrieved from a txn

9a24ece

started implementation of MultiStore/Reader/Writer

dc66714

more updates

8c0b270

multi RW and example completed

89f0d73

formatting

6da26ef

exporting Iter for Multi

3d9ae40

added get_first method, fixed some comments and hopefully fixed forma…

91479aa

…tting

removed exposure of MultiIter until I can figure out a good way to do it

d64386b

Merge branch '2018-edition' of github.com:rrichardson/rkv into multi

15004d7

new api interface complete.. making it compile is still WIP

b31d275

fixed lmdb-rs version

a43cb39

transition to new API is complete. All tests and examples pass

18cffcb

rrichardson changed the title ~~[WIP] Refactor into store-centric API~~ Refactor into store-centric API Dec 22, 2018

This was referenced Dec 22, 2018

Unify *Reader and *Writer #95

Closed

prevent applications from opening both named databases and default database #98

Open

rrichardson added 2 commits December 22, 2018 09:17

exporting LmdbError

ac708f1

merging upstream 2018 changes as well as format changes

5e0fbcb

mykmelez self-requested a review January 4, 2019 17:42

running rustfmt

25316f3

mykmelez mentioned this pull request Jan 4, 2019

remove unnecessary use declarations, self:: reference #102

Merged

Merge branch 'master' of https://github.com/mozilla/rkv into store-ap…

0d6e01e

…i-refactor

Merge branch 'master' of https://github.com/mozilla/rkv into store-ap…

a939dcd

…i-refactor

mykmelez suggested changes Jan 8, 2019

View reviewed changes

examples/simple-store.rs Show resolved Hide resolved

src/env.rs Show resolved Hide resolved

src/env.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

src/store/integermulti.rs Show resolved Hide resolved

src/store/mod.rs Outdated Show resolved Hide resolved

rrichardson added 3 commits January 13, 2019 12:24

Added Store::Options to clean up store creation, other tweaks for style

47f868b

added test for multi-integer-store

0b7c539

merging in master

2c0e2d8

formatting

a65e889

mykmelez approved these changes Jan 15, 2019

View reviewed changes

mykmelez merged commit a81dfbe into mozilla:master Jan 16, 2019

This was referenced Jan 28, 2019

fix nits and docs #111

Merged

committing transaction returns lmdb::Error instead of StoreError #115

Closed

rrichardson mentioned this pull request Jun 25, 2019

Export 'Readable' trait to allow user making generic method that take '&Readable' as parameter instead of '&Read' or '&Write' #131

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor into store-centric API #101

Refactor into store-centric API #101

rrichardson commented Dec 21, 2018 •

edited

rrichardson commented Dec 22, 2018

mykmelez commented Jan 4, 2019

rrichardson commented Jan 4, 2019

rrichardson commented Jan 4, 2019

mykmelez commented Jan 4, 2019

mykmelez commented Jan 4, 2019

mykmelez commented Jan 7, 2019

mykmelez left a comment

rrichardson commented Jan 14, 2019

mykmelez left a comment

mykmelez Jan 15, 2019

mykmelez Jan 15, 2019

rrichardson Jan 15, 2019

mykmelez Jan 15, 2019

rrichardson Jan 15, 2019

mykmelez Jan 15, 2019

mykmelez Jan 15, 2019

rrichardson Jan 15, 2019

mykmelez Jan 15, 2019

mykmelez Jan 15, 2019


		use crate::value::Value;

		fn read_transform(val: Result<&[u8], lmdb::Error>) -> Result<Option<Value>, StoreError> {

Refactor into store-centric API #101

Refactor into store-centric API #101

Conversation

rrichardson commented Dec 21, 2018 • edited

rrichardson commented Dec 22, 2018

mykmelez commented Jan 4, 2019

rrichardson commented Jan 4, 2019

rrichardson commented Jan 4, 2019

mykmelez commented Jan 4, 2019

mykmelez commented Jan 4, 2019

mykmelez commented Jan 7, 2019

mykmelez left a comment

Choose a reason for hiding this comment

rrichardson commented Jan 14, 2019

mykmelez 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

rrichardson commented Dec 21, 2018 •

edited