jrsteinebrey commented on code in PR #9273:
URL: https://github.com/apache/nifi/pull/9273#discussion_r1765706305


##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/service/StandardControllerServiceProvider.java:
##########
@@ -253,38 +252,44 @@ public CompletableFuture<Void> 
enableControllerService(final ControllerServiceNo
 
     @Override
     public void enableControllerServices(final 
Collection<ControllerServiceNode> serviceNodes) {
-        boolean shouldStart = true;
+        for (ControllerServiceNode controllerServiceNode : 
filterStartableControllerServices(serviceNodes)) {
+            try {
+                final Future<Void> future = 
enableControllerServiceAndDependencies(controllerServiceNode);
 
-        Iterator<ControllerServiceNode> serviceIter = serviceNodes.iterator();
-        while (serviceIter.hasNext() && shouldStart) {
-            ControllerServiceNode controllerServiceNode = serviceIter.next();
-            List<ControllerServiceNode> requiredServices = 
controllerServiceNode.getRequiredControllerServices();
-            for (ControllerServiceNode requiredService : requiredServices) {
-                if (!requiredService.isActive() && 
!serviceNodes.contains(requiredService)) {
-                    shouldStart = false;
-                    logger.debug("Will not start {} because required service 
{} is not active and is not part of the collection of things to start", 
serviceNodes, requiredService);
+                future.get(30, TimeUnit.SECONDS);
+                logger.debug("Successfully enabled {}; service state = {}", 
controllerServiceNode, controllerServiceNode.getState());
+            } catch (final ControllerServiceNotValidException csnve) {
+                logger.warn("Failed to enable service {} because it is not 
currently valid", controllerServiceNode);
+            } catch (Exception e) {
+                logger.error("Failed to enable {}", controllerServiceNode, e);
+                if (this.bulletinRepo != null) {
+                    
this.bulletinRepo.addBulletin(BulletinFactory.createBulletin("Controller 
Service",
+                            Severity.ERROR.name(), "Could not start " + 
controllerServiceNode + " due to " + e));
                 }
             }
         }
+    }
 
-        if (shouldStart) {
-            for (ControllerServiceNode controllerServiceNode : serviceNodes) {
-                try {
-                    final Future<Void> future = 
enableControllerServiceAndDependencies(controllerServiceNode);
-
-                    future.get(30, TimeUnit.SECONDS);
-                    logger.debug("Successfully enabled {}; service state = 
{}", controllerServiceNode, controllerServiceNode.getState());
-                } catch (final ControllerServiceNotValidException csnve) {
-                    logger.warn("Failed to enable service {} because it is not 
currently valid", controllerServiceNode);
-                } catch (Exception e) {
-                    logger.error("Failed to enable {}", controllerServiceNode, 
e);
-                    if (this.bulletinRepo != null) {
-                        
this.bulletinRepo.addBulletin(BulletinFactory.createBulletin("Controller 
Service",
-                                Severity.ERROR.name(), "Could not start " + 
controllerServiceNode + " due to " + e));
-                    }
+    private Collection<ControllerServiceNode> 
filterStartableControllerServices(final Collection<ControllerServiceNode> 
serviceNodes) {
+        Collection<ControllerServiceNode> startableServiceNodes = new 
HashSet<>();
+        for (ControllerServiceNode serviceNode : serviceNodes) {
+            boolean isStartable = true;
+            List<ControllerServiceNode> requiredServices = 
serviceNode.getRequiredControllerServices();
+            for (ControllerServiceNode requiredService : requiredServices) {
+                if (!requiredService.isActive() && 
!serviceNodes.contains(requiredService)) {
+                    isStartable = false;
+                    logger.error("Will not start {} because its required 
service {} is not active and is not part of the collection of things to start", 
serviceNode, requiredService);
                 }
             }
+            if (isStartable) {
+                startableServiceNodes.add(serviceNode);
+            }
+        }
+        if (startableServiceNodes.size() < serviceNodes.size()) {
+            // If any service was removed, then recheck all remaining services 
because the removed one might be required by another service.
+            startableServiceNodes = 
filterStartableControllerServices(startableServiceNodes);
         }
+        return startableServiceNodes;

Review Comment:
   So this change does **not** enable any controller services which the caller 
of this method did **not** ask to enable. And does not introduce any new side 
effect of enabled unwanted controller services.



-- 
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

Reply via email to