MINIFICPP-2849 Implement LMDB based content repository

[lordgamez]

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]

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

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

[martinzink]

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

[lordgamez]

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.

[martinzink]

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

[lordgamez]

Yes, that sounds good

[martinzink]

LGTM, only nitpick is the MinifiOption naming

[szaszm]

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.

[lordgamez]

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.

Updated in b045aa6

[szaszm]

why is this necessary or even working?

[lordgamez]

The winternl.h include was needed with newer MSVC version for the NTSTATUS symbol, but that caused a conflict of the existing Windows types with the custom Nt* typedefs in this file so they needed to be renamed.

[szaszm]

AI review

1. The append parameter to write() is silently ignored

LmdbStream always loads the existing value for the key (loadValue()), and subsequent write() calls always append new bytes to value_ regardless of the caller's intent. This happens to work today because BufferedContentSession guarantees a brand-new (empty) key for every write()/overwrite path and only sets is_append=true when genuine appending is intended — so the two code paths currently converge on the same correct behavior. However, this is fragile:

If a resource-claim path is ever reused while a stale LMDB entry exists (e.g., a failed removeKey() that ended up on purge_list_ and hasn't been retried yet, followed by path reuse), a "new" write would silently concatenate onto leftover bytes instead of overwriting — silent data corruption rather than a clear failure.
It also means the append flag on the interface is effectively decorative for this backend, which is misleading to future maintainers and out of step with the RocksDB/FileSystem implementations that do act on it.

Suggest either truncating/deleting the existing value explicitly when append == false (defensive), or at least asserting/logging if a "new" write ever encounters a pre-existing key.

[szaszm]

I'm only sharing sloppypasta because I think this could be a genuine concern, and the suggested solution, disabling / asserting on the unsupported path, would be an improvement.

[lordgamez]

I think here it was just using the same design as RocksDB where when creating the RocksDBStream was also ignoring the append argument on write. I think here we can handle the case and add a warning when we overwrite an existing value as it is unusual, but can happen in the aforementioned erroneous case. I'm still on the edge of either it should be a warning or an assertion, as if that happens we can still proceed, or could be an intentional use case of the LmdbStream so I left it as a warning for now. Updated in 3512381

[szaszm]

AI review
2. getRepositorySize() likely under-reports actual footprint
C++

uint64_t LmdbContentRepository::getRepositorySize() const {
const auto stat = getDbStat();
return stat.ms_psize * (stat.ms_branch_pages + stat.ms_leaf_pages + stat.ms_overflow_pages);
}

This reflects only live/used B-tree pages, not the actual on-disk/mapped file size. Given the documentation explicitly calls out that "the database file never shrinks" as a caveat, users querying repository size after deletions will see a number that doesn't match the real file footprint they were warned about. Consider deriving this from the mapped file size (e.g., via mdb_env_info/mdb_env_stat combined with actual file size) if the intent is to reflect disk usage.

Human comment
Consider adding a new metric for lmdb repositories only for live vs full size.

[lordgamez]

I changed it to use the filesystem size for now on in 3512381 as non of the statistics given by LMDB really reflect the actual disk footprint that is the most useful for us. I would keep that for now and we can think of implementing other useful stats later after a repository interface refactor. Currently only getRocksDbStats is available on the interface, so that should be changed to a generic custom repository stats to be able to implement repository specific statistics.