markap14 commented on code in PR #11539:
URL: https://github.com/apache/nifi/pull/11539#discussion_r3786935832
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/components/connector/StandardConnectorNode.java:
##########
@@ -1037,15 +1037,15 @@ public void restoreTroubleshootingState() {
@Override
public void endTroubleshooting() throws FlowUpdateException {
verifyCanEndTroubleshooting();
Review Comment:
[claude-opus-5] `verifyCanEndTroubleshooting` still preflights
`verifyUpdateFlow` against the flow returned by `getActiveFlow` (in
`getReasonCannotEndTroubleshooting`, around line 930), which is no longer the
flow this method installs, and the premise of this PR is that those two differ.
That cuts both ways: the preflight can return 409 because data is queued in a
Connection the real apply would never have removed, and it can pass while the
real apply fails or destroys something the preflight never inspected, surfacing
halfway through the state change. The comment around line 924 saying the
preflight "mirrors exactly what endTroubleshooting() will do" is now stale.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/components/connector/StandardConnectorNode.java:
##########
@@ -1037,15 +1037,15 @@ public void restoreTroubleshootingState() {
@Override
public void endTroubleshooting() throws FlowUpdateException {
verifyCanEndTroubleshooting();
- logger.info("Exiting TROUBLESHOOTING state for {} by restoring
Connector's authoritative flow", this);
+ logger.info("Exiting TROUBLESHOOTING state for {} by re-applying the
Connector's configuration", this);
- final VersionedExternalFlow flowToApply = resolveAuthoritativeFlow();
-
- // Route the update through the ConnectorInitializationContext so that
bundle coordinates referenced by the
- // authoritative flow are resolved against the currently-available
bundles. This mirrors how the initial flow
- // is applied in initializeConnector and avoids failing validation
when the Connector hard-codes a bundle
- // version that differs from the currently-installed NAR (which is
common in test Connectors).
- initializationContext.updateFlow(activeFlowContext, flowToApply,
BundleCompatibility.RESOLVE_BUNDLE);
+ // Re-run the Connector's apply path (rather than a side-effect-free
flow restore) so that value-derived
Review Comment:
[claude-opus-5] This says manual managed Process Group edits are discarded,
but after this change that only holds when the Connector's `applyUpdate` calls
`updateFlow`. Several Connectors in the repository do not:
`NopConnector.applyUpdate` and `DataQueuingConnector.applyUpdate` are empty
method bodies, `GhostConnector.applyUpdate` is empty, and
`ParameterContextConnector.applyUpdate` returns early whenever the Connector is
not fully configured. For any of those, a user can add or delete components
during Troubleshooting, exit, and keep the edits, which is the opposite of the
documented behavior of the state. Previously the framework called `updateFlow`
itself so the revert always happened.
Related: the `Connector.getActiveFlow` JavaDoc in `nifi-api` still states
that it "is invoked when ending Troubleshooting mode in order to restore the
flow that the Connector expects, discarding any user edits made to the Managed
Process Group while in Troubleshooting." That is no longer true, and since
`nifi-api` is pinned at 2.10.0 it cannot be corrected in this PR.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/components/connector/StandardConnectorNode.java:
##########
@@ -1037,15 +1037,15 @@ public void restoreTroubleshootingState() {
@Override
public void endTroubleshooting() throws FlowUpdateException {
verifyCanEndTroubleshooting();
- logger.info("Exiting TROUBLESHOOTING state for {} by restoring
Connector's authoritative flow", this);
+ logger.info("Exiting TROUBLESHOOTING state for {} by re-applying the
Connector's configuration", this);
- final VersionedExternalFlow flowToApply = resolveAuthoritativeFlow();
-
- // Route the update through the ConnectorInitializationContext so that
bundle coordinates referenced by the
- // authoritative flow are resolved against the currently-available
bundles. This mirrors how the initial flow
- // is applied in initializeConnector and avoids failing validation
when the Connector hard-codes a bundle
- // version that differs from the currently-installed NAR (which is
common in test Connectors).
- initializationContext.updateFlow(activeFlowContext, flowToApply,
BundleCompatibility.RESOLVE_BUNDLE);
+ // Re-run the Connector's apply path (rather than a side-effect-free
flow restore) so that value-derived
+ // flow state (any state the Connector computes at apply time) is
recomputed onto the active flow and manual
+ // managed Process Group edits are discarded. Configuration edits are
blocked during Troubleshooting, so the
+ // working configuration still reflects the authoritative
configuration.
+ try (final NarCloseable ignored =
NarCloseable.withComponentNarLoader(extensionManager,
getConnector().getClass(), getIdentifier())) {
Review Comment:
[claude-opus-5] `applyUpdate` is invoked without `prepareForUpdate` having
run. The `Connector` JavaDoc positions `applyUpdate` after `prepareForUpdate`,
and `AbstractConnector.prepareForUpdate` stops the active flow and records
`prepareUpdateFuture`. In practice `verifyCanEndTroubleshooting` already
requires every component to be stopped or disabled, so `AbstractConnector`
subclasses are probably safe, but a Connector that provisions or reserves
anything in `prepareForUpdate` will see `applyUpdate` called in a state it does
not expect. The same JavaDoc also states that the working FlowContext is
destroyed and recreated after `applyUpdate`, which does not happen here.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/components/connector/TestStandardConnectorNode.java:
##########
@@ -1108,6 +1108,27 @@ public void
testIsModifiedReportsTrueWhenAManagedComponentHasStoredState() throw
assertTrue(node.isModified());
}
+ @Test
+ public void testEndTroubleshootingReAppliesConnectorConfiguration() throws
FlowUpdateException {
Review Comment:
[claude-opus-5] This asserts `getApplyUpdateInvocations()` and the context
types that were passed in, which verifies the calling mechanism rather than an
observable contract. Because `TroubleshootingApplyConnector.getActiveFlow`
returns `null`, the test also never exercises an actual flow installation.
Worth adding coverage for: a Connector whose `applyUpdate` does not
reinstall the flow, asserting that manual managed Process Group edits are still
discarded; entering and exiting Troubleshooting with pending
working-configuration changes, asserting the active configuration and the live
flow agree afterward and survive a restart; and `applyUpdate` throwing,
asserting the resulting state and error.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/components/connector/StandardConnectorNode.java:
##########
@@ -1037,15 +1037,15 @@ public void restoreTroubleshootingState() {
@Override
public void endTroubleshooting() throws FlowUpdateException {
verifyCanEndTroubleshooting();
- logger.info("Exiting TROUBLESHOOTING state for {} by restoring
Connector's authoritative flow", this);
+ logger.info("Exiting TROUBLESHOOTING state for {} by re-applying the
Connector's configuration", this);
- final VersionedExternalFlow flowToApply = resolveAuthoritativeFlow();
-
- // Route the update through the ConnectorInitializationContext so that
bundle coordinates referenced by the
- // authoritative flow are resolved against the currently-available
bundles. This mirrors how the initial flow
- // is applied in initializeConnector and avoids failing validation
when the Connector hard-codes a bundle
- // version that differs from the currently-installed NAR (which is
common in test Connectors).
- initializationContext.updateFlow(activeFlowContext, flowToApply,
BundleCompatibility.RESOLVE_BUNDLE);
+ // Re-run the Connector's apply path (rather than a side-effect-free
flow restore) so that value-derived
+ // flow state (any state the Connector computes at apply time) is
recomputed onto the active flow and manual
+ // managed Process Group edits are discarded. Configuration edits are
blocked during Troubleshooting, so the
+ // working configuration still reflects the authoritative
configuration.
Review Comment:
[claude-opus-5] This is not accurate. Configuration edits are blocked
*during* Troubleshooting, but the working configuration can already have
diverged before Troubleshooting was entered.
`StandardConnectorRepository.configureConnector` writes only to the working
configuration and performs no state transition, and
`createEnterTroubleshootingAction` does not block on pending changes;
`hasWorkingConfigurationChanges()` exists precisely because that divergence is
a normal state. Troubleshooting can also be entered from `UPDATE_FAILED`, where
the working configuration holds the edits that just failed to apply.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/components/connector/TestStandardConnectorNode.java:
##########
@@ -1108,6 +1108,27 @@ public void
testIsModifiedReportsTrueWhenAManagedComponentHasStoredState() throw
assertTrue(node.isModified());
}
+ @Test
+ public void testEndTroubleshootingReAppliesConnectorConfiguration() throws
FlowUpdateException {
+ final TroubleshootingApplyConnector connector = new
TroubleshootingApplyConnector();
+ final StandardConnectorNode connectorNode =
createConnectorNode(connector);
+ assertEquals(ConnectorState.STOPPED, connectorNode.getCurrentState());
+
+ connectorNode.enterTroubleshooting();
+ assertEquals(ConnectorState.TROUBLESHOOTING,
connectorNode.getCurrentState());
+
+ // Restoring the authoritative flow via the side-effect-free
getActiveFlow would leave value-derived flow state
Review Comment:
[claude-opus-5] This comment describes the rejected alternative
implementation rather than the behavior under test. Project convention is to
describe the scenario and the contract being verified, not the change relative
to a prior implementation. The same applies to the comments on the new test in
`ConnectorTroubleshootingIT` and to the class JavaDoc on
`TroubleshootingApplyConnector`.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/components/connector/StandardConnectorNode.java:
##########
@@ -1037,15 +1037,15 @@ public void restoreTroubleshootingState() {
@Override
public void endTroubleshooting() throws FlowUpdateException {
verifyCanEndTroubleshooting();
- logger.info("Exiting TROUBLESHOOTING state for {} by restoring
Connector's authoritative flow", this);
+ logger.info("Exiting TROUBLESHOOTING state for {} by re-applying the
Connector's configuration", this);
- final VersionedExternalFlow flowToApply = resolveAuthoritativeFlow();
-
- // Route the update through the ConnectorInitializationContext so that
bundle coordinates referenced by the
- // authoritative flow are resolved against the currently-available
bundles. This mirrors how the initial flow
- // is applied in initializeConnector and avoids failing validation
when the Connector hard-codes a bundle
- // version that differs from the currently-installed NAR (which is
common in test Connectors).
- initializationContext.updateFlow(activeFlowContext, flowToApply,
BundleCompatibility.RESOLVE_BUNDLE);
+ // Re-run the Connector's apply path (rather than a side-effect-free
flow restore) so that value-derived
+ // flow state (any state the Connector computes at apply time) is
recomputed onto the active flow and manual
+ // managed Process Group edits are discarded. Configuration edits are
blocked during Troubleshooting, so the
+ // working configuration still reflects the authoritative
configuration.
+ try (final NarCloseable ignored =
NarCloseable.withComponentNarLoader(extensionManager,
getConnector().getClass(), getIdentifier())) {
+ getConnector().applyUpdate(workingFlowContext, activeFlowContext);
+ }
Review Comment:
[claude-opus-5] There is no error handling around what is now third-party
Connector code. The private `applyUpdate(FrameworkFlowContext)` catches
`Throwable`, logs it, moves the state to `UPDATE_FAILED`, and wraps it in
`FlowUpdateException`. Here an unchecked exception propagates raw through
`StandardConnectorRepository.endTroubleshooting` and
`StandardConnectorDAO.endTroubleshooting`, which only catches
`FlowUpdateException`. That produces a 500 with no log line, the Connector
stuck in `TROUBLESHOOTING`, and a possibly half-rebuilt managed flow.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/components/connector/StandardConnectorNode.java:
##########
@@ -1037,15 +1037,15 @@ public void restoreTroubleshootingState() {
@Override
public void endTroubleshooting() throws FlowUpdateException {
verifyCanEndTroubleshooting();
- logger.info("Exiting TROUBLESHOOTING state for {} by restoring
Connector's authoritative flow", this);
+ logger.info("Exiting TROUBLESHOOTING state for {} by re-applying the
Connector's configuration", this);
Review Comment:
[claude-opus-5] Suggestion on the overall shape: consider keeping the
framework-driven `updateFlow(activeFlowContext, resolveAuthoritativeFlow(),
RESOLVE_BUNDLE)` and then letting the Connector recompute derived state on top
of it, or routing the exit through the real update sequence
(`transitionStateForUpdating` then `prepareForUpdate` then the private
`applyUpdate`). Either one fixes NIFI-16198 while preserving the revert
guarantee and the configuration bookkeeping.
##########
nifi-system-tests/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/connectors/ConnectorTroubleshootingIT.java:
##########
@@ -113,6 +113,38 @@ public void testEnterAndExitTroubleshootingRestoresFlow()
throws NiFiClientExcep
assertConnectorState(connectorId, ConnectorState.RUNNING);
}
+ /**
+ * A Connector may resolve value-derived flow state during its apply path
rather than in the side-effect-free
+ * getActiveFlow. Exiting Troubleshooting must re-run the Connector's
apply path so that derived state is
+ * recomputed onto the managed flow; restoring only the authoritative flow
would leave a required, Parameter-bound
+ * processor property unresolved and the Connector INVALID and unable to
start.
+ */
+ @Test
+ public void testExitTroubleshootingReappliesDerivedParameters() throws
NiFiClientException, IOException, InterruptedException {
Review Comment:
[claude-opus-5] This covers the NIFI-16198 scenario end to end and passed in
CI, so the reported bug is well covered. The gap is the opposite direction:
`testEnterAndExitTroubleshootingRestoresFlow` uses
`ComponentLifecycleConnector`, whose `applyUpdate` does call `updateFlow`, so
nothing in this class catches the case where a Connector's `applyUpdate` does
not reinstall the flow. An equivalent test using `DataQueuingConnector` or
`NopConnector` (add a component during Troubleshooting, exit, assert it is
gone) would fail today.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/components/connector/StandardConnectorNode.java:
##########
@@ -1037,15 +1037,15 @@ public void restoreTroubleshootingState() {
@Override
public void endTroubleshooting() throws FlowUpdateException {
verifyCanEndTroubleshooting();
- logger.info("Exiting TROUBLESHOOTING state for {} by restoring
Connector's authoritative flow", this);
+ logger.info("Exiting TROUBLESHOOTING state for {} by re-applying the
Connector's configuration", this);
- final VersionedExternalFlow flowToApply = resolveAuthoritativeFlow();
-
- // Route the update through the ConnectorInitializationContext so that
bundle coordinates referenced by the
- // authoritative flow are resolved against the currently-available
bundles. This mirrors how the initial flow
- // is applied in initializeConnector and avoids failing validation
when the Connector hard-codes a bundle
- // version that differs from the currently-installed NAR (which is
common in test Connectors).
- initializationContext.updateFlow(activeFlowContext, flowToApply,
BundleCompatibility.RESOLVE_BUNDLE);
+ // Re-run the Connector's apply path (rather than a side-effect-free
flow restore) so that value-derived
+ // flow state (any state the Connector computes at apply time) is
recomputed onto the active flow and manual
+ // managed Process Group edits are discarded. Configuration edits are
blocked during Troubleshooting, so the
+ // working configuration still reflects the authoritative
configuration.
+ try (final NarCloseable ignored =
NarCloseable.withComponentNarLoader(extensionManager,
getConnector().getClass(), getIdentifier())) {
+ getConnector().applyUpdate(workingFlowContext, activeFlowContext);
Review Comment:
[claude-opus-5] This hands the working configuration to the Connector but
never commits it. Connectors read their property values from the working
context (`ParameterContextConnector`, and `DeferredParameterConnector` in this
PR), so the managed flow is rebuilt from the pending configuration. Unlike the
private `applyUpdate(FrameworkFlowContext)` above, nothing here copies the
working configuration onto the active configuration via `replaceProperties` and
nothing calls `recreateWorkingFlowContext()`.
The result is that the live flow reflects the pending configuration while
`flow.json.gz` still holds the old active configuration, so after a restart the
flow silently changes back. `DISCARD_WORKING_CONFIGURATION` also still offers
to discard changes that are already live, and the state is set to `STOPPED`
rather than `UPDATED`. Entering Troubleshooting from `UPDATE_FAILED` makes this
worse: exiting would apply the update that had just failed.
--
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]
