bobpaulin commented on code in PR #11872:
URL: https://github.com/apache/nifi/pull/11872#discussion_r4482359592
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/components/connector/ConnectorRepository.java:
##########
@@ -125,10 +125,15 @@ public interface ConnectorRepository {
/**
* Starts the given Connector, managing any appropriate lifecycle events.
+ * When a flow evolution is pending, the context persists the flow after
phase one is applied, after the connector starts, and when the evolution fails.
+ * The context is required even when no evolution is pending.
+ *
* @param connector the Connector to start
+ * @param persistenceContext persists the flow during evolution
* @return a CompletableFuture that will be completed when the Connector
has started
+ * @throws NullPointerException if the persistence context is null
*/
- Future<Void> startConnector(ConnectorNode connector);
+ Future<Void> startConnector(ConnectorNode connector,
ConnectorUpdateContext persistenceContext);
Review Comment:
The feedback here is clear but I think keeping the old startConnector may
imply that flow evolution logic is optional. Removing the
ConnectorUpdateContext means we won't be able to save the flow following the
successful start unless we introduce the ConnectorUpdateContext to the
ConnectorRepository through another way. I did explore this a bit while
creating the PR and it is possible to pass the ability to persist at
construction. For example I could
[Opus 5.5]
FlowController now has a late-bound setFlowService (line 2530) and
saveFlowAfterConnectorEvolution (line 2547). That means the initialization
context could now carry this::saveFlowAfterConnectorEvolution, which looks up
the flow service only when a save happens. So "during construction" would now
work, with no setter and no cast.
Suggests the following pros/cons
**Pros**
1. startConnector(ConnectorNode) goes back to main's signature, so custom
repositories and their callers compile unchanged.
2. The ConnectorDAO and StandardNiFiServiceFacade changes go away, along
with the ConnectorAuditor pointcut change.
3. It also removes markap14's minor comment about two save paths
(r4473343087). Every evolution save would go through
flowService.saveFlowChanges() instead of a mix of that and
controllerFacade::saveImmediate.
4. No mutable field and no instanceof: the save function is fixed when the
repository is initialized, like the timeout.
**Cons**
1. It adds a method to ConnectorRepositoryInitializationContext. That is
another public interface change, though it can have a default.
2. The repository decides how to save, not the caller. Today a start from
the web layer saves immediately with saveImmediate. A built-in save function
would always use the flow service's path, so one action could follow two
different save rules.
3. applyUpdate would still take a context as an argument while
startConnector would not, which brings back the inconsistency the
thread_flow_persistence_context plan removed.
4. A save failure becomes harder to trace. With a parameter, the save that
fails is the one the caller passed in. 5. With a built-in function, a save that
failed because the flow service isn't set yet shows up as an
IllegalStateException deep in the coordinator.
6. Churn: the three save calls, the DAO, the facade, the auditor, and
TestStandardConnectorRepository all change again, after the last change reached
a stable state.
I am open to making this change but I think we should agree on whether
evolution should be an optional implementation in the repository. I think it
should be requried.
--
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]
