MINIFICPP-1395 - Use Identifier instead of its stringified form wherever possible #932
Conversation
libminifi/src/controllers/keyvalue/PersistableKeyValueStoreService.cpp
Outdated
Show resolved
Hide resolved
libminifi/src/controllers/keyvalue/PersistableKeyValueStoreService.cpp
Outdated
Show resolved
Hide resolved
| void appendJsonStr(const std::string& value, rapidjson::Value& parent, rapidjson::Document::AllocatorType& alloc) { // NOLINT | ||
| template<size_t N> | ||
| rapidjson::Value getStringValue(const utils::SmallString<N>& value, rapidjson::Document::AllocatorType& alloc) { | ||
| rapidjson::Value Val; |
There was a problem hiding this comment.
very minor, but I would rename this variable (and the other valueVals) to target_value or json_value or something similar to match our naming conventions
libminifi/include/controllers/keyvalue/AbstractCoreComponentStateManagerProvider.h
Outdated
Show resolved
Hide resolved
| thread_pool_.stopTasks(p); | ||
| for (const auto& processor_id : processors_running_) { | ||
| logger_->log_error("SchedulingAgent is stopped before processor was unscheduled: %s", processor_id.to_string()); | ||
| thread_pool_.stopTasks(processor_id.to_string()); |
There was a problem hiding this comment.
We're passing a processor ID to a function expecting a task ID. This looks suspicious. Do the tasks of a processor have the same ID as the processor?
There was a problem hiding this comment.
yes that seems to be the case, so processors with greater than one maxConcurrentTasks share the same task_status_ flag, this is probably not intentional
There was a problem hiding this comment.
@arpadboda could you confirm or clarify, as the author/major refactorer of this code?
There was a problem hiding this comment.
I think it's intentional.
The IDs are only used to identify the tasks belonging to a given component, the threadpool otherwise doesn't care. So they are not required to be unique.
I wouldn't call it a beautiful design, but works as expected, so I kept it when the code was refactored.
| @@ -128,7 +128,7 @@ bool ProvenanceEventRecord::Serialize(org::apache::nifi::minifi::io::BufferStrea | |||
| return false; | |||
| } | |||
|
|
|||
| ret = outStream.write(this->flow_uuid_); | |||
| ret = outStream.write(this->flow_uuid_.to_string()); | |||
There was a problem hiding this comment.
these .to_string()s are no longer needed here, right?
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit?
For code changes:
For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.