[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1458: MINIFICPP-1991 - Remove unused ControllerServiceProvider methods

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

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-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org