[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1490: MINIFICPP-2022 Add valid repository size metrics for all repositories
szaszm commented on code in PR #1490:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1490#discussion_r1171220635
##
libminifi/test/rocksdb-tests/RepoTests.cpp:
##
@@ -515,4 +519,221 @@ TEST_CASE("FlowFileRepository synchronously pushes
existing flow files") {
}
}
+TEST_CASE("Test getting flow file repository size properties",
"[TestGettingRepositorySize]") {
+
LogTestController::getInstance().setDebug();
+
LogTestController::getInstance().setDebug();
+
LogTestController::getInstance().setDebug();
+
LogTestController::getInstance().setDebug();
+ TestController testController;
+ auto dir = testController.createTempDirectory();
+
+ std::shared_ptr repository;
+ auto expected_is_full = false;
+ uint64_t expected_max_repo_size = 0;
+ SECTION("FlowFileRepository") {
+repository = std::make_shared("ff",
dir.string(), 0ms, 0, 1ms);
+ }
+
+ SECTION("ProvenanceRepository") {
+repository =
std::make_shared("ff", dir.string(),
0ms, 0, 1ms);
+ }
+
+ SECTION("VolatileFlowFileRepository") {
+repository =
std::make_shared("ff",
dir.string(), 0ms, 10, 1ms);
+expected_is_full = true;
+expected_max_repo_size = 7;
+ }
+
+ SECTION("VolatileProvenanceRepository") {
+repository =
std::make_shared("ff",
dir.string(), 0ms, 10, 1ms);
+expected_is_full = true;
+expected_max_repo_size = 7;
+ }
+ auto configuration = std::make_shared();
+ repository->initialize(configuration);
+
+ auto flow_file = std::make_shared();
+
+ for (auto i = 0; i < 100; ++i) {
+flow_file->addAttribute("key" + std::to_string(i), "testattributevalue" +
std::to_string(i));
+ }
+
+ auto original_size = repository->getRepositorySize();
+ using org::apache::nifi::minifi::utils::verifyEventHappenedInPollTime;
+ REQUIRE(verifyEventHappenedInPollTime(std::chrono::seconds(5),
[_size, ] {
+ auto old_size = original_size;
+ original_size = repository->getRepositorySize();
+ return old_size == original_size;
+},
+std::chrono::milliseconds(50)));
+ REQUIRE(true == flow_file->Persist(repository));
+ auto flow_file_2 = std::make_shared();
+ REQUIRE(true == flow_file_2->Persist(repository));
+
+ repository->flush();
+ repository->stop();
+
+ auto new_size = repository->getRepositorySize();
+ REQUIRE(verifyEventHappenedInPollTime(std::chrono::seconds(5), [_size,
] {
+ auto old_size = new_size;
+ new_size = repository->getRepositorySize();
+ return old_size == new_size;
+},
+std::chrono::milliseconds(50)));
+ REQUIRE(new_size > original_size);
+ REQUIRE(expected_is_full == repository->isFull());
+ REQUIRE(expected_max_repo_size == repository->getMaxRepositorySize());
+ REQUIRE(2 == repository->getRepositoryEntryCount());
+}
+
+TEST_CASE("Test getting noop repository size properties",
"[TestGettingRepositorySize]") {
+ TestController testController;
+ auto dir = testController.createTempDirectory();
+
+ auto repository = minifi::core::createRepository("NoOpRepository", "ff");
+
+ repository->initialize(std::make_shared());
+
+ auto flow_file = std::make_shared();
+
+ flow_file->addAttribute("key", "testattributevalue");
+
+ repository->flush();
+ repository->stop();
+
+ REQUIRE(repository->getRepositorySize() == 0);
+ REQUIRE(!repository->isFull());
+ REQUIRE(repository->getMaxRepositorySize() == 0);
+ REQUIRE(repository->getRepositoryEntryCount() == 0);
+}
+
+TEST_CASE("Test getting content repository size properties",
"[TestGettingRepositorySize]") {
+ LogTestController::getInstance().setDebug();
+
LogTestController::getInstance().setDebug();
+
LogTestController::getInstance().setDebug();
+
LogTestController::getInstance().setDebug();
+ TestController testController;
+ auto dir = testController.createTempDirectory();
+
+ auto repository =
std::make_shared("ff", dir.string(), 0ms,
0, 1ms);
+
+ auto content_repo_dir = testController.createTempDirectory();
+ auto configuration = std::make_shared();
+
configuration->set(minifi::Configure::nifi_dbcontent_repository_directory_default,
content_repo_dir.string());
+ std::string content = "content";
+
configuration->set(minifi::Configure::nifi_volatile_repository_options_content_max_bytes,
std::to_string(content.size()));
+
+ std::shared_ptr content_repo;
+ auto expected_is_full = false;
+ uint64_t expected_max_repo_size = 0;
+ SECTION("FileSystemRepository") {
+content_repo = std::make_shared();
+ }
+
+ SECTION("VolatileContentRepository") {
+content_repo =
std::make_shared("content");
+expected_is_full = true;
+expected_max_repo_size = content.size();
+ }
+
+ SECTION("DatabaseContentRepository") {
+content_repo =
std::make_shared();
+ }
+
+ content_repo->initialize(configuration);
+
+ repository->initialize(configuration);
+ repository->loadComponent(content_repo);
+ auto original_content_repo_size = content_repo->getRepositorySize();
+
+ auto flow_file = std::make_shared();
+
+ auto content_session
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1490: MINIFICPP-2022 Add valid repository size metrics for all repositories
szaszm commented on code in PR #1490:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1490#discussion_r1132545897
##
libminifi/test/unit/MetricsTests.cpp:
##
@@ -126,6 +126,15 @@ TEST_CASE("RepositorymetricsHaveRepo", "[c2m4]") {
REQUIRE("size" == size.name);
REQUIRE("0" == size.value);
+
+minifi::state::response::SerializedResponseNode max_size =
resp.children.at(3);
Review Comment:
Could we convert this to a search, instead of using fixed offsets in the
response node? Just to future-proof the test, in case a new metric is added
later.
##
libminifi/src/core/state/nodes/ResponseNodeLoader.cpp:
##
@@ -32,11 +32,12 @@
namespace org::apache::nifi::minifi::state::response {
-ResponseNodeLoader::ResponseNodeLoader(std::shared_ptr
configuration, std::shared_ptr provenance_repo,
-std::shared_ptr flow_file_repo,
std::shared_ptr flow_configuration)
+ResponseNodeLoader::ResponseNodeLoader(std::shared_ptr
configuration, std::shared_ptr provenance_repo,
+ std::shared_ptr flow_file_repo,
std::shared_ptr content_repo,
std::shared_ptr flow_configuration)
Review Comment:
Couln't we pass a range of metrics sources instead of the fixed number of
repos?
--
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] szaszm commented on a diff in pull request #1490: MINIFICPP-2022 Add valid repository size metrics for all repositories
szaszm commented on code in PR #1490:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1490#discussion_r1115782479
##
libminifi/include/core/Repository.h:
##
@@ -54,28 +54,22 @@ constexpr auto MAX_REPOSITORY_STORAGE_SIZE = 10_MiB;
constexpr auto MAX_REPOSITORY_ENTRY_LIFE_TIME = std::chrono::minutes(10);
constexpr auto REPOSITORY_PURGE_PERIOD = std::chrono::milliseconds(2500);
-class Repository : public core::CoreComponent {
+class Repository : public core::ReportableRepository {
Review Comment:
Looks good. Forgot to mention the design principle behind my recommendation:
[Interface Segregation
Principle](https://en.wikipedia.org/wiki/Interface_segregation_principle) (I in
SOLID)
Whoever depends on `RepositoryMetricsSource` does not need to depend on
`CoreComponent`.
--
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] szaszm commented on a diff in pull request #1490: MINIFICPP-2022 Add valid repository size metrics for all repositories
szaszm commented on code in PR #1490:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1490#discussion_r1108677895
##
libminifi/include/core/Repository.h:
##
@@ -54,28 +54,22 @@ constexpr auto MAX_REPOSITORY_STORAGE_SIZE = 10_MiB;
constexpr auto MAX_REPOSITORY_ENTRY_LIFE_TIME = std::chrono::minutes(10);
constexpr auto REPOSITORY_PURGE_PERIOD = std::chrono::milliseconds(2500);
-class Repository : public core::CoreComponent {
+class Repository : public core::ReportableRepository {
Review Comment:
I would roughly keep the structure, but rename `ReportableRepository` to
`RepositoryMetricsSource`, remove the `CoreComponent` inheritance and the
constructors (e.g. move the inheritance to `Repository`), and practically make
it an interface only, with no data other than the vtable.
After the change, the is-a relationship is sound: `Repository` is a
`RepositoryMetricsSource` (i.e. source of repository metrics).
And `RepositoryMetricsSource` has no requirements other than certain
functions of the interface. Decoupled from component and directory dependencies.
--
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] szaszm commented on a diff in pull request #1490: MINIFICPP-2022 Add valid repository size metrics for all repositories
szaszm commented on code in PR #1490:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1490#discussion_r1108677895
##
libminifi/include/core/Repository.h:
##
@@ -54,28 +54,22 @@ constexpr auto MAX_REPOSITORY_STORAGE_SIZE = 10_MiB;
constexpr auto MAX_REPOSITORY_ENTRY_LIFE_TIME = std::chrono::minutes(10);
constexpr auto REPOSITORY_PURGE_PERIOD = std::chrono::milliseconds(2500);
-class Repository : public core::CoreComponent {
+class Repository : public core::ReportableRepository {
Review Comment:
I would roughly keep the structure, but rename `ReportableRepository` to
`RepositoryMetricsSource`, remove the `CoreComponent` inheritance and the
constructors (e.g. move the inheritance to `Repository`), and practically make
it an interface only, with no data other than the vtable.
--
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] szaszm commented on a diff in pull request #1490: MINIFICPP-2022 Add valid repository size metrics for all repositories
szaszm commented on code in PR #1490:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1490#discussion_r1093453991
##
extensions/rocksdb-repos/database/OpenRocksDb.cpp:
##
@@ -118,8 +114,18 @@ rocksdb::DB* OpenRocksDb::get() {
return impl_.get();
}
-} // namespace internal
-} // namespace minifi
-} // namespace nifi
-} // namespace apache
-} // namespace org
+std::optional OpenRocksDb::getApproximateSizes() const {
+ rocksdb::SizeApproximationOptions options;
+ options.include_memtabtles = true;
+ std::array ranges;
+ ranges[0].start = "";
+ ranges[0].limit = "~";
+ uint64_t value = 0;
+ auto status = impl_->GetApproximateSizes(options, column_->handle.get(),
ranges.data(), 1, );
Review Comment:
```suggestion
const rocksdb::SizeApproximationOptions options{ .include_memtabtles =
true };
const rocksdb::Range range{ .start = "", .limit = "~" };
uint64_t value = 0;
auto status = impl_->GetApproximateSizes(options, column_->handle.get(),
, 1, );
```
Btw, is that typo in the options part of the rocksdb API?
##
libminifi/include/core/Repository.h:
##
@@ -54,28 +54,22 @@ constexpr auto MAX_REPOSITORY_STORAGE_SIZE = 10_MiB;
constexpr auto MAX_REPOSITORY_ENTRY_LIFE_TIME = std::chrono::minutes(10);
constexpr auto REPOSITORY_PURGE_PERIOD = std::chrono::milliseconds(2500);
-class Repository : public core::CoreComponent {
+class Repository : public core::ReportableRepository {
Review Comment:
I feel like this relationship should be modeled the other way around: A
repository is not necessarily "reportable", but a "reportable" repository is
definitely a repository.
It would be even better if "reportability" was implemented in terms of an
independent interface used to export certain metrics.
An interitance relationship results in a very tight coupling of the involved
classes, that is hard to change later, so we need to get this right. What do
you think?
--
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
