szaszm commented on code in PR #2201: URL: https://github.com/apache/nifi-minifi-cpp/pull/2201#discussion_r3843473381
########## thirdparty/lmdb/all/patches/fix-windows-symbols.patch: ########## Review Comment: why is this necessary or even working? ########## extensions/lmdb/LmdbContentRepository.cpp: ########## @@ -0,0 +1,299 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "LmdbContentRepository.h" + +#include <filesystem> +#include <iterator> +#include <memory> +#include <string> +#include <utility> +#include <vector> + +#include "LmdbStream.h" +#include "core/Resource.h" +#include "lmdb.h" +#include "minifi-cpp/Exception.h" +#include "minifi-cpp/utils/gsl.h" +#include "utils/Locations.h" +#include "minifi-cpp/utils/Literals.h" + +namespace org::apache::nifi::minifi::core::repository { + +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"); + } + + const auto writeResource = [&lmdb_content_repository](const std::shared_ptr<ResourceClaim>& resource_claim, const std::shared_ptr<io::BaseStream>& stream, bool is_append) { + auto outStream = lmdb_content_repository->write(*resource_claim, is_append); + if (outStream == nullptr) { + throw Exception(REPOSITORY_EXCEPTION, "Couldn't open the underlying resource for write: " + resource_claim->getContentFullPath()); + } + const auto size = stream->size(); + auto lmdb_out_stream = std::dynamic_pointer_cast<io::LmdbStream>(outStream); + if (lmdb_out_stream == nullptr) { + throw Exception(REPOSITORY_EXCEPTION, "Couldn't cast output stream to LmdbStream for commit: " + resource_claim->getContentFullPath()); + } + if (outStream->write(stream->getBuffer()) != size) { + throw Exception(REPOSITORY_EXCEPTION, "Failed to write " + std::string(is_append ? "appended" : "new") + " resource: " + resource_claim->getContentFullPath()); + } + if (!lmdb_out_stream->commit()) { + throw Exception(REPOSITORY_EXCEPTION, "Failed to commit " + std::string(is_append ? "appended" : "new") + " resource: " + resource_claim->getContentFullPath()); + } + }; + + for (const auto& resource : managed_resources_) { + writeResource(resource.first, resource.second, false); + } + + for (const auto& resource : append_state_) { + writeResource(resource.first, resource.second.stream, true); + } + + managed_resources_.clear(); + append_state_.clear(); +} + +bool LmdbContentRepository::initialize(const std::shared_ptr<minifi::Configure>& configuration) { + if (const int rc = mdb_env_create(&lmdb_env_); rc != MDB_SUCCESS) { + logger_->log_error("Failed to create LMDB environment: {}", mdb_strerror(rc)); + return false; + } + + // Reserve virtual address space for the DB file (max size it can grow to) + std::expected<uint64_t, std::error_code> max_db_size; + auto max_db_size_str_opt = configuration->get(Configure::nifi_content_repository_lmdb_max_db_size); + if (!max_db_size_str_opt || max_db_size_str_opt->empty()) { + max_db_size = 10_GiB; + } else { + max_db_size = parsing::parseDataSize(*max_db_size_str_opt); + } + + if (!max_db_size) { + logger_->log_error("Invalid max DB size configuration for LMDB Content Repository: {}", *max_db_size_str_opt); + mdb_env_close(lmdb_env_); + lmdb_env_ = nullptr; + return false; + } + + logger_->log_info("Setting LMDB max DB size to {} bytes", *max_db_size); + if (const auto rc = mdb_env_set_mapsize(lmdb_env_, gsl::narrow<size_t>(*max_db_size)); rc != MDB_SUCCESS) { + logger_->log_error("Failed to set LMDB map size: {}", mdb_strerror(rc)); + mdb_env_close(lmdb_env_); + lmdb_env_ = nullptr; + return false; + } + + const auto working_dir = utils::getMinifiDir(); + + std::string value; + if (configuration->get(Configure::nifi_dbcontent_repository_directory_default, value) && !value.empty()) { + directory_ = value; + } else { + directory_ = (working_dir / "lmdbcontentrepository").string(); + } + + if (std::filesystem::exists(directory_)) { + logger_->log_info("Using existing LMDB Content Repository directory at {}", directory_); + } else { + logger_->log_info("Creating LMDB Content Repository directory at {}", directory_); + if (!std::filesystem::create_directories(directory_)) { + logger_->log_error("Failed to create LMDB Content Repository directory at {}", directory_); + mdb_env_close(lmdb_env_); + lmdb_env_ = nullptr; + return false; + } + } + + if (const int rc = mdb_env_open(lmdb_env_, directory_.c_str(), MDB_NOTLS, 0664)) { + logger_->log_error("Failed to open LMDB environment: {}", mdb_strerror(rc)); + mdb_env_close(lmdb_env_); + lmdb_env_ = nullptr; + return false; + } + + MDB_txn* init_txn = nullptr; + if (const int rc = mdb_txn_begin(lmdb_env_, nullptr, 0, &init_txn); rc != MDB_SUCCESS) { + logger_->log_error("Failed to begin LMDB transaction during initialize: {}", mdb_strerror(rc)); + mdb_env_close(lmdb_env_); + lmdb_env_ = nullptr; + return false; + } + if (const int rc = mdb_dbi_open(init_txn, nullptr, 0, &lmdb_handle_); rc != MDB_SUCCESS) { + logger_->log_error("Failed to open LMDB database: {}", mdb_strerror(rc)); + mdb_txn_abort(init_txn); + mdb_env_close(lmdb_env_); + lmdb_env_ = nullptr; + return false; + } + + if (const int rc = mdb_txn_commit(init_txn); rc != MDB_SUCCESS) { + logger_->log_error("Failed to commit LMDB transaction during initialize: {}", mdb_strerror(rc)); + mdb_env_close(lmdb_env_); + lmdb_env_ = nullptr; + return false; + } + + return true; +} + +void LmdbContentRepository::start() {} +void LmdbContentRepository::stop() {} + +std::shared_ptr<ContentSession> LmdbContentRepository::createSession() { + return std::make_shared<Session>(sharedFromThis<LmdbContentRepository>()); +} + +std::shared_ptr<io::BaseStream> LmdbContentRepository::write(const minifi::ResourceClaim& claim, bool) { + return std::make_shared<io::LmdbStream>(claim.getContentFullPath(), lmdb_env_, &lmdb_handle_, true); +} + +std::shared_ptr<io::BaseStream> LmdbContentRepository::read(const minifi::ResourceClaim& claim) { + return std::make_shared<io::LmdbStream>(claim.getContentFullPath(), lmdb_env_, &lmdb_handle_, false); +} + +bool LmdbContentRepository::exists(const minifi::ResourceClaim& streamId) { + const auto path = streamId.getContentFullPath(); + MDB_val key{path.size(), const_cast<char*>(path.data())}; + MDB_val value{}; + + MDB_txn* txn = nullptr; + if (const int rc = mdb_txn_begin(lmdb_env_, nullptr, MDB_RDONLY, &txn); rc != MDB_SUCCESS) { + logger_->log_error("Failed to begin LMDB read transaction in exists: {}", mdb_strerror(rc)); + return false; + } + auto guard = gsl::finally([txn] { mdb_txn_abort(txn); }); + + const auto rc = mdb_get(txn, lmdb_handle_, &key, &value); + if (rc != MDB_SUCCESS && rc != MDB_NOTFOUND) { + logger_->log_error("Failed to get value from LMDB database: {}", mdb_strerror(rc)); + } + return rc == MDB_SUCCESS; +} + +bool LmdbContentRepository::removeKey(const std::string& content_path) { + MDB_val key{content_path.size(), const_cast<char*>(content_path.data())}; + + MDB_txn* txn = nullptr; + if (const int rc = mdb_txn_begin(lmdb_env_, nullptr, 0, &txn); rc != MDB_SUCCESS) { + logger_->log_error("Failed to begin LMDB write transaction in removeKey: {}", mdb_strerror(rc)); + return false; + } + int rc = mdb_del(txn, lmdb_handle_, &key, nullptr); + + if (rc == MDB_SUCCESS) { + if (const int rc = mdb_txn_commit(txn); rc != MDB_SUCCESS) { + logger_->log_error("Failed to commit LMDB transaction during delete: {}", mdb_strerror(rc)); + return false; + } + return true; + } else if (rc == MDB_NOTFOUND) { + logger_->log_debug("Key {} not found in LMDB database during delete", content_path); + mdb_txn_abort(txn); + return true; + } else { + logger_->log_error("Failed to delete key '{}' from LMDB database: {}", content_path, mdb_strerror(rc)); + mdb_txn_abort(txn); + return false; + } +} + +void LmdbContentRepository::clearOrphans() { + std::vector<std::string> keys_to_be_deleted; + + { + MDB_txn* txn = nullptr; + if (const int rc = mdb_txn_begin(lmdb_env_, nullptr, MDB_RDONLY, &txn); rc != MDB_SUCCESS) { + logger_->log_error("Failed to begin LMDB read transaction in clearOrphans: {}", mdb_strerror(rc)); + return; + } + auto txn_guard = gsl::finally([txn] { mdb_txn_abort(txn); }); + + MDB_cursor* cursor = nullptr; + if (const int rc = mdb_cursor_open(txn, lmdb_handle_, &cursor); rc != MDB_SUCCESS) { + logger_->log_error("Failed to open LMDB cursor in clearOrphans: {}", mdb_strerror(rc)); + return; + } + auto cursor_guard = gsl::finally([cursor] { mdb_cursor_close(cursor); }); + + MDB_val key{}; + MDB_val val{}; + int rc = mdb_cursor_get(cursor, &key, &val, MDB_FIRST); + + { + std::lock_guard<std::mutex> lock(count_map_mutex_); + while (rc == MDB_SUCCESS) { + std::string key_string = std::string(static_cast<char*>(key.mv_data), key.mv_size); + + auto claim_it = count_map_.find(key_string); + if (claim_it == count_map_.end() || claim_it->second == 0) { + logger_->log_debug("Deleting orphan resource {}", key_string); + keys_to_be_deleted.push_back(key_string); + } + rc = mdb_cursor_get(cursor, &key, &val, MDB_NEXT); + } + } + + if (rc != MDB_NOTFOUND) { + logger_->log_error("Failed to iterate over LMDB database: {}", mdb_strerror(rc)); + return; + } + } + + std::vector<std::string> failed_deletions; + for (const auto& key : keys_to_be_deleted) { + auto delete_result = removeKey(key); + if (!delete_result) { + logger_->log_warn("Failed to delete orphan resource {} from LMDB database", key); + failed_deletions.push_back(key); + } + } + + std::lock_guard<std::mutex> lock(purge_list_mutex_); + purge_list_.insert(purge_list_.end(), std::make_move_iterator(failed_deletions.begin()), std::make_move_iterator(failed_deletions.end())); +} + +MDB_stat LmdbContentRepository::getDbStat() const { + MDB_stat stat{}; + MDB_txn* txn = nullptr; + if (const int rc = mdb_txn_begin(lmdb_env_, nullptr, MDB_RDONLY, &txn); rc != MDB_SUCCESS) { + logger_->log_error("Failed to begin LMDB read transaction in getDbStat: {}", mdb_strerror(rc)); + return stat; + } + if (const int rc = mdb_stat(txn, lmdb_handle_, &stat); rc != MDB_SUCCESS) { + logger_->log_error("Failed to read LMDB database stats: {}", mdb_strerror(rc)); + } + mdb_txn_abort(txn); + return stat; +} + +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); +} Review Comment: 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. ########## extensions/lmdb/LmdbContentRepository.cpp: ########## @@ -0,0 +1,299 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "LmdbContentRepository.h" + +#include <filesystem> +#include <iterator> +#include <memory> +#include <string> +#include <utility> +#include <vector> + +#include "LmdbStream.h" +#include "core/Resource.h" +#include "lmdb.h" +#include "minifi-cpp/Exception.h" +#include "minifi-cpp/utils/gsl.h" +#include "utils/Locations.h" +#include "minifi-cpp/utils/Literals.h" + +namespace org::apache::nifi::minifi::core::repository { + +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"); + } + + const auto writeResource = [&lmdb_content_repository](const std::shared_ptr<ResourceClaim>& resource_claim, const std::shared_ptr<io::BaseStream>& stream, bool is_append) { + auto outStream = lmdb_content_repository->write(*resource_claim, is_append); + if (outStream == nullptr) { + throw Exception(REPOSITORY_EXCEPTION, "Couldn't open the underlying resource for write: " + resource_claim->getContentFullPath()); + } + const auto size = stream->size(); + auto lmdb_out_stream = std::dynamic_pointer_cast<io::LmdbStream>(outStream); + if (lmdb_out_stream == nullptr) { + throw Exception(REPOSITORY_EXCEPTION, "Couldn't cast output stream to LmdbStream for commit: " + resource_claim->getContentFullPath()); + } + if (outStream->write(stream->getBuffer()) != size) { + throw Exception(REPOSITORY_EXCEPTION, "Failed to write " + std::string(is_append ? "appended" : "new") + " resource: " + resource_claim->getContentFullPath()); + } + if (!lmdb_out_stream->commit()) { + throw Exception(REPOSITORY_EXCEPTION, "Failed to commit " + std::string(is_append ? "appended" : "new") + " resource: " + resource_claim->getContentFullPath()); + } + }; + + for (const auto& resource : managed_resources_) { + writeResource(resource.first, resource.second, false); + } + + for (const auto& resource : append_state_) { + writeResource(resource.first, resource.second.stream, true); + } + + managed_resources_.clear(); + append_state_.clear(); +} + +bool LmdbContentRepository::initialize(const std::shared_ptr<minifi::Configure>& configuration) { + if (const int rc = mdb_env_create(&lmdb_env_); rc != MDB_SUCCESS) { + logger_->log_error("Failed to create LMDB environment: {}", mdb_strerror(rc)); + return false; + } + + // Reserve virtual address space for the DB file (max size it can grow to) + std::expected<uint64_t, std::error_code> max_db_size; + auto max_db_size_str_opt = configuration->get(Configure::nifi_content_repository_lmdb_max_db_size); + if (!max_db_size_str_opt || max_db_size_str_opt->empty()) { + max_db_size = 10_GiB; + } else { + max_db_size = parsing::parseDataSize(*max_db_size_str_opt); + } + + if (!max_db_size) { + logger_->log_error("Invalid max DB size configuration for LMDB Content Repository: {}", *max_db_size_str_opt); + mdb_env_close(lmdb_env_); + lmdb_env_ = nullptr; + return false; + } + + logger_->log_info("Setting LMDB max DB size to {} bytes", *max_db_size); + if (const auto rc = mdb_env_set_mapsize(lmdb_env_, gsl::narrow<size_t>(*max_db_size)); rc != MDB_SUCCESS) { + logger_->log_error("Failed to set LMDB map size: {}", mdb_strerror(rc)); + mdb_env_close(lmdb_env_); + lmdb_env_ = nullptr; + return false; + } + + const auto working_dir = utils::getMinifiDir(); + + std::string value; + if (configuration->get(Configure::nifi_dbcontent_repository_directory_default, value) && !value.empty()) { + directory_ = value; + } else { + directory_ = (working_dir / "lmdbcontentrepository").string(); + } + + if (std::filesystem::exists(directory_)) { + logger_->log_info("Using existing LMDB Content Repository directory at {}", directory_); + } else { + logger_->log_info("Creating LMDB Content Repository directory at {}", directory_); + if (!std::filesystem::create_directories(directory_)) { + logger_->log_error("Failed to create LMDB Content Repository directory at {}", directory_); + mdb_env_close(lmdb_env_); + lmdb_env_ = nullptr; + return false; + } + } + + if (const int rc = mdb_env_open(lmdb_env_, directory_.c_str(), MDB_NOTLS, 0664)) { + logger_->log_error("Failed to open LMDB environment: {}", mdb_strerror(rc)); + mdb_env_close(lmdb_env_); + lmdb_env_ = nullptr; + return false; + } + + MDB_txn* init_txn = nullptr; + if (const int rc = mdb_txn_begin(lmdb_env_, nullptr, 0, &init_txn); rc != MDB_SUCCESS) { + logger_->log_error("Failed to begin LMDB transaction during initialize: {}", mdb_strerror(rc)); + mdb_env_close(lmdb_env_); + lmdb_env_ = nullptr; + return false; + } + if (const int rc = mdb_dbi_open(init_txn, nullptr, 0, &lmdb_handle_); rc != MDB_SUCCESS) { + logger_->log_error("Failed to open LMDB database: {}", mdb_strerror(rc)); + mdb_txn_abort(init_txn); + mdb_env_close(lmdb_env_); + lmdb_env_ = nullptr; + return false; + } + + if (const int rc = mdb_txn_commit(init_txn); rc != MDB_SUCCESS) { + logger_->log_error("Failed to commit LMDB transaction during initialize: {}", mdb_strerror(rc)); + mdb_env_close(lmdb_env_); + lmdb_env_ = nullptr; + return false; + } + + return true; +} + +void LmdbContentRepository::start() {} +void LmdbContentRepository::stop() {} + +std::shared_ptr<ContentSession> LmdbContentRepository::createSession() { + return std::make_shared<Session>(sharedFromThis<LmdbContentRepository>()); +} + +std::shared_ptr<io::BaseStream> LmdbContentRepository::write(const minifi::ResourceClaim& claim, bool) { Review Comment: 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
