szaszm commented on a change in pull request #932:
URL: https://github.com/apache/nifi-minifi-cpp/pull/932#discussion_r513840725
##########
File path: libminifi/include/core/logging/Logger.h
##########
@@ -59,11 +59,18 @@ inline char const* conditional_conversion(const
utils::SmallString<N>& arr) {
return arr.c_str();
}
-template<typename T>
+template<typename T, typename = typename std::enable_if<
+ std::is_arithmetic<T>::value ||
+ std::is_enum<T>::value ||
+ std::is_pointer<T>::value>::type>
inline T conditional_conversion(T const& t) {
return t;
}
+inline char const* conditional_conversion(const char* str) {
+ return str;
+}
Review comment:
Why do we need this overload? `std::is_pointer<const char*>::value ==
true`
##########
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);
+ 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());
Review 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?
##########
File path:
libminifi/include/controllers/keyvalue/AbstractCoreComponentStateManagerProvider.h
##########
@@ -59,10 +60,10 @@ class AbstractCoreComponentStateManagerProvider : public
std::enable_shared_from
};
protected:
- virtual bool setImpl(const std::string& key, const std::string& value) = 0;
- virtual bool getImpl(const std::string& key, std::string& value) = 0;
- virtual bool getImpl(std::unordered_map<std::string, std::string>& kvs) = 0;
- virtual bool removeImpl(const std::string& key) = 0;
+ virtual bool setImpl(const utils::Identifier& key, const std::string& value)
= 0;
+ virtual bool getImpl(const utils::Identifier& key, std::string& value) = 0;
+ virtual bool getImpl(std::map<utils::Identifier, std::string>& kvs) = 0;
+ virtual bool removeImpl(const utils::Identifier& key) = 0;
Review comment:
These got me confused for a second, I thought that the keys are state
storage keys and the values are the corresponding values. Would you mind adding
a comment above these declarations describing that these are CoreComponent IDs
and their corresponding state serialized to a string (if I understand
correctly)?
##########
File path:
libminifi/src/controllers/keyvalue/PersistableKeyValueStoreService.cpp
##########
@@ -29,20 +29,31 @@
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) {
+ utils::optional<utils::Identifier> optional_uuid =
utils::Identifier::parse(state.first);
+ if (optional_uuid) {
+ kvs[optional_uuid.value()] = state.second;
+ }
Review comment:
Can we log or throw in the else branch?
##########
File path: libminifi/include/core/CoreComponentState.h
##########
@@ -48,13 +49,13 @@ class CoreComponentStateManagerProvider {
public:
virtual ~CoreComponentStateManagerProvider() = default;
- virtual std::shared_ptr<CoreComponentStateManager>
getCoreComponentStateManager(const std::string& uuid) = 0;
+ virtual std::shared_ptr<CoreComponentStateManager>
getCoreComponentStateManager(const utils::Identifier& uuid) = 0;
virtual std::shared_ptr<CoreComponentStateManager>
getCoreComponentStateManager(const CoreComponent& component) {
- return getCoreComponentStateManager(component.getUUIDStr());
+ return getCoreComponentStateManager(component.getUUID());
}
- virtual std::unordered_map<std::string, std::unordered_map<std::string,
std::string>> getAllCoreComponentStates() = 0;
+ virtual std::map<utils::Identifier, std::unordered_map<std::string,
std::string>> getAllCoreComponentStates() = 0;
Review comment:
You can specialize `std::hash` on the `Identifier` type to avoid having
to explicitly specify the hash function every time.
An easy way to write a hash function: https://stackoverflow.com/a/54728293
I'm not insisting that you write one, but it seems to be an easy win 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.
For queries about this service, please contact Infrastructure at:
[email protected]
