exceptionfactory commented on code in PR #10630:
URL: https://github.com/apache/nifi/pull/10630#discussion_r2619303863
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/service/StandardControllerServiceNode.java:
##########
@@ -753,6 +753,18 @@ public CompletableFuture<Void> disable(final
ScheduledExecutorService scheduler)
final CompletableFuture<Void> future = new CompletableFuture<>();
final boolean transitioned =
this.stateTransition.transitionToDisabling(ControllerServiceState.ENABLING,
future);
if (transitioned) {
+ // If we transitioned from ENABLING to DISABLING, we need to
immediately complete the disable
+ // because the enable task may be scheduled to run far in the
future (up to 10 minutes) due to
+ // validation retries. Rather than making the user wait, we
immediately transition to DISABLED.
+ scheduler.execute(() -> {
+ stateTransition.disable();
Review Comment:
Transitioning the state to `DISABLED` makes sense based on the description,
but it raises about invoking the `OnDisabled` methods using `invokeDisable` as
done in the next block of this method. The enabling process handles this
scenario.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/service/StandardControllerServiceReference.java:
##########
@@ -46,17 +47,25 @@ public Set<ComponentNode> getReferencingComponents() {
return Collections.unmodifiableSet(components);
}
+ /**
+ * Determines if a component is running or in the process of starting.
+ * A component might be stuck in STARTING state if it references an
invalid controller service
+ * (e.g., after a restart when the controller service configuration became
invalid).
+ * Such components are considered "active" and would prevent the
controller service from being disabled.
+ */
private boolean isRunning(final ComponentNode component) {
Review Comment:
Since the behavior of this method is changing, what do you think about
renaming it is `isActive()` aligning more with the `ControllerServiceNode`
check, and also with the reference is building active references.
##########
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:
It seems like this introduces more embedded behavior in this method, versus
requiring the other method `updateControllerServiceReferencingComponents` to be
called first. Should this be a separate operation invoked elsewhere?
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/service/TestStandardControllerServiceProvider.java:
##########
@@ -437,6 +440,107 @@ public void testEnableReferencingComponents() {
assertEquals(ScheduledState.STOPPED, procNode.getScheduledState());
}
+ /**
+ * Test that unscheduleReferencingComponents handles processors in
STARTING state.
+ * This scenario can occur when a processor references an invalid
controller service
+ * (e.g., after a restart when the controller service configuration became
invalid).
+ * The processor might be stuck in STARTING state and should still be
stopped.
+ */
+ @Test
+ public void
testUnscheduleReferencingComponentsIncludesStartingProcessors() {
+ final ProcessGroup procGroup = new MockProcessGroup(flowManager);
+
+ final FlowManager flowManager = Mockito.mock(FlowManager.class);
Review Comment:
The `Mockito.mock()` references here can be replaced with a static `mock()`
method reference.
##########
nifi-system-tests/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/controllerservice/ControllerServiceDisableWithReferencesIT.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.tests.system.controllerservice;
+
+import org.apache.nifi.tests.system.NiFiSystemIT;
+import org.apache.nifi.toolkit.client.NiFiClientException;
+import org.apache.nifi.web.api.dto.ProcessorDTO;
+import org.apache.nifi.web.api.entity.ControllerServiceEntity;
+import org.apache.nifi.web.api.entity.ProcessorEntity;
+import org.junit.jupiter.api.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Map;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * System tests for verifying that disabling a Controller Service properly
stops
+ * referencing components (processors, reporting tasks, etc.) before disabling.
+ *
+ * This addresses the scenario where:
+ * 1. A Controller Service is enabled with referencing processors running
+ * 2. User attempts to disable the Controller Service
+ * 3. The system should automatically stop referencing processors first
+ * 4. Then disable the Controller Service
+ */
+public class ControllerServiceDisableWithReferencesIT extends NiFiSystemIT {
+
+ /**
+ * Test that disabling a Controller Service automatically stops
referencing processors.
+ * This verifies the fix for the issue where disabling a Controller
Service would fail
+ * if there were running processors that referenced it.
+ */
+ @Test
+ public void testDisableControllerServiceStopsReferencingProcessors()
throws NiFiClientException, IOException, InterruptedException {
+ // Create a simple controller service (StandardSleepService doesn't
require external resources)
+ final ControllerServiceEntity sleepService =
getClientUtil().createControllerService("StandardSleepService");
+
+ // Create a processor that references this controller service
+ final ProcessorEntity processor =
getClientUtil().createProcessor("Sleep");
+ getClientUtil().updateProcessorProperties(processor, Map.of("Sleep
Service", sleepService.getId()));
+ getClientUtil().setAutoTerminatedRelationships(processor, "success");
+
+ // Enable the controller service
+ getClientUtil().enableControllerService(sleepService);
+ getClientUtil().waitForControllerServicesEnabled("root");
+
+ // Start the processor
+ getClientUtil().waitForValidProcessor(processor.getId());
+ getClientUtil().startProcessor(processor);
+ getClientUtil().waitForProcessorState(processor.getId(), "RUNNING");
+
+ // Now disable the controller service - this should automatically stop
the processor first
+ final ControllerServiceEntity serviceToDisable =
getNifiClient().getControllerServicesClient().getControllerService(sleepService.getId());
+ getClientUtil().disableControllerService(serviceToDisable);
+
+ // Wait for the controller service to be disabled
+
getClientUtil().waitForControllerServiceRunStatus(sleepService.getId(),
"DISABLED");
+
+ // Verify the processor was stopped
+ final ProcessorEntity updatedProcessor =
getNifiClient().getProcessorClient().getProcessor(processor.getId());
+ final ProcessorDTO processorDto = updatedProcessor.getComponent();
+ assertEquals("STOPPED", processorDto.getState(),
+ "Processor should be stopped after disabling the referenced
controller service");
+
+ // Verify the controller service is disabled
+ final ControllerServiceEntity updatedService =
getNifiClient().getControllerServicesClient().getControllerService(sleepService.getId());
+ assertEquals("DISABLED", updatedService.getComponent().getState(),
Review Comment:
Recommend declaring and reusing static variables for some of these values,
such as `DISABLED`.
--
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]
