Github user alopresto commented on a diff in the pull request: https://github.com/apache/nifi/pull/541#discussion_r67581551 --- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/service/StandardControllerServiceProvider.java --- @@ -372,98 +369,32 @@ public void enableControllerService(final ControllerServiceNode serviceNode) { @Override public void enableControllerServices(final Collection<ControllerServiceNode> serviceNodes) { - final Set<ControllerServiceNode> servicesToEnable = new HashSet<>(); - // Ensure that all nodes are already disabled - for (final ControllerServiceNode serviceNode : serviceNodes) { - final ControllerServiceState curState = serviceNode.getState(); - if (ControllerServiceState.DISABLED.equals(curState)) { - servicesToEnable.add(serviceNode); - } else { - logger.warn("Cannot enable {} because it is not disabled; current state is {}", serviceNode, curState); + boolean shouldStart = true; + + Iterator<ControllerServiceNode> serviceIter = serviceNodes.iterator(); + while (serviceIter.hasNext() && shouldStart) { + ControllerServiceNode controllerServiceNode = serviceIter.next(); + List<ControllerServiceNode> requiredServices = ((StandardControllerServiceNode) controllerServiceNode).getRequiredControllerServices(); + for (ControllerServiceNode requiredService : requiredServices) { + if (!requiredService.isActive() && !serviceNodes.contains(requiredService)) { + shouldStart = false; + } } } - // determine the order to load the services. We have to ensure that if service A references service B, then B - // is enabled first, and so on. - final Map<String, ControllerServiceNode> idToNodeMap = new HashMap<>(); - for (final ControllerServiceNode node : servicesToEnable) { - idToNodeMap.put(node.getIdentifier(), node); - } - - // We can have many Controller Services dependent on one another. We can have many of these - // disparate lists of Controller Services that are dependent on one another. We refer to each - // of these as a branch. - final List<List<ControllerServiceNode>> branches = determineEnablingOrder(idToNodeMap); - - if (branches.isEmpty()) { - logger.info("No Controller Services to enable"); - return; - } else { - logger.info("Will enable {} Controller Services", servicesToEnable.size()); - } - - final Set<ControllerServiceNode> enabledNodes = Collections.synchronizedSet(new HashSet<ControllerServiceNode>()); - final ExecutorService executor = Executors.newFixedThreadPool(Math.min(10, branches.size()), new ThreadFactory() { - @Override - public Thread newThread(final Runnable r) { - final Thread t = Executors.defaultThreadFactory().newThread(r); - t.setDaemon(true); - t.setName("Enable Controller Services"); - return t; - } - }); - - for (final List<ControllerServiceNode> branch : branches) { - final Runnable enableBranchRunnable = new Runnable() { + if (shouldStart) { + List<ControllerServiceNode> services = new ArrayList<>(serviceNodes); + Collections.sort(services, new Comparator<ControllerServiceNode>() { --- End diff -- This looks like a much cleaner way to perform this behavior, but are there any tests for the dependency traversal to ensure it's complete?
--- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---