MINIFICPP-2849 Implement LMDB based content repository - #2201
Conversation
| size_t LmdbStream::write(const uint8_t* value, size_t size) { | ||
| if (!write_enable_) { return STREAM_ERROR; } | ||
| if (size != 0 && IsNullOrEmpty(value)) { return STREAM_ERROR; } | ||
| value_.append(reinterpret_cast<const char*>(value), size); |
There was a problem hiding this comment.
LMDB does not have an append function when writing a value like RocksDB's Merge function, so instead of rereading the original value, appending to it, then writing back the new value, all the writes are buffered until the stream is closed, that's when the actual write and commit happens. Currently all content repository streams are used either for write-only or read-only use cases, so there should be no use case where reads and writes are mixed. This should be addressed in a separate PR to change the content repository interface to use separate OutputStream and InputStream types for reads and writes to enforce this, which would also result in separate LmdbInputStream and LmdbOutputStream types (same for RocksDB).
There was a problem hiding this comment.
Pull request overview
Implements an LMDB-backed Content Repository extension for MiNiFi C++ (as an alternative to the RocksDB-based DatabaseContentRepository), including build integration, configuration plumbing, and dedicated unit tests.
Changes:
- Adds a new
minifi-lmdbextension implementingLmdbContentRepositoryandLmdbStream, plus LMDB-focused unit tests. - Wires LMDB into configuration (new
nifi.content.repository.lmdb.max.db.sizeproperty) and improves user-facing error reporting/docs. - Introduces a FetchContent-based LMDB third-party build with patching, and enables LMDB in CI/default extensions.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| thirdparty/lmdb/fix-windows-symbols.patch | Patches upstream LMDB to avoid Windows NT API symbol collisions. |
| thirdparty/lmdb/add-cmake-file.patch | Adds a CMakeLists.txt to LMDB upstream sources for cross-platform builds. |
| minifi-api/include/minifi-cpp/properties/Configuration.h | Adds config key for LMDB max DB size. |
| libminifi/test/libtest/unit/TestBase.h | Adds virtual destructor to TestController for safer polymorphic use. |
| libminifi/src/core/RepositoryFactory.cpp | Adds clearer error log when LMDB extension is missing. |
| libminifi/src/Configuration.cpp | Registers LMDB max DB size property validator. |
| extensions/rocksdb-repos/tests/ContentSessionTests.cpp | Renames member and fixes destructor override for test controller. |
| extensions/lmdb/tests/LmdbStreamTests.cpp | Adds unit tests for LmdbStream read/write/commit behavior. |
| extensions/lmdb/tests/LmdbContentSessionTests.cpp | Adds session semantics tests for LMDB content repository. |
| extensions/lmdb/tests/LmdbContentRepositoryTests.cpp | Adds repository init/exists/read/remove/orphan tests for LMDB. |
| extensions/lmdb/tests/CMakeLists.txt | Adds CMake rules to build/register LMDB unit tests. |
| extensions/lmdb/LmdbStream.h | Introduces LMDB-backed stream abstraction. |
| extensions/lmdb/LmdbStream.cpp | Implements LMDB stream read/write/commit logic. |
| extensions/lmdb/LmdbContentRepository.h | Declares LMDB-backed ContentRepository implementation and session type. |
| extensions/lmdb/LmdbContentRepository.cpp | Implements LMDB-backed content repository lifecycle, GC/orphaning, stats. |
| extensions/lmdb/CMakeLists.txt | Adds extension build and registration for minifi-lmdb. |
| CONFIGURE.md | Documents LMDB content repository option and caveats. |
| conf/minifi.properties.in | Adds commented LMDB max DB size property template. |
| CMakeLists.txt | Adds minifi-lmdb to default enabled extensions list. |
| cmake/MiNiFiOptions.cmake | Adds ENABLE_LMDB option. |
| cmake/LMDB.cmake | FetchContent integration for building LMDB and applying local patches. |
| .github/workflows/ci.yml | Enables LMDB in CI build matrices. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (lmdb_env_) { | ||
| mdb_dbi_close(lmdb_env_, lmdb_handle_); | ||
| mdb_env_close(lmdb_env_); | ||
| } | ||
| } |
There was a problem hiding this comment.
In the name of the single responsibility principle, I would extract the lifetime management of LMDB handles to a separate type. LmdbContentRepository should have one responsibility: implement a minifi c++ content repository in terms of LMDB. We can use helper types and functions to extract individual steps, and have this class only have the minimum necessary complexity.
There was a problem hiding this comment.
Agreed, this was extracted in the LmdbWrapper class in #2203 As it would be a bit complicated changing both PRs to move that here and we will probably handle and merge both PRs close together or at least in the same release I would keep that change in the follow up PR for now.
92f698e to
3653c3b
Compare
| add_minifi_option(ENABLE_ALL "Enables all extensions" OFF) | ||
| add_minifi_option(ENABLE_CIVET "Enables CivetWeb components." ON) | ||
| add_minifi_option(ENABLE_ROCKSDB "Enables the RocksDB extension." ON) | ||
| add_minifi_option(ENABLE_LMDB "Enables the LMDB extension." ON) |
There was a problem hiding this comment.
Could we name this MINIFI_LMBD instead? or anything else but prefixed with MINIFI?
There was a problem hiding this comment.
Renamed it in aa0f261
The only problem was that the bootstrap script checked for the ENABLE_ prefix to choose the extension options. I tweaked it a bit to move MINIFI_LMDB and MINIFI_RUST to the extension options and AWS_ENABLE_UNITY_BUILD to the build options.
There was a problem hiding this comment.
(not in this PR) but maybe we could do MINIFI_BUILD_ and MINIFI_EXT_ prefixes?
There was a problem hiding this comment.
Yes, that sounds good
martinzink
left a comment
martinzink
left a comment
There was a problem hiding this comment.
LGTM, only nitpick is the MinifiOption naming
151f90d to
aa0f261
Compare
szaszm
left a comment
szaszm
left a comment
There was a problem hiding this comment.
I'm not sure LMDB-based repositories are a good direction long term. In theory it works better than RocksDB in every aspect for our use case, but I want to evaluate other repository implementation options before releasing this and committing to supporting it.
We can merge this later to keep the code in sync and avoid merge conflicts until we finish the evaluation of other options, but please mark it as experimental in the docs, and make it disabled by default.
aa0f261 to
b045aa6
Compare
Updated in b045aa6 |
https://issues.apache.org/jira/browse/MINIFICPP-2849
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit?
For code changes:
For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.