-
Notifications
You must be signed in to change notification settings - Fork 790
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
Add submodule lmdbxx which is a c++ wrapper for lmdb #4724
base: develop
Are you sure you want to change the base?
Conversation
…ction name described.
…ace as a separation of concerns.
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.
Looks OK, but I'd argue that abandoning nano::store::lmdb::env
class instead of embracing it and introducing nano::store::env
as an abstraction for acid key-value stores is counterproductive to the goal of unifying lmdb and rocksdb codebases.
It also seems like assert for mismatched store ids got lost in the refactor, this would crash with mismatched store message on current develop:
TEST (block_store_DeathTest, transaction_mismatch)
{
bool init;
nano::store::lmdb::env env{ init, nano::unique_path () / "test.ldb" };
ASSERT_FALSE (init);
auto transaction = env.tx_begin_write ();
nano::logger logger;
auto store = nano::make_store (logger, nano::unique_path (), nano::dev::constants);
ASSERT_FALSE (store->init_error ());
ASSERT_DEATH (store->block.begin (transaction), "");
}
This change adds the lmdbxx submodule which is a c++ wrapper for LMDB. https://github.com/hoytech/lmdbxx
Currently we're using an amalgamation of direct access to the LMDB C API and some custom c++ wrapping of select portions of functionality.
Major motivations for using this library:
The library has two implementation strategies, a near 1-to-1 wrapping of the C API with exception handling, and full RAII types. Migrating existing code to use the direct API was relatively painless and some of the RAII types can already be used.
The primary motivation for the change is to get a clear separation of the lmdb c++ wrapper from node-specific logic. Additionally, this removes the burden of wrapping the C API from the node code.
The library itself seems to make good design decisions and is reasonably maintained.
There are parts of the library which are not fully complete e.g. ::lmdb::dbi does not fully implement RAII, it is both copyable and doesn't destroy the underlying resources when it goes out of scope. To get full benefits we will likely have to upstream some patches which they will hopefully be receptive and responsive to.