jrsteinebrey commented on code in PR #9273:
URL: https://github.com/apache/nifi/pull/9273#discussion_r1765704164
##########
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:
@exceptionfactory Thanks for reviewing this and giving feedback.
This method being changed:
StandardControllerServiceProvider.enableControllerServices(
final Collection<ControllerServiceNode> **serviceNodesIn**)
In the existing code and in the new code, the controller services in the
**serviceNodes** parameter
contains only controller services which are supposed to be enabled.
The caller decides which services it intends to enable.
If a controller service is not supposed to be started on Nifi restart (i.e.
scheduledState is disabled), then it should not be (and is not) put in
**serviceNodes** by the existing calling code.
To recap, the method I am changing receives a list of only the services that
the caller wants to enable (based on the caller's decision logic). It does
**not** get a list of all controller services when NiFi is being restarted.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
