markap14 commented on code in PR #6937:
URL: https://github.com/apache/nifi/pull/6937#discussion_r1102042670
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizer.java:
##########
@@ -612,18 +612,33 @@ private void synchronizeControllerServices(final
ProcessGroup group, final Versi
});
}
- private void removeMissingConnections(final ProcessGroup group, final
VersionedProcessGroup proposed, final Map<String, Connection>
connectionsByVersionedId) {
+ private void removeMissingConnections(final ProcessGroup group, final
VersionedProcessGroup proposed, final Map<String, Connection>
connectionsByVersionedId) { // remove ones with same id but different source as
well
final Set<String> connectionsRemoved = new
HashSet<>(connectionsByVersionedId.keySet());
+ final Set<String> connectionsRemovedDueToChangingSourceId = new
HashSet<>();
for (final VersionedConnection proposedConnection :
proposed.getConnections()) {
connectionsRemoved.remove(proposedConnection.getIdentifier());
}
+ for (final VersionedConnection proposedConnection :
proposed.getConnections()) {
+ if
(connectionsByVersionedId.containsKey(proposedConnection.getIdentifier())
+ &&
+
!proposedConnection.getSource().getId().equals(connectionsByVersionedId.get(proposedConnection.getIdentifier()).getSource().getVersionedComponentId())
Review Comment:
This line appears to be incorrect. IntelliJ is flagging this because the
operands for the `equals` method are not compatible - the lefthand side is a
`String` while the righthand side is an `Optional<String>`. As a result, this
conditional is always going to be `true` (due to the preceding `!`). This means
that for every VersionedConnection that is proposed, if the connection exists,
we will remove it. So if any connection in the flow has data this will fail. We
definitely need to fix this.
I think we can fix this by changing the call to `getVersionedComponentId()`
to `getVersionedComponentId().orElse(null)`.
But this is already a very complicated expression. Please extract out
appropriate variable names to make this more readable and add some comments to
explain what we're doing here. Perhaps:
```
// Check for any case where there's an existing connection whose ID
matches the proposed connection, but whose source doesn't match
// the proposed source ID. The source of a Connection should never
change from one component to another. However, there are cases
// in which the Versioned Component ID might change, in order to
avoid conflicts with sibling Process Groups. In such a case, we must remove
// the connection and create a new one, since we cannot simply
change the source in the same way that we can change the destination.
final Set<String> connectionsRemovedDueToChangingSourceId = new
HashSet<>();
for (final VersionedConnection proposedConnection :
proposed.getConnections()) {
final Connection existingConnection =
connectionsByVersionedId.get(proposedConnection.getIdentifier());
if (existingConnection != null) {
final String proposedSourceId =
proposedConnection.getSource().getId();
final String existingSourceId =
existingConnection.getSource().getVersionedComponentId().orElse(null);
if (!Objects.equals(proposedSourceId, existingSourceId)) {
connectionsRemovedDueToChangingSourceId.add(proposedConnection.getIdentifier());
connectionsRemoved.add(proposedConnection.getIdentifier());
}
}
}
```
--
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]
