fgerlits commented on code in PR #1458:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1458#discussion_r1060126151
##########
libminifi/include/core/controller/StandardControllerServiceProvider.h:
##########
@@ -97,22 +73,15 @@ class StandardControllerServiceProvider : public
ControllerServiceProvider, publ
return new_service_node;
}
- std::future<utils::TaskRescheduleInfo>
enableControllerService(std::shared_ptr<ControllerServiceNode> &serviceNode) {
- if (serviceNode->canEnable()) {
- return agent_->enableControllerService(serviceNode);
- } else {
- std::future<utils::TaskRescheduleInfo> no_run =
std::async(std::launch::deferred, utils::TaskRescheduleInfo::Done);
- return no_run;
- }
- }
-
virtual void enableAllControllerServices() {
logger_->log_info("Enabling %u controller services",
controller_map_->getAllControllerServices().size());
for (auto service : controller_map_->getAllControllerServices()) {
- if (service->canEnable()) {
- logger_->log_info("Enabling %s", service->getName());
- agent_->enableControllerService(service);
- } else {
+ logger_->log_info("Enabling %s", service->getName());
+ if (!service->canEnable()) {
+ logger_->log_warn("Service %s can not be enabled", service->getName());
Review Comment:
sorry about the pedantry, but this should be "cannot" instead of "can not"
##########
libminifi/include/core/ProcessGroup.h:
##########
@@ -231,7 +231,7 @@ class ProcessGroup : public CoreComponent {
void verify() const;
protected:
- void startProcessingProcessors(const
std::shared_ptr<TimerDrivenSchedulingAgent>& timeScheduler, const
std::shared_ptr<EventDrivenSchedulingAgent> &eventScheduler, const
std::shared_ptr<CronDrivenSchedulingAgent> &cronScheduler); // NOLINT
+ void startProcessingProcessors(TimerDrivenSchedulingAgent& timeScheduler,
EventDrivenSchedulingAgent& eventScheduler, CronDrivenSchedulingAgent&
cronScheduler); // NOLINT
Review Comment:
is the `// NOLINT` still needed?
##########
extensions/http-curl/tests/ControllerServiceIntegrationTests.cpp:
##########
@@ -108,27 +108,28 @@ int main(int argc, char **argv) {
assert(ssl_client->getCACertificate().length() > 0);
// now let's disable one of the controller services.
std::shared_ptr<core::controller::ControllerServiceNode> cs_id =
controller->getControllerServiceNode("ID");
- const auto checkCsIdEnabledMatchesDisabledFlag = [&cs_id] { return !disabled
== cs_id->enabled(); };
assert(cs_id != nullptr);
- {
- std::lock_guard<std::mutex> lock(control_mutex);
- controller->enableControllerService(cs_id);
- disabled = false;
- }
- std::shared_ptr<core::controller::ControllerServiceNode> mock_cont =
controller->getControllerServiceNode("MockItLikeIts1995");
- assert(verifyEventHappenedInPollTime(std::chrono::seconds(4),
checkCsIdEnabledMatchesDisabledFlag));
- {
- std::lock_guard<std::mutex> lock(control_mutex);
- controller->disableReferencingServices(mock_cont);
- disabled = true;
- }
- assert(verifyEventHappenedInPollTime(std::chrono::seconds(2),
checkCsIdEnabledMatchesDisabledFlag));
- {
- std::lock_guard<std::mutex> lock(control_mutex);
- controller->enableReferencingServices(mock_cont);
- disabled = false;
- }
- assert(verifyEventHappenedInPollTime(std::chrono::seconds(2),
checkCsIdEnabledMatchesDisabledFlag));
+ // TODO(adebreceni): MINIFICPP-1992
Review Comment:
can you mention this TODO in the Jira, so we'll remember to remove it as
part of fixing the Jira, please?
##########
libminifi/include/utils/ThreadPool.h:
##########
@@ -296,6 +297,9 @@ class ThreadPool {
std::this_thread::sleep_for(std::chrono::milliseconds(1));
}
}
+
+ static std::shared_ptr<core::logging::Logger> logger_;
Review Comment:
Are you sure it's worth the savings of making this static? Crashes due to a
messed-up static initialization order are difficult to debug, so I would make
it an instance variable.
##########
libminifi/src/utils/ThreadPool.cpp:
##########
@@ -182,15 +185,22 @@ void ThreadPool<T>::manageWorkers() {
template<typename T>
void ThreadPool<T>::start() {
- if (nullptr != controller_service_provider_) {
- auto thread_man =
controller_service_provider_->getControllerService("ThreadPoolManager");
- thread_manager_ = thread_man != nullptr ?
std::dynamic_pointer_cast<controllers::ThreadManagementService>(thread_man) :
nullptr;
- } else {
- thread_manager_ = nullptr;
- }
-
std::lock_guard<std::recursive_mutex> lock(manager_mutex_);
if (!running_) {
+ thread_manager_.reset();
+ if (nullptr != controller_service_provider_) {
+ auto service =
controller_service_provider_->getControllerService("ThreadPoolManager");
+ if (!service) {
+ logger_->log_info("Could not find a ThreadPoolManager service");
+ } else {
+ if (auto thread_manager_service =
std::dynamic_pointer_cast<controllers::ThreadManagementService>(service)) {
+ thread_manager_ = thread_manager_service;
+ } else {
+ logger_->log_error("Found ThreadPoolManager, but it is not a
ThreadManagementService");
+ }
+ }
+ }
Review Comment:
I think this would be nicer as
```suggestion
thread_manager_ = createThreadManager();
```
with the rest of the logic moved to the helper function, which could use a
return-early pattern
--
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.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
