kevdoran commented on code in PR #11231:
URL: https://github.com/apache/nifi/pull/11231#discussion_r3221546225
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/components/connector/StandardConnectorRepository.java:
##########
@@ -459,19 +459,21 @@ private void purgeConnectorAndAwait(final ConnectorNode
connector) {
}
@Override
- public ConnectorNode getConnector(final String identifier) {
+ public ConnectorNode getConnector(final String identifier, final
ConnectorSyncMode syncMode) {
final ConnectorNode connector = connectors.get(identifier);
- if (connector != null) {
+ if (connector != null && syncMode ==
ConnectorSyncMode.SYNC_WITH_PROVIDER) {
syncFromProvider(connector);
}
return connector;
}
@Override
- public List<ConnectorNode> getConnectors() {
+ public List<ConnectorNode> getConnectors(final ConnectorSyncMode syncMode)
{
final List<ConnectorNode> connectorList =
List.copyOf(connectors.values());
- for (final ConnectorNode connector : connectorList) {
- syncFromProvider(connector);
+ if (syncMode == ConnectorSyncMode.SYNC_WITH_PROVIDER) {
Review Comment:
This assumes that a `null` value for `syncMode` will default to
`LOCAL_ONLY`. It might be preferable to require that arg is not null.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/FlowController.java:
##########
@@ -2414,6 +2416,8 @@ public void startConnector(final ConnectorNode
connectorNode) {
writeLock.lock();
try {
if (initialized.get()) {
+ // The connector is about to begin running; ensure it starts
with the latest provider configuration.
+
connectorRepository.getConnector(connectorNode.getIdentifier(),
ConnectorSyncMode.SYNC_WITH_PROVIDER);
connectorRepository.startConnector(connectorNode);
Review Comment:
Starting a connector only utilized the activeFlowContext, which is not
managed by the `ConnectorConfigurationProvider`, so this additional
getConnector(...) call is not needed, can just start.
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/dao/impl/StandardConnectorDAO.java:
##########
@@ -125,43 +131,44 @@ public void deleteConnector(final String id) {
@Override
public void startConnector(final String id) {
- final ConnectorNode connector = getConnector(id);
+ // The connector is about to begin running; ensure it is started with
the latest provider configuration.
+ final ConnectorNode connector = requireConnector(id,
ConnectorSyncMode.SYNC_WITH_PROVIDER);
getConnectorRepository().startConnector(connector);
Review Comment:
Starting a connector only utilized the activeFlowContext, which is not
managed by the `ConnectorConfigurationProvider`, so this additional
getConnector(...) call is not needed, can just start.
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/dao/impl/StandardConnectorDAO.java:
##########
@@ -111,7 +115,9 @@ public ConnectorNode createConnector(final String type,
final String id, final B
@Override
public void updateConnector(final ConnectorDTO connectorDTO) {
- final ConnectorNode connector = getConnector(connectorDTO.getId());
+ // Rename writes the in-memory working configuration back to the
provider; sync first so we
+ // do not overwrite changes the provider made out-of-band with stale
local state.
+ final ConnectorNode connector = requireConnector(connectorDTO.getId(),
ConnectorSyncMode.SYNC_WITH_PROVIDER);
if (connectorDTO.getName() != null) {
getConnectorRepository().updateConnector(connector,
connectorDTO.getName());
}
Review Comment:
The sync happens in the repository, which is where it should happen for the
update. If you want to ensure the connector exists before calling
`getConnectorRepository().updateConnector(connector, connectorDTO.getName());`,
I would use the `LOCAL_ONLY` sync mode, because the full connector sync with
the provider will happen as part of calling `updateConnector1
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/FlowController.java:
##########
@@ -1507,7 +1508,8 @@ public void trigger(final ComponentNode component) {
LOG.info("Starting {} Connectors",
startConnectorsAfterInitialization.size());
for (final ConnectorNode connectorNode :
startConnectorsAfterInitialization) {
try {
- final ConnectorNode existingConnector =
connectorRepository.getConnector(connectorNode.getIdentifier());
+ // The connector is about to begin running; ensure it
starts with the latest provider configuration.
+ final ConnectorNode existingConnector =
connectorRepository.getConnector(connectorNode.getIdentifier(),
ConnectorSyncMode.SYNC_WITH_PROVIDER);
Review Comment:
Starting a connector only utilized the activeFlowContext, which is not
managed by the `ConnectorConfigurationProvider`, so this additional
getConnector(...) call is not needed, can just start.
--
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]
