Add submodule lmdbxx which is a c++ wrapper for lmdb #4724
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ction name described.
…ace as a separation of concerns.
pwojcikdev
approved these changes
Sep 19, 2024
There was a problem hiding this comment.
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), "");
}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.