fgerlits commented on a change in pull request #932:
URL: https://github.com/apache/nifi-minifi-cpp/pull/932#discussion_r513306436
##########
File path: libminifi/include/utils/SmallString.h
##########
@@ -34,6 +34,10 @@ class SmallString : public std::array<char, N + 1> {
return {c_str()};
}
+ size_t length() const noexcept {
Review comment:
this could be a `constexpr` function
##########
File path: libminifi/include/utils/ThreadPool.h
##########
@@ -45,6 +45,8 @@ namespace nifi {
namespace minifi {
namespace utils {
+using TaskId = std::string;
Review comment:
are these not UUIDs? why did they remain strings?
##########
File path: libminifi/src/ThreadedSchedulingAgent.cpp
##########
@@ -110,16 +110,15 @@ void
ThreadedSchedulingAgent::schedule(std::shared_ptr<core::Processor> processo
thread_pool_.execute(std::move(functor), future);
}
logger_->log_debug("Scheduled thread %d concurrent workers for for process
%s", processor->getMaxConcurrentTasks(), processor->getName());
- processors_running_.insert(processor->getUUIDStr());
- return;
+ processors_running_.insert(processor->getUUID());
}
void ThreadedSchedulingAgent::stop() {
SchedulingAgent::stop();
std::lock_guard<std::mutex> lock(mutex_);
for (const auto& p : processors_running_) {
- logger_->log_error("SchedulingAgent is stopped before processor was
unscheduled: %s", p);
- thread_pool_.stopTasks(p);
+ logger_->log_error("SchedulingAgent is stopped before processor was
unscheduled: %s", p.to_string());
Review comment:
old code, but I would rename `p` -> `processor_id`
##########
File path: libminifi/src/provenance/Provenance.cpp
##########
@@ -346,25 +352,33 @@ bool ProvenanceEventRecord::DeSerialize(const uint8_t
*buffer, const size_t buff
}
for (uint32_t i = 0; i < number; i++) {
- std::string parentUUID;
- ret = outStream.read(parentUUID);
+ std::string parentUUIDStr;
+ ret = outStream.read(parentUUIDStr);
if (ret <= 0) {
return false;
}
- this->addParentUuid(parentUUID);
+ utils::optional<utils::Identifier> parentUUID =
utils::Identifier::parse(parentUUIDStr);
+ if (!parentUUID) {
+ return false;
+ }
+ this->addParentUuid(parentUUID.value());
}
number = 0;
ret = outStream.read(number);
if (ret != 4) {
return false;
}
for (uint32_t i = 0; i < number; i++) {
- std::string childUUID;
- ret = outStream.read(childUUID);
+ std::string childUUIDStr;
+ ret = outStream.read(childUUIDStr);
if (ret <= 0) {
return false;
}
- this->addChildUuid(childUUID);
+ utils::optional<utils::Identifier> childUUID =
utils::Identifier::parse(childUUIDStr);
+ if (!childUUID) {
+ return false;
+ }
+ this->addChildUuid(childUUID.value());
Review comment:
we do this a lot; it could be pulled out to a function, or even maybe to
a new `outStream.read(utils::Identifier&)` overload
##########
File path: nanofi/src/cxx/Plan.cpp
##########
@@ -304,11 +304,11 @@ std::shared_ptr<minifi::Connection>
ExecutionPlan::connectProcessors(std::shared
connection->setSource(src_proc);
utils::Identifier uuid_copy, uuid_copy_next;
- src_proc->getUUID(uuid_copy);
+ uuid_copy = src_proc->getUUID();
connection->setSourceUUID(uuid_copy);
Review comment:
here, too, this could be
`connection->setSourceUUID(src_proc->getUUID());` etc
##########
File path: libminifi/test/TestBase.cpp
##########
@@ -104,9 +104,9 @@ std::shared_ptr<core::Processor>
TestPlan::addProcessor(const std::shared_ptr<co
connection->setDestination(processor);
utils::Identifier uuid_copy, uuid_copy_next;
- last->getUUID(uuid_copy);
+ uuid_copy = last->getUUID();
connection->setSourceUUID(uuid_copy);
- processor->getUUID(uuid_copy_next);
+ uuid_copy_next = processor->getUUID();
connection->setDestinationUUID(uuid_copy_next);
Review comment:
minor, but this could be simplified to
```c++
connection->setSourceUUID(last->getUUID());
connection->setDestinationUUID(processor->getUUID());
```
(also further down in this file)
##########
File path: libminifi/src/core/reporting/SiteToSiteProvenanceReportingTask.cpp
##########
@@ -68,16 +68,29 @@ void setJsonStr(const std::string& key, const std::string&
value, rapidjson::Val
parent.AddMember(keyVal, valueVal, alloc);
}
-rapidjson::Value getStringValue(const std::string& value,
rapidjson::Document::AllocatorType& alloc) { // NOLINT
+rapidjson::Value getStringValue(const std::string& value,
rapidjson::Document::AllocatorType& alloc) {
rapidjson::Value Val;
Val.SetString(value.c_str(), value.length(), alloc);
return Val;
}
-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;
Review comment:
very minor, but I would rename this variable (and the other `valueVal`s)
to `target_value` or `json_value` or something similar to match our naming
conventions
##########
File path:
libminifi/src/controllers/keyvalue/PersistableKeyValueStoreService.cpp
##########
@@ -29,20 +29,29 @@
PersistableKeyValueStoreService::PersistableKeyValueStoreService(const std::stri
PersistableKeyValueStoreService::~PersistableKeyValueStoreService() = default;
-bool PersistableKeyValueStoreService::setImpl(const std::string& key, const
std::string& value) {
- return set(key, value);
+bool PersistableKeyValueStoreService::setImpl(const utils::Identifier& key,
const std::string& value) {
+ return set(key.to_string(), value);
}
-bool PersistableKeyValueStoreService::getImpl(const std::string& key,
std::string& value) {
- return get(key, value);
+bool PersistableKeyValueStoreService::getImpl(const utils::Identifier& key,
std::string& value) {
+ return get(key.to_string(), value);
}
-bool PersistableKeyValueStoreService::getImpl(std::unordered_map<std::string,
std::string>& kvs) {
- return get(kvs);
+bool PersistableKeyValueStoreService::getImpl(std::map<utils::Identifier,
std::string>& kvs) {
+ std::unordered_map<std::string, std::string> states;
+ if (!get(states)) {
+ return false;
+ }
+ kvs.clear();
+ for (const auto& state : states) {
+ const auto uuid = utils::Identifier::parse(state.first);
Review comment:
it is not immediately clear to me from this code what kind of thing
`uuid` is; I would either change `auto` to `utils::optional<Identifier>` or
change the name of the variable to something like `optional_uuid`
##########
File path:
libminifi/src/controllers/keyvalue/PersistableKeyValueStoreService.cpp
##########
@@ -29,20 +29,29 @@
PersistableKeyValueStoreService::PersistableKeyValueStoreService(const std::stri
PersistableKeyValueStoreService::~PersistableKeyValueStoreService() = default;
-bool PersistableKeyValueStoreService::setImpl(const std::string& key, const
std::string& value) {
- return set(key, value);
+bool PersistableKeyValueStoreService::setImpl(const utils::Identifier& key,
const std::string& value) {
+ return set(key.to_string(), value);
}
-bool PersistableKeyValueStoreService::getImpl(const std::string& key,
std::string& value) {
- return get(key, value);
+bool PersistableKeyValueStoreService::getImpl(const utils::Identifier& key,
std::string& value) {
+ return get(key.to_string(), value);
}
-bool PersistableKeyValueStoreService::getImpl(std::unordered_map<std::string,
std::string>& kvs) {
- return get(kvs);
+bool PersistableKeyValueStoreService::getImpl(std::map<utils::Identifier,
std::string>& kvs) {
+ std::unordered_map<std::string, std::string> states;
+ if (!get(states)) {
+ return false;
+ }
+ kvs.clear();
+ for (const auto& state : states) {
+ const auto uuid = utils::Identifier::parse(state.first);
+ kvs[uuid.value()] = state.second;
Review comment:
what happens if this throws? where do we catch 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.
For queries about this service, please contact Infrastructure at:
[email protected]
