[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #932: MINIFICPP-1395 - Use Identifier instead of its stringified form wherever possible

[GitBox <gi...@apache.org>]

arpadboda commented on a change in pull request #932:
URL: https://github.com/apache/nifi-minifi-cpp/pull/932#discussion_r515886712



##########
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:
       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. 
   I wouldn't call it a beautiful design, but works as expected, so I kept it when the code was refactored. 

##########
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:
       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. 




----------------------------------------------------------------
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:
users@infra.apache.org