MINIFICPP-2849 Implement LMDB based content repository by lordgamez · Pull Request #2201 · apache/nifi-minifi-cpp

lordgamez · 2026-06-18T12:05:08Z

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:

If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE file?
If applicable, have you updated the NOTICE file?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?

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.

lordgamez · 2026-06-18T12:11:30Z

+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);


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).

szaszm · 2026-06-18T13:41:42Z

+
+if (WIN32)
+    get_directory_property(MINIFI_SAVED_COMPILE_DEFS COMPILE_DEFINITIONS)
+    remove_definitions(-DWIN32_LEAN_AND_MEAN)


can you explain why you did this?

LMDB fails to compile on Windows when WIN32_LEAN_AND_MEAN is defined, which is automatically added to the compile definitions in CMakeLists.txt so it is included in all thirdparties used with FetchContent, so it needs to be removed separately for LMDB on Windows.

I'd like to make absolutely sure that we only undefine WIN32_LEAN_AND_MEAN while compiling LMDB, or find another workaround. If you look up what it does, it prevents windows.h from including a bunch of additional headers that usually end up unused, and may end up conflicting with other headers, while also increasing compile times. Alternatively, we could look up the transitively included headers that LMDB relies on, and include them explicitly with target_compile_options(target PRIVATE -include foo.h)

Ideally Microsoft would've made the lightweight header the default, with the option to opt in to additional features, but for historical reasons, the heavyweight header is the default, and you can opt out of the extra features. (bloat)

When I checked this according to my understanding this change should only impact the lmdb subdirectory, so it should not impact any other compilation. Anyway I can still check if there is another workaround.

There were 2 issues:

Defining WIN32_LEAN_AND_MEAN removed winternl.h needed for NTSTATUS usage, that needed to be added explicitly

After including winternl.h the function pointer names in mdb.c clashed with the Windows symbols, those needed to be renamed

Added patch in: 52d93b2

Copilot

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-lmdb extension implementing LmdbContentRepository and LmdbStream, plus LMDB-focused unit tests.
Wires LMDB into configuration (new nifi.content.repository.lmdb.max.db.size property) 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.

szaszm · 2026-08-03T12:20:31Z

+  explicit LmdbContentRepository(std::string_view name = className<LmdbContentRepository>(), const utils::Identifier& uuid = {})
+      : core::ContentRepositoryImpl(name, uuid) {}
+
+  ~LmdbContentRepository() override {


The class should follow the rule of 5. Disable copy and move?
Destructors should be noexcept when possible.

Suggested change

~LmdbContentRepository() override {

LmdbContentRepository(const LmdbContentRepository&) = delete;

LmdbContentRepository(LmdbContentRepository&&) = delete;

LmdbContentRepository& operator=(const LmdbContentRepository&) = delete;

LmdbContentRepository& operator=(LmdbContentRepository&&) = delete;

~LmdbContentRepository() noexcept override {

Good point, updated in f0f3414

szaszm · 2026-08-03T12:24:27Z

+      : core::ContentRepositoryImpl(name, uuid) {}
+
+  ~LmdbContentRepository() override {
+    stop();


Don't call virtual functions of the same class from constructors and destructors. Qualify the function name to clarify what gets called.

Suggested change

stop();

LmdbContentRepository::stop();

Updated in f0f3414

szaszm · 2026-08-03T12:28:20Z

+    if (lmdb_env_) {
+      mdb_dbi_close(lmdb_env_, lmdb_handle_);
+      mdb_env_close(lmdb_env_);
+    }
+  }


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.

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.

szaszm · 2026-08-03T12:28:52Z

+
+  std::shared_ptr<ContentSession> createSession() override;
+  bool initialize(const std::shared_ptr<minifi::Configure>& configuration) override;
+  std::shared_ptr<io::BaseStream> write(const minifi::ResourceClaim& claim, bool append = false) override;


Don't use default arguments on virtual functions

Good point, updated to use explicit parameter for all repository writes removing default value in f0f3414

szaszm · 2026-08-03T12:35:05Z

+LmdbContentRepository::Session::Session(std::shared_ptr<ContentRepository> repository) : BufferedContentSession(std::move(repository)) {}
+
+void LmdbContentRepository::Session::commit() {
+  auto lmdb_content_repository = std::dynamic_pointer_cast<LmdbContentRepository>(repository_);
+  if (!lmdb_content_repository) { throw Exception(REPOSITORY_EXCEPTION, "Session's repository is not an LmdbContentRepository"); }


If we require LmdbContentRepository, we should say so in the constructor signature

Suggested change

LmdbContentRepository::Session::Session(std::shared_ptr<ContentRepository> repository) : BufferedContentSession(std::move(repository)) {}

void LmdbContentRepository::Session::commit() {

auto lmdb_content_repository = std::dynamic_pointer_cast<LmdbContentRepository>(repository_);

if (!lmdb_content_repository) { throw Exception(REPOSITORY_EXCEPTION, "Session's repository is not an LmdbContentRepository"); }

LmdbContentRepository::Session::Session(std::shared_ptr<LmdbContentRepository> repository) : BufferedContentSession(std::move(repository)) {}

void LmdbContentRepository::Session::commit() {

auto lmdb_content_repository = std::dynamic_pointer_cast<LmdbContentRepository>(repository_);

if (!lmdb_content_repository) { throw Exception(REPOSITORY_EXCEPTION, "Session's repository is not an LmdbContentRepository"); }

Updated in f0f3414

szaszm · 2026-08-03T12:35:23Z

+
+  class Session : public BufferedContentSession {
+   public:
+    explicit Session(std::shared_ptr<ContentRepository> repository);


Suggested change

explicit Session(std::shared_ptr<ContentRepository> repository);

explicit Session(std::shared_ptr<LmdbContentRepository> repository);

Updated in f0f3414

szaszm · 2026-08-03T16:23:42Z

+
+TEST_CASE("LmdbContentRepository::Session commit throws when underlying repository is not LmdbContentRepository", "[lmdb]") {
+  auto unrelated_repository = std::make_shared<core::repository::VolatileContentRepository>();
+  unrelated_repository->initialize(std::make_shared<ConfigureImpl>());
+
+  core::repository::LmdbContentRepository::Session session(unrelated_repository);
+  REQUIRE_THROWS_WITH(session.commit(), Catch::Matchers::ContainsSubstring("Session's repository is not an LmdbContentRepository"));
+}


If Session is changed to only accept LmdbContentRepository, then this will no longer compile, therefore no runtime testing is necessary or possible.

Suggested change

TEST_CASE("LmdbContentRepository::Session commit throws when underlying repository is not LmdbContentRepository", "[lmdb]") {

auto unrelated_repository = std::make_shared<core::repository::VolatileContentRepository>();

unrelated_repository->initialize(std::make_shared<ConfigureImpl>());

core::repository::LmdbContentRepository::Session session(unrelated_repository);

REQUIRE_THROWS_WITH(session.commit(), Catch::Matchers::ContainsSubstring("Session's repository is not an LmdbContentRepository"));

}

Removed in f0f3414

szaszm · 2026-08-03T16:24:18Z

+void LmdbContentRepository::stop() {}
+
+std::shared_ptr<ContentSession> LmdbContentRepository::createSession() {
+  return std::make_shared<Session>(sharedFromThis<ContentRepository>());


Suggested change

return std::make_shared<Session>(sharedFromThis<ContentRepository>());

return std::make_shared<Session>(sharedFromThis<LmdbContentRepository>());

Updated in f0f3414

szaszm · 2026-08-03T16:29:22Z

+    return parsing::parseDataSize(max_db_size_str) | utils::orThrow(fmt::format("{} was set to invalid value: '{}'", Configure::nifi_content_repository_lmdb_max_db_size, max_db_size_str));
+  }) | utils::orElse([] {
+    // Default to 10 GB if the property is not set
+    return std::make_optional<uint64_t>(10ULL * 1024 * 1024 * 1024);


from Literals.h

Suggested change

return std::make_optional<uint64_t>(10ULL * 1024 * 1024 * 1024);

return std::make_optional<uint64_t>(10_GiB);

Good idea, updated in f0f3414

martinzink · 2026-08-12T15:57:32Z

 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)


Could we name this MINIFI_LMBD instead? or anything else but prefixed with MINIFI?

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.

(not in this PR) but maybe we could do MINIFI_BUILD_ and MINIFI_EXT_ prefixes?

Yes, that sounds good

martinzink

LGTM, only nitpick is the MinifiOption naming

lordgamez commented Jun 18, 2026

View reviewed changes

lordgamez marked this pull request as ready for review June 22, 2026 09:54

szaszm reviewed Jun 22, 2026

View reviewed changes

martinzink requested a review from Copilot June 25, 2026 12:51

Copilot started reviewing on behalf of martinzink June 25, 2026 12:52 View session

Copilot AI reviewed Jun 25, 2026

View reviewed changes

fgerlits reviewed Jul 17, 2026

View reviewed changes

szaszm reviewed Aug 3, 2026

View reviewed changes

lordgamez added 6 commits August 10, 2026 17:55

MINIFICPP-2849 Implement LMDB based content repository

5246112

Fix lmdb lifetime issues

46cce20

Fix LMDB build on Windows with WIN32_LEAN_AND_MEAN defined

3195845

Review update

e0c2d8e

Review update

f0f3414

Update LMDB and add conan recipe

3653c3b

lordgamez force-pushed the MINIFICPP-2849 branch from 92f698e to 3653c3b Compare August 11, 2026 10:18

fgerlits approved these changes Aug 11, 2026

View reviewed changes

martinzink reviewed Aug 12, 2026

View reviewed changes

martinzink approved these changes Aug 12, 2026

View reviewed changes

Replace ENABLE_LMDB option with MINIFI_LMDB

aa0f261

lordgamez force-pushed the MINIFICPP-2849 branch from 151f90d to aa0f261 Compare August 13, 2026 12:16

-  ~LmdbContentRepository() override {
+  LmdbContentRepository(const LmdbContentRepository&) = delete;
+  LmdbContentRepository(LmdbContentRepository&&) = delete;
+  LmdbContentRepository& operator=(const LmdbContentRepository&) = delete;
+  LmdbContentRepository& operator=(LmdbContentRepository&&) = delete;
+  ~LmdbContentRepository() noexcept override {

	explicit Session(std::shared_ptr<ContentRepository> repository);
	explicit Session(std::shared_ptr<LmdbContentRepository> repository);

	return std::make_shared<Session>(sharedFromThis<ContentRepository>());
	return std::make_shared<Session>(sharedFromThis<LmdbContentRepository>());

	return std::make_optional<uint64_t>(10ULL * 1024 * 1024 * 1024);
	return std::make_optional<uint64_t>(10_GiB);

Uh oh!

Conversation

lordgamez commented Jun 18, 2026

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szaszm Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szaszm Jun 22, 2026 •

edited

Loading

lordgamez Aug 13, 2026 •

edited

Loading