pvillard31 commented on code in PR #10630:
URL: https://github.com/apache/nifi/pull/10630#discussion_r2619569350
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/dao/impl/StandardControllerServiceDAO.java:
##########
@@ -186,6 +188,25 @@ public ControllerServiceNode updateControllerService(final
ControllerServiceDTO
if
(ControllerServiceState.ENABLED.equals(purposedControllerServiceState)) {
serviceProvider.enableControllerService(controllerService);
} else if
(ControllerServiceState.DISABLED.equals(purposedControllerServiceState)) {
+ // First, unschedule all referencing schedulable
components (processors, reporting tasks, etc.)
+ final Map<ComponentNode, Future<Void>> unscheduleFutures =
serviceProvider.unscheduleReferencingComponents(controllerService);
Review Comment:
So... the embedded behavior was intentional to fix the root cause of Issue 1
in the PR description.
Basically, when disabling a Controller Service through the UI, the frontend
is supposed to coordinate two separate API calls:
- First call `updateControllerServiceReferencingComponents` to stop
referencing processors
- Then call `updateControllerService` to disable the service
However, when the Controller Service is invalid, this coordination breaks
down. The UI code path that handles invalid services doesn't properly invoke
the first step, leading to the error where users see "Cannot disable -
referencing components are still running" even though the UI claims it stopped
them.
Why embedding the logic here makes sense:
- Disabling a Controller Service requires that referencing components be
stopped first so this ensures the invariant is always maintained regardless of
how the API is called.
- The caller says "disable this service" and the system ensures all
prerequisites are handled. This is more robust than requiring callers to
coordinate multiple calls correctly.
I could extract this into a dedicated method like
`disableControllerServiceWithReferences()` and call it from here to make the
embedded logic more explicit. Something like:
```java
} else if
(ControllerServiceState.DISABLED.equals(purposedControllerServiceState)) {
disableControllerServiceWithReferences(controllerService);
}
```
Thoughts?
--
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]
