[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1516: MINIFICPP-2054 - Periodically run manual compaction
adamdebreceni commented on code in PR #1516:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1516#discussion_r1124392896
##
libminifi/include/utils/StoppableThread.h:
##
@@ -0,0 +1,60 @@
+/**
+ * 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
+#include
+#include
+#include
+#include
+
+namespace org::apache::nifi::minifi::utils {
+
+// mimics some aspects of std::jthread
+// unfortunately clang's jthread support is lacking
+// TODO(adebreceni): replace this with std::jthread
+class StoppableThread {
+ public:
+ explicit StoppableThread(std::function fn);
+
+ void stopWait() {
Review Comment:
renamed
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1516: MINIFICPP-2054 - Periodically run manual compaction
adamdebreceni commented on code in PR #1516:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1516#discussion_r1121933631
##
libminifi/include/properties/Configuration.h:
##
@@ -69,6 +69,11 @@ class Configuration : public Properties {
static constexpr const char *nifi_provenance_repository_directory_default =
"nifi.provenance.repository.directory.default";
static constexpr const char *nifi_flowfile_repository_directory_default =
"nifi.flowfile.repository.directory.default";
static constexpr const char *nifi_dbcontent_repository_directory_default =
"nifi.database.content.repository.directory.default";
+
+ // these are internal properties related to the rocksdb backend
+ static constexpr const char *nifi_flowfile_repository_compaction_period =
"nifi.flowfile.repository.compaction.period";
+ static constexpr const char *nifi_dbcontent_repository_compaction_period =
"nifi.database.content.repository.compaction.period";
Review Comment:
added to `CONFIGURATION_PROPERTIES ` and added documentation
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1516: MINIFICPP-2054 - Periodically run manual compaction
adamdebreceni commented on code in PR #1516:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1516#discussion_r1121875581
##
extensions/rocksdb-repos/FlowFileRepository.cpp:
##
@@ -200,6 +200,20 @@ bool FlowFileRepository::initialize(const
std::shared_ptr &configure)
}
logger_->log_debug("NiFi FlowFile Repository Directory %s", directory_);
+ compaction_period_ = DEFAULT_COMPACTION_PERIOD;
+ if (auto compaction_period_str =
configure->get(Configure::nifi_flowfile_repository_compaction_period)) {
+if (auto compaction_period =
TimePeriodValue::fromString(compaction_period_str.value())) {
+ compaction_period_ = compaction_period->getMilliseconds();
+ if (compaction_period_.count() == 0) {
+logger_->log_warn("Setting '%s' to 0 disables forced compaction",
Configure::nifi_dbcontent_repository_compaction_period);
+ }
+} else {
+ logger_->log_error("Malformed property '%s', expected time period, using
default", Configure::nifi_flowfile_repository_compaction_period);
+}
+ } else {
+logger_->log_debug("Using default compaction period of %" PRId64 " ms",
int64_t{compaction_period_.count()});
+ }
Review Comment:
done
##
libminifi/include/utils/StoppableThread.h:
##
@@ -0,0 +1,60 @@
+/**
+ * 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
+#include
+#include
+#include
+#include
+
+namespace org::apache::nifi::minifi::utils {
+
+// mimics some aspects of std::jthread
+// unfortunately clang's jthread support is lacking
+// TODO(adebreceni): replace this with std::jthread
+class StoppableThread {
+ public:
+ explicit StoppableThread(std::function fn);
+
+ void stopWait() {
+running_ = false;
+{
+ std::unique_lock lock(mtx_);
+ cv_.notify_all();
+}
+if (thread_.joinable()) {
+ thread_.join();
+}
+ }
+
+ ~StoppableThread() {
+stopWait();
+ }
+
+ // return true if stop was requested
+ static bool wait_stop_requested(std::chrono::milliseconds time);
Review Comment:
done
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1516: MINIFICPP-2054 - Periodically run manual compaction
adamdebreceni commented on code in PR #1516:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1516#discussion_r1121875405
##
extensions/rocksdb-repos/DatabaseContentRepository.h:
##
@@ -69,12 +72,23 @@ class DatabaseContentRepository : public
core::ContentRepository {
void clearOrphans() override;
+ void start() override;
+ void stop() override;
+
+ // test-only utility
+ void test_deinitialize();
Review Comment:
done
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1516: MINIFICPP-2054 - Periodically run manual compaction
adamdebreceni commented on code in PR #1516:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1516#discussion_r1121874993
##
extensions/rocksdb-repos/DatabaseContentRepository.cpp:
##
@@ -42,6 +43,21 @@ bool DatabaseContentRepository::initialize(const
std::shared_ptrgetHome()},
DbEncryptionOptions{directory_, ENCRYPTION_KEY_NAME});
logger_->log_info("Using %s DatabaseContentRepository", encrypted_env ?
"encrypted" : "plaintext");
+ compaction_period_ = DEFAULT_COMPACTION_PERIOD;
+ if (auto compaction_period_str =
configuration->get(Configure::nifi_dbcontent_repository_compaction_period)) {
+if (auto compaction_period =
TimePeriodValue::fromString(compaction_period_str.value())) {
+ compaction_period_ = compaction_period->getMilliseconds();
+ if (compaction_period_.count() == 0) {
+logger_->log_warn("Setting '%s' to 0 disables forced compaction",
Configure::nifi_dbcontent_repository_compaction_period);
+ }
+} else {
+ logger_->log_error("Malformed property '%s', expected time period, using
default", Configure::nifi_dbcontent_repository_compaction_period);
+}
+ } else {
+logger_->log_debug("Using default compaction period of %" PRId64 " ms",
int64_t{compaction_period_.count()});
+ }
+
Review Comment:
removed
##
extensions/rocksdb-repos/DatabaseContentRepository.cpp:
##
@@ -42,6 +43,21 @@ bool DatabaseContentRepository::initialize(const
std::shared_ptrgetHome()},
DbEncryptionOptions{directory_, ENCRYPTION_KEY_NAME});
logger_->log_info("Using %s DatabaseContentRepository", encrypted_env ?
"encrypted" : "plaintext");
+ compaction_period_ = DEFAULT_COMPACTION_PERIOD;
+ if (auto compaction_period_str =
configuration->get(Configure::nifi_dbcontent_repository_compaction_period)) {
+if (auto compaction_period =
TimePeriodValue::fromString(compaction_period_str.value())) {
+ compaction_period_ = compaction_period->getMilliseconds();
+ if (compaction_period_.count() == 0) {
+logger_->log_warn("Setting '%s' to 0 disables forced compaction",
Configure::nifi_dbcontent_repository_compaction_period);
+ }
+} else {
+ logger_->log_error("Malformed property '%s', expected time period, using
default", Configure::nifi_dbcontent_repository_compaction_period);
+}
+ } else {
+logger_->log_debug("Using default compaction period of %" PRId64 " ms",
int64_t{compaction_period_.count()});
+ }
Review Comment:
done
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1516: MINIFICPP-2054 - Periodically run manual compaction
adamdebreceni commented on code in PR #1516:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1516#discussion_r1121852830
##
libminifi/include/utils/StoppableThread.h:
##
@@ -0,0 +1,60 @@
+/**
+ * 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
+#include
+#include
+#include
+#include
+
+namespace org::apache::nifi::minifi::utils {
+
+// mimics some aspects of std::jthread
+// unfortunately clang's jthread support is lacking
+// TODO(adebreceni): replace this with std::jthread
+class StoppableThread {
+ public:
+ explicit StoppableThread(std::function fn);
+
+ void stopWait() {
Review Comment:
comparing this with std::jthread::request_stop, that does not wait for the
thread to terminate, I think it is important to emphasise that this waits
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1516: MINIFICPP-2054 - Periodically run manual compaction
adamdebreceni commented on code in PR #1516:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1516#discussion_r1121850110
##
libminifi/include/properties/Configuration.h:
##
@@ -69,6 +69,11 @@ class Configuration : public Properties {
static constexpr const char *nifi_provenance_repository_directory_default =
"nifi.provenance.repository.directory.default";
static constexpr const char *nifi_flowfile_repository_directory_default =
"nifi.flowfile.repository.directory.default";
static constexpr const char *nifi_dbcontent_repository_directory_default =
"nifi.database.content.repository.directory.default";
+
+ // these are internal properties related to the rocksdb backend
+ static constexpr const char *nifi_flowfile_repository_compaction_period =
"nifi.flowfile.repository.compaction.period";
+ static constexpr const char *nifi_dbcontent_repository_compaction_period =
"nifi.database.content.repository.compaction.period";
Review Comment:
I added them but then removed them, I don't feel that these should be used
by the user as it exposes internals of the repository, essentially it is a
workaround, not a standalone feature like compression
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1516: MINIFICPP-2054 - Periodically run manual compaction
adamdebreceni commented on code in PR #1516:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1516#discussion_r1121426450
##
extensions/rocksdb-repos/FlowFileRepository.cpp:
##
@@ -200,6 +200,20 @@ bool FlowFileRepository::initialize(const
std::shared_ptr &configure)
}
logger_->log_debug("NiFi FlowFile Repository Directory %s", directory_);
+ compaction_period_ = DEFAULT_COMPACTION_PERIOD;
+ if (auto compaction_period_str =
configure->get(Configure::nifi_flowfile_repository_compaction_period)) {
+if (auto compaction_period =
TimePeriodValue::fromString(compaction_period_str.value())) {
+ compaction_period_ = compaction_period->getMilliseconds();
+ if (compaction_period_.count() == 0) {
+logger_->log_warn("Setting '%s' to 0 disables forced compaction",
Configure::nifi_dbcontent_repository_compaction_period);
+ }
+} else {
+ logger_->log_error("Malformed property '%s', expected time period, using
default", Configure::nifi_flowfile_repository_compaction_period);
+}
+ } else {
+logger_->log_info("Using default compaction period");
Review Comment:
changed it in both repositories
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1516: MINIFICPP-2054 - Periodically run manual compaction
adamdebreceni commented on code in PR #1516:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1516#discussion_r1120015965
##
libminifi/test/rocksdb-tests/DBContentRepositoryTests.cpp:
##
@@ -159,7 +159,7 @@ TEST_CASE("Delete NonExistent Claim", "[TestDBCR4]") {
stream->close();
- content_repo->stop();
+ content_repo->deinitialize();
Review Comment:
we could drop the deinitialize (or make it private) and allow the tests to
access it
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1516: MINIFICPP-2054 - Periodically run manual compaction
adamdebreceni commented on code in PR #1516:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1516#discussion_r1120013803
##
libminifi/test/rocksdb-tests/DBContentRepositoryTests.cpp:
##
@@ -159,7 +159,7 @@ TEST_CASE("Delete NonExistent Claim", "[TestDBCR4]") {
stream->close();
- content_repo->stop();
+ content_repo->deinitialize();
Review Comment:
for the database content repository `stop` was a test-only feature, since it
had no background threads there wasn't
really a point in following in flowfilerepository's footsteps
now that it has a background thread its lifecycle should mimic the
flowfilerepository/provenancerepository's `ctor -> initialize -> start <->
stop -> dtor` pattern IMO
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1516: MINIFICPP-2054 - Periodically run manual compaction
adamdebreceni commented on code in PR #1516:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1516#discussion_r1119993446
##
extensions/rocksdb-repos/DatabaseContentRepository.cpp:
##
@@ -72,13 +87,40 @@ bool DatabaseContentRepository::initialize(const
std::shared_ptropen()) {
+ auto status = opendb->RunCompaction();
+ logger_->log_trace("Compaction triggered: %s", status.ToString());
+} else {
+ logger_->log_error("Failed to open database for compaction");
+}
+ } while (!utils::StoppableThread::wait_stop_requested(compaction_period_));
+}
+
+void DatabaseContentRepository::start() {
+ if (!db_ || !is_valid_) {
+return;
+ }
+ if (compaction_period_.count() != 0) {
+compaction_thread_ = std::make_unique([this] () {
+ runCompaction();
+});
+ }
+}
+
void DatabaseContentRepository::stop() {
if (db_) {
auto opendb = db_->open();
if (opendb) {
opendb->FlushWAL(true);
}
+compaction_thread_.reset();
Review Comment:
the thread's destructor called during reset block until it is joined
##
extensions/rocksdb-repos/DatabaseContentRepository.cpp:
##
@@ -72,13 +87,40 @@ bool DatabaseContentRepository::initialize(const
std::shared_ptropen()) {
+ auto status = opendb->RunCompaction();
+ logger_->log_trace("Compaction triggered: %s", status.ToString());
+} else {
+ logger_->log_error("Failed to open database for compaction");
+}
+ } while (!utils::StoppableThread::wait_stop_requested(compaction_period_));
+}
+
+void DatabaseContentRepository::start() {
+ if (!db_ || !is_valid_) {
+return;
+ }
+ if (compaction_period_.count() != 0) {
+compaction_thread_ = std::make_unique([this] () {
+ runCompaction();
+});
+ }
+}
+
void DatabaseContentRepository::stop() {
if (db_) {
auto opendb = db_->open();
if (opendb) {
opendb->FlushWAL(true);
}
+compaction_thread_.reset();
Review Comment:
the thread's destructor called during reset blocks until it is joined
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1516: MINIFICPP-2054 - Periodically run manual compaction
adamdebreceni commented on code in PR #1516:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1516#discussion_r1119992159
##
libminifi/test/rocksdb-tests/DBContentRepositoryTests.cpp:
##
@@ -159,7 +159,7 @@ TEST_CASE("Delete NonExistent Claim", "[TestDBCR4]") {
stream->close();
- content_repo->stop();
+ content_repo->deinitialize();
Review Comment:
the resource claim stores a shared_ptr to the content repository, "stop()"
previously put the repo into an "empty" state, so that a new repository
instance could open and manage the rocksdb backend, (during normal operations a
ResourceClaim should not outlive the content repo), but these tests depend on
being able to invalidate a previous instance and reusing an old claim, these
tests should be revised
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
