fgerlits commented on code in PR #2201: URL: https://github.com/apache/nifi-minifi-cpp/pull/2201#discussion_r3601782840
########## extensions/lmdb/LmdbContentRepository.cpp: ########## @@ -0,0 +1,288 @@ +/** + * 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" + +namespace org::apache::nifi::minifi::core::repository { + +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"); } + + 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(); + if (outStream->write(stream->getBuffer()) != size) { + throw Exception(REPOSITORY_EXCEPTION, "Failed to write " + std::string(is_append ? "appended" : "new") + " resource: " + resource_claim->getContentFullPath()); + } + 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()); } Review Comment: should we check this before the `write()`? ########## extensions/lmdb/tests/LmdbContentSessionTests.cpp: ########## @@ -0,0 +1,301 @@ +/** + * + * 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 <array> +#include <memory> +#include <string> + +#include "LmdbContentRepository.h" +#include "catch2/matchers/catch_matchers_string.hpp" +#include "core/repository/VolatileContentRepository.h" +#include "unit/Catch.h" +#include "unit/TestBase.h" +#include "utils/ConfigurationUtils.h" + +namespace org::apache::nifi::minifi::test { + +class LmdbContentSessionController : public TestController { + public: + LmdbContentSessionController() : content_repository_(std::make_shared<core::repository::LmdbContentRepository>()) { + auto content_repo_path = createTempDirectory(); + auto config = std::make_shared<ConfigureImpl>(); + config->set(Configure::nifi_dbcontent_repository_directory_default, content_repo_path.string()); + content_repository_->initialize(config); + } + + ~LmdbContentSessionController() override { log.reset(); } + + LmdbContentSessionController(LmdbContentSessionController&&) = delete; + LmdbContentSessionController(const LmdbContentSessionController&) = delete; + LmdbContentSessionController& operator=(LmdbContentSessionController&&) = delete; + LmdbContentSessionController& operator=(const LmdbContentSessionController&) = delete; + + std::shared_ptr<core::ContentRepository> content_repository_; +}; + +namespace { + +std::shared_ptr<io::OutputStream> operator<<(std::shared_ptr<io::OutputStream> stream, const std::string& str) { Review Comment: nitpick, but `stream` should be `const &` (also in `>>`) ########## extensions/lmdb/tests/LmdbStreamTests.cpp: ########## @@ -0,0 +1,273 @@ +/** + * + * 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 "LmdbStream.h" +#include "lmdb.h" +#include "unit/Catch.h" +#include "unit/TestBase.h" + +namespace org::apache::nifi::minifi::test { + +class LmdbStreamTest : TestController { + public: + LmdbStreamTest() : db_path_(createTempDirectory().string()) { + if (const int rc = mdb_env_create(&lmdb_env_)) { + throw std::runtime_error("Failed to create LMDB environment: " + std::string(mdb_strerror(rc))); + } + if (const int rc = mdb_env_set_mapsize(lmdb_env_, 100ULL * 1024 * 1024); rc != MDB_SUCCESS) { + throw std::runtime_error("Failed to set LMDB map size: " + std::string(mdb_strerror(rc))); + } + + if (const int rc = mdb_env_open(lmdb_env_, db_path_.c_str(), MDB_NOTLS, 0664)) { + throw std::runtime_error("Failed to open LMDB environment " + db_path_ + ": " + std::string(mdb_strerror(rc))); + } Review Comment: I would add `; rc != MDB_SUCCESS` in lines 29 and 36, too ########## extensions/lmdb/LmdbStream.h: ########## @@ -0,0 +1,66 @@ +/** + * + * 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. + */ +#pragma once + +#include <cstdint> +#include <memory> +#include <string> + +#include "core/logging/LoggerFactory.h" +#include "io/BaseStream.h" +#include "lmdb.h" + +namespace org::apache::nifi::minifi::io { + +class LmdbStream : public io::BaseStreamImpl { + public: + explicit LmdbStream(std::string path, MDB_env* lmdb_env, MDB_dbi* lmdb_handle, bool write_enable = false); Review Comment: I can't see any place where we use the default, and it's better not to have it, so I would remove it: ```suggestion explicit LmdbStream(std::string path, MDB_env* lmdb_env, MDB_dbi* lmdb_handle, bool write_enable); ``` ########## extensions/lmdb/LmdbStream.cpp: ########## @@ -0,0 +1,119 @@ +/** + * + * 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 "LmdbStream.h" + +#include <cstring> +#include <algorithm> +#include <string> +#include <utility> + +#include "io/validation.h" + +namespace org::apache::nifi::minifi::io { + +LmdbStream::LmdbStream(std::string path, MDB_env* lmdb_env, MDB_dbi* lmdb_handle, bool write_enable) + : BaseStreamImpl(), + path_(std::move(path)), + write_enable_(write_enable), + lmdb_env_(lmdb_env), + lmdb_handle_(lmdb_handle), + exists_(loadValue()) {} + +bool LmdbStream::loadValue() { + 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 loadValue: {}", 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) { + value_ = std::string(static_cast<char*>(value.mv_data), value.mv_size); + return true; + } else if (rc != MDB_NOTFOUND) { + logger_->log_error("Failed to get value from LMDB database: {}", mdb_strerror(rc)); + } + return false; +} + +void LmdbStream::close() { + commit(); +} + +bool LmdbStream::commit() { + if (!write_enable_ || !dirty_) { return false; } + + MDB_txn* txn = nullptr; + auto rc = mdb_txn_begin(lmdb_env_, nullptr, 0, &txn); + if (rc != MDB_SUCCESS) { + logger_->log_error("Failed to begin LMDB transaction in close: {}", mdb_strerror(rc)); + return false; + } + + MDB_val key{path_.size(), const_cast<char*>(path_.data())}; + MDB_val val{value_.size(), const_cast<char*>(value_.data())}; + rc = mdb_put(txn, *lmdb_handle_, &key, &val, 0); + if (rc != MDB_SUCCESS) { + logger_->log_error("Failed to put value in LMDB database during close: {}", mdb_strerror(rc)); + mdb_txn_abort(txn); + return false; + } + + rc = mdb_txn_commit(txn); + if (rc != MDB_SUCCESS) { + logger_->log_error("Failed to commit LMDB transaction during close: {}", mdb_strerror(rc)); + return false; + } + + dirty_ = false; + return true; +} Review Comment: nitpick: the error logs should say "commit" instead of "close" ########## extensions/lmdb/LmdbContentRepository.cpp: ########## @@ -0,0 +1,288 @@ +/** + * 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" + +namespace org::apache::nifi::minifi::core::repository { + +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"); } + + 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(); + if (outStream->write(stream->getBuffer()) != size) { + throw Exception(REPOSITORY_EXCEPTION, "Failed to write " + std::string(is_append ? "appended" : "new") + " resource: " + resource_claim->getContentFullPath()); + } + 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 (!lmdb_out_stream->commit()) { throw Exception(REPOSITORY_EXCEPTION, "Failed to commit " + std::string(is_append ? "appended" : "new") + " resource: " + resource_claim->getContentFullPath()); } + }; Review Comment: Very minor nitpick, but these `throw Exception...` lines look too long to me for a single-line format. I would put all of them on separate lines as you did at lines 46-48. ########## extensions/lmdb/LmdbContentRepository.cpp: ########## @@ -0,0 +1,288 @@ +/** + * 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" + +namespace org::apache::nifi::minifi::core::repository { + +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"); } + + 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(); + if (outStream->write(stream->getBuffer()) != size) { + throw Exception(REPOSITORY_EXCEPTION, "Failed to write " + std::string(is_append ? "appended" : "new") + " resource: " + resource_claim->getContentFullPath()); + } + 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 (!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_)) { Review Comment: I would add the explicit comparison here: ```suggestion if (const int rc = mdb_env_create(&lmdb_env_); rc != MDB_SUCCESS) { ``` ########## extensions/lmdb/LmdbContentRepository.cpp: ########## @@ -0,0 +1,288 @@ +/** + * 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" + +namespace org::apache::nifi::minifi::core::repository { + +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"); } + + 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(); + if (outStream->write(stream->getBuffer()) != size) { + throw Exception(REPOSITORY_EXCEPTION, "Failed to write " + std::string(is_append ? "appended" : "new") + " resource: " + resource_claim->getContentFullPath()); + } + 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 (!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_)) { + 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) + const auto max_db_size = configuration->get(Configure::nifi_content_repository_lmdb_max_db_size) | utils::andThen([](auto max_db_size_str) -> std::optional<uint64_t> { + if (max_db_size_str.empty()) { return std::nullopt; } + 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); + }); + + if (!max_db_size) { + logger_->log_error("Invalid max DB size configuration for LMDB Content Repository"); + 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<ContentRepository>()); +} + +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); + + while (rc == MDB_SUCCESS) { + std::string key_string = std::string(static_cast<char*>(key.mv_data), key.mv_size); + + std::lock_guard<std::mutex> lock(count_map_mutex_); Review Comment: Should we lock the `count_map_mutex_` for the whole duration of the iteration? Allowing modifications to `count_map_` while we are checking the orphans makes it difficult to reason about what is happening. ########## extensions/lmdb/LmdbContentRepository.cpp: ########## @@ -0,0 +1,288 @@ +/** + * 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" + +namespace org::apache::nifi::minifi::core::repository { + +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"); } + + 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(); + if (outStream->write(stream->getBuffer()) != size) { + throw Exception(REPOSITORY_EXCEPTION, "Failed to write " + std::string(is_append ? "appended" : "new") + " resource: " + resource_claim->getContentFullPath()); + } + 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 (!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_)) { + 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) + const auto max_db_size = configuration->get(Configure::nifi_content_repository_lmdb_max_db_size) | utils::andThen([](auto max_db_size_str) -> std::optional<uint64_t> { + if (max_db_size_str.empty()) { return std::nullopt; } + 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); + }); + + if (!max_db_size) { + logger_->log_error("Invalid max DB size configuration for LMDB Content Repository"); + mdb_env_close(lmdb_env_); + lmdb_env_ = nullptr; + return false; + } Review Comment: I don't think `max_db_size` can be `nullopt` here: if the property was unset or empty, then we are using the default value, and if it is an invalid number, then we throw earlier. Maybe we should set `max_db_size` to `nullopt` instead of throwing? ########## extensions/lmdb/LmdbStream.cpp: ########## @@ -0,0 +1,119 @@ +/** + * + * 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 "LmdbStream.h" + +#include <cstring> +#include <algorithm> +#include <string> +#include <utility> + +#include "io/validation.h" + +namespace org::apache::nifi::minifi::io { + +LmdbStream::LmdbStream(std::string path, MDB_env* lmdb_env, MDB_dbi* lmdb_handle, bool write_enable) + : BaseStreamImpl(), + path_(std::move(path)), + write_enable_(write_enable), + lmdb_env_(lmdb_env), + lmdb_handle_(lmdb_handle), + exists_(loadValue()) {} + +bool LmdbStream::loadValue() { + 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 loadValue: {}", 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) { + value_ = std::string(static_cast<char*>(value.mv_data), value.mv_size); + return true; + } else if (rc != MDB_NOTFOUND) { + logger_->log_error("Failed to get value from LMDB database: {}", mdb_strerror(rc)); + } + return false; +} + +void LmdbStream::close() { + commit(); +} + +bool LmdbStream::commit() { + if (!write_enable_ || !dirty_) { return false; } + + MDB_txn* txn = nullptr; + auto rc = mdb_txn_begin(lmdb_env_, nullptr, 0, &txn); + if (rc != MDB_SUCCESS) { + logger_->log_error("Failed to begin LMDB transaction in close: {}", mdb_strerror(rc)); + return false; + } + + MDB_val key{path_.size(), const_cast<char*>(path_.data())}; + MDB_val val{value_.size(), const_cast<char*>(value_.data())}; + rc = mdb_put(txn, *lmdb_handle_, &key, &val, 0); + if (rc != MDB_SUCCESS) { + logger_->log_error("Failed to put value in LMDB database during close: {}", mdb_strerror(rc)); + mdb_txn_abort(txn); + return false; + } + + rc = mdb_txn_commit(txn); + if (rc != MDB_SUCCESS) { + logger_->log_error("Failed to commit LMDB transaction during close: {}", mdb_strerror(rc)); + return false; + } + + dirty_ = false; + return true; +} + +void LmdbStream::seek(size_t offset) { + offset_ = offset; +} + +size_t LmdbStream::tell() const { + return offset_; +} + +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; } Review Comment: `value` is just a pointer, so `IsNullOrEmpty` is just a null check; I would change this line to ```suggestion if (size != 0 && value == nullptr) { return STREAM_ERROR; } ``` to make this clearer -- 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]
