[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #932: MINIFICPP-1395 - Use Identifier instead of its stringified form wherever possible
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 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 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 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 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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #932: MINIFICPP-1395 - Use Identifier instead of its stringified form wherever possible
arpadboda commented on a change in pull request #932: URL: https://github.com/apache/nifi-minifi-cpp/pull/932#discussion_r513335438 ## File path: libminifi/src/CronDrivenSchedulingAgent.cpp ## @@ -56,8 +56,8 @@ utils::TaskRescheduleInfo CronDrivenSchedulingAgent::run(const std::shared_ptrgetCronPeriod()); result = schedule.cron_to_next(from); -last_exec_[uuidStr] = result; -schedules_.insert(std::make_pair(uuidStr, schedule)); +last_exec_[uuid] = result; +schedules_.insert(std::make_pair(uuid, schedule)); Review comment: 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: us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #932: MINIFICPP-1395 - Use Identifier instead of its stringified form wherever possible
arpadboda commented on a change in pull request #932:
URL: https://github.com/apache/nifi-minifi-cpp/pull/932#discussion_r513335352
##
File path: libminifi/include/core/CoreComponentState.h
##
@@ -48,13 +49,13 @@ class CoreComponentStateManagerProvider {
public:
virtual ~CoreComponentStateManagerProvider() = default;
- virtual std::shared_ptr
getCoreComponentStateManager(const std::string& uuid) = 0;
+ virtual std::shared_ptr
getCoreComponentStateManager(const utils::Identifier& uuid) = 0;
virtual std::shared_ptr
getCoreComponentStateManager(const CoreComponent& component) {
-return getCoreComponentStateManager(component.getUUIDStr());
+return getCoreComponentStateManager(component.getUUID());
}
- virtual std::unordered_map> getAllCoreComponentStates() = 0;
+ virtual std::map> getAllCoreComponentStates() = 0;
Review comment:
Okay, makes sense, thanks!
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #932: MINIFICPP-1395 - Use Identifier instead of its stringified form wherever possible
arpadboda commented on a change in pull request #932:
URL: https://github.com/apache/nifi-minifi-cpp/pull/932#discussion_r513309724
##
File path: libminifi/include/sitetosite/SiteToSite.h
##
@@ -246,24 +246,24 @@ class Transaction {
current_transfers_ = 0;
total_transfers_ = 0;
_bytes = 0;
-
-// Generate the global UUID for the transaction
-uuid_str_ = uuid_.to_string();
}
// Destructor
virtual ~Transaction() = default;
// getUUIDStr
- std::string getUUIDStr() {
-return uuid_str_;
+ utils::SmallString<36> getUUIDStr() {
Review comment:
const
##
File path: libminifi/include/core/CoreComponentState.h
##
@@ -48,13 +49,13 @@ class CoreComponentStateManagerProvider {
public:
virtual ~CoreComponentStateManagerProvider() = default;
- virtual std::shared_ptr
getCoreComponentStateManager(const std::string& uuid) = 0;
+ virtual std::shared_ptr
getCoreComponentStateManager(const utils::Identifier& uuid) = 0;
virtual std::shared_ptr
getCoreComponentStateManager(const CoreComponent& component) {
-return getCoreComponentStateManager(component.getUUIDStr());
+return getCoreComponentStateManager(component.getUUID());
}
- virtual std::unordered_map> getAllCoreComponentStates() = 0;
+ virtual std::map> getAllCoreComponentStates() = 0;
Review comment:
What's the reason behind this change?
If you do this, unordered map include can be removed imho.
##
File path: libminifi/src/CronDrivenSchedulingAgent.cpp
##
@@ -56,8 +56,8 @@ utils::TaskRescheduleInfo
CronDrivenSchedulingAgent::run(const std::shared_ptrgetCronPeriod());
result = schedule.cron_to_next(from);
-last_exec_[uuidStr] = result;
-schedules_.insert(std::make_pair(uuidStr, schedule));
+last_exec_[uuid] = result;
+schedules_.insert(std::make_pair(uuid, schedule));
Review comment:
It's old code, but it's a map! :)
schedules_[uuid] = schedule;
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:
us...@infra.apache.org
