-
Notifications
You must be signed in to change notification settings - Fork 52
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
Conversation
9ebdd98
to
18831e1
Compare
f832db7
to
1a3c41d
Compare
@ncloudioj For more context, see https://docs.google.com/document/d/1aoVEcn1PR-W8qE2wOx0FSchO8qMpqaAzbt4pHktf-Wc |
@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! |
29f1f8c
to
6c2f213
Compare
Signed-off-by: Victor Porof <[email protected]>
Signed-off-by: Victor Porof <[email protected]>
Signed-off-by: Victor Porof <[email protected]>
Signed-off-by: Victor Porof <[email protected]>
Signed-off-by: Victor Porof <[email protected]>
Signed-off-by: Victor Porof <[email protected]>
Signed-off-by: Victor Porof <[email protected]>
@ncloudioj To recap, here's how I've split up this PR. 1. Pre: split key types into separate moduleThis commit makes it so that changeset no. 2 (a4d76eb) is readable in case of 2. Generalize LMDB usage and add support for multiple backing storesIsolates 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 Changeset no. 5 (4a955a7) actually implements the safe mode backend. 3. Drop copy and clone traits requirement on backend databasesThe current API requires that an environment returns an owned database (a.k.a. not a reference to one) to consumers when calling 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 It's possible to keep the 4. Drop eq and partial-eq trait requirement on backend databasesSame as above (commit no. 3). A smart pointer such as 5. Implement safe mode backing storeImplements 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 backendThese commits add more tests. |
Signed-off-by: Victor Porof <[email protected]>
Addressed comments. |
Landed in feature branch |
There was a problem hiding this 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.
Closing in favor of #159 |
WIP for allowing Rkv to use multiple backing stores.
"Safe mode" backing store implementation should now be a breeze.