Copilot commented on code in PR #2195:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2195#discussion_r3352632393
##########
extensions/rocksdb-repos/tests/ProvenanceTests.cpp:
##########
@@ -52,7 +52,7 @@ TEST_CASE("Test Provenance record serialization",
"[Testprovenance::ProvenanceEv
record1->setEventDuration(sample);
testRepository->storeElement(record1);
- auto record2 = std::make_shared<provenance::ProvenanceEventRecordImpl>();
+ auto record2 = provenance::ProvenanceEventRecord::create();
record2->setEventId(eventId);
Review Comment:
`ProvenanceEventRecordImpl` no longer has a `(ProvenanceEventType,
std::string, std::string)` constructor (it now takes `utils::Identifier` for
the component id). This test file still constructs `ProvenanceEventRecordImpl`
with string literals in multiple places (e.g. earlier `record1 =
std::make_shared<...>(..., "componentid", ...)`), which will fail to compile
after this refactor. Please update those `record1` constructions to pass a
`utils::Identifier` (e.g. a default/nil `utils::Identifier{}` if the actual
value is irrelevant to the assertions, or a real parsed UUID if it matters).
##########
libminifi/src/core/reporting/SiteToSiteProvenanceReportingTask.cpp:
##########
@@ -148,24 +147,27 @@ void
SiteToSiteProvenanceReportingTask::getJsonReport(core::ProcessContext&, cor
rapidjson::PrettyWriter<rapidjson::StringBuffer> writer(buffer);
array.Accept(writer);
- report = buffer.GetString();
-}
-
-void SiteToSiteProvenanceReportingTask::onSchedule(core::ProcessContext&,
core::ProcessSessionFactory&) {
+ return buffer.GetString();
}
void SiteToSiteProvenanceReportingTask::onTrigger(core::ProcessContext&
context, core::ProcessSession& session) {
- logger_->log_debug("SiteToSiteProvenanceReportingTask -- onTrigger");
- std::vector<std::shared_ptr<core::SerializableComponent>> records;
- logger_->log_debug("batch size {} records", batch_size_);
- size_t deserialized = batch_size_;
+ auto* state_manager = context.getStateManager();
+ if (!state_manager) {
+ logger_->log_error("Failed to get StateManager");
+ context.yield();
+ return;
+ }
std::shared_ptr<core::Repository> repo = context.getProvenanceRepository();
- if (!repo->getElements(records, deserialized) && deserialized == 0) {
+ if (!repo) {
+ throw minifi::Exception(ExceptionType::REPOSITORY_EXCEPTION, "Failed to
retrieve provenance repository");
+ }
Review Comment:
Throwing an exception from `onTrigger` when the provenance repository is
unavailable can take down the scheduling thread / task unexpectedly. This code
already uses `context.yield()` for other transient failures (missing
StateManager / protocol), so repository lookup failure should follow the same
pattern (log + yield + return) instead of throwing.
##########
extensions/rocksdb-repos/ProvenanceRepository.cpp:
##########
@@ -73,28 +73,30 @@ bool ProvenanceRepository::initialize(const
std::shared_ptr<org::apache::nifi::m
return true;
}
-bool
ProvenanceRepository::getElements(std::vector<std::shared_ptr<core::SerializableComponent>>
&records, size_t &max_size) {
+std::vector<std::shared_ptr<core::SerializableComponent>>
ProvenanceRepository::getElements(size_t max_size) {
+ if (max_size == 0) {
+ return {};
+ }
auto opendb = db_->open();
if (!opendb) {
- return false;
+ return {};
}
Review Comment:
If the RocksDB database cannot be opened here, returning an empty vector
makes the failure indistinguishable from "no new provenance records" at call
sites. Logging an error before returning would make operational diagnosis much
easier (similar to other RocksDB-backed components that log on open failures).
--
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]
