Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement multiple backing stores for Rkv #158

Closed
wants to merge 8 commits into from
Closed

Implement multiple backing stores for Rkv #158

wants to merge 8 commits into from

Conversation

victorporof
Copy link
Contributor

@victorporof victorporof commented Oct 21, 2019

WIP for allowing Rkv to use multiple backing stores.
"Safe mode" backing store implementation should now be a breeze.

@victorporof
Copy link
Contributor Author

@victorporof
Copy link
Contributor Author

victorporof commented Oct 23, 2019

Also fixes #117 and #2

@ncloudioj
Copy link
Member

@victorporof could you please add some comments to outline the major changes of this patch? The attached document has given me a great holistic rationale for this change, it will be very helpful for me to reivew the code with those inputs.

Thanks!

@victorporof victorporof force-pushed the wip branch 6 times, most recently from 29f1f8c to 6c2f213 Compare October 31, 2019 12:09
@victorporof
Copy link
Contributor Author

victorporof commented Nov 1, 2019

@ncloudioj To recap, here's how I've split up this PR.
It would be excellent if we could land this today.

1. Pre: split key types into separate module

This commit makes it so that changeset no. 2 (a4d76eb) is readable in case of src/store/integer.rs.

2. Generalize LMDB usage and add support for multiple backing stores

Isolates LMDB's surface area that this crate requires, splitting it into a set of interdependent traits. Structures implementing these traits are usable by RKV as a backing engine for key-value storage.

This commit also implements these traits for newtypes wrapping LMDB itself. This showcases the only changes required to user code when picking the LMDB backend, which involves the turbofish syntax (e.g Rkv::new<Lmdb>(...)).

Changeset no. 5 (4a955a7) actually implements the safe mode backend.

3. Drop copy and clone traits requirement on backend databases

The current API requires that an environment returns an owned database (a.k.a. not a reference to one) to consumers when calling open_db or create_db. This database type must be the same everywhere, and the instance returned must refer to the same database with every call as well, a constraint expressed through the Database trait and associated types in the various traits defined in commit no. 2.

In case of LMDB, we do this by simply copying the structs representing LMDB databases. This is cheap in those situations because they're just wrappers around pointers to the C data structures.

In case of the safe mode implementation, we don't want to clone the entire database in this scenario. Returning an owned database without duplicating its contents is easy through smart pointers such as Arc. However we can't copy an Arc<T>, and since we don't want to do this unsafely, dropping the Copy trait requirement is (probably) the way to go.

It's possible to keep the Clone requirement instead of Copy if we're particular about having the Database types clone-able, however this doesn't seem necessary and I'd prefer to have consumers wrap databases in smart pointers themselves if they need this.

4. Drop eq and partial-eq trait requirement on backend databases

Same as above (commit no. 3). A smart pointer such as RwLock cannot easily derive equality, and we need to wrap Databases in reference counted read-write locks to satisfy the existing API and avoid unnecessarily duplicating entire database when calling open_db or create_db, as well as in other cases. We could manually check equality with built-in methods like Arc::ptr_eq, but things get complicated for Arc<RwLock<T>>, and in practice this feature doesn't seem necessary.

5. Implement safe mode backing store

Implements the traits describing a backing store (defined in commit no. 2). We don't aim for feature completeness here, just enough so that various RKV tests still pass (to verify that the implementation actually behaves like LMDB in the most common cases). CRLite support dictates how relaxed we can be about performance.

6. Add more tests for the rkv backend and 7. Add more tests for the safe mode backend

These commits add more tests.

@victorporof
Copy link
Contributor Author

Addressed comments.

@victorporof
Copy link
Contributor Author

victorporof commented Nov 1, 2019

Landed in feature branch safe-mode: https://github.com/mozilla/rkv/tree/safe-mode

Copy link
Member

@ncloudioj ncloudioj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

NB: I haven't thoroughly reviewed the design yet, let's keep a close look at how it performs in the wild. And keep polishing it if necessary.

@victorporof
Copy link
Contributor Author

Closing in favor of #159

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants