markap14 commented on PR #11670:
URL: https://github.com/apache/nifi/pull/11670#issuecomment-6001979759
[claude-opus 5.5]
Thanks for digging into this, @awelless. The Jira lays out the problem
clearly, and the system test reproduces it well. I'd like to suggest a much
simpler direction for the fix, though. The current approach adds about 235
lines of matching logic to `StandardVersionedComponentSynchronizer`, which is
already one of the most complex classes in the framework. I think the same
result can be reached with about a dozen lines of logic spread across two
existing methods.
## Why the service gets replaced
When a new flow version is applied, the synchronizer matches local
components to the components in that version purely by Versioned Component ID.
A service created by `migrateProperties` has no Versioned Component ID, so the
synchronizer falls back to an ID generated from the service's instance ID. The
instance ID is built from the Process Group's instance ID, and that differs
between the NiFi instance where the flow author committed the flow and the NiFi
instance that later imports it. The two IDs never match, so the synchronizer
treats them as different services. It creates the declared service from
scratch, points the processor at it, and removes the local service along with
its state.
## Suggested approach
**1. Give migration-created services a deterministic Versioned Component ID
when they are created.**
In `StandardControllerServiceFactory.create`, set the Versioned Component ID
from the creator's Versioned Component ID, the implementation class name, and
the sorted property values. The creator's Versioned Component ID comes from the
versioned flow itself, so it is the same on every NiFi instance that runs the
flow, not just on every node of one cluster:
```java
serviceNode.setProperties(creationDetails.serviceProperties());
// The Versioned Component ID must be set before the service's own migration
so that any service it creates can derive its ID from this one.
determineVersionedServiceId(creationDetails.type(),
creationDetails.serviceProperties()).ifPresent(serviceNode::setVersionedComponentId);
```
```java
private Optional<String> determineVersionedServiceId(final String className,
final Map<String, String> propertyValues) {
final Optional<String> creatorVersionedId = switch (creator) {
case final ProcessorNode processor ->
processor.getVersionedComponentId();
case final ControllerServiceNode service ->
service.getVersionedComponentId();
default -> Optional.empty();
};
final SortedMap<String, String> sortedProperties = new
TreeMap<>(propertyValues);
return creatorVersionedId.map(versionedId -> {
final String componentDescription = versionedId + className +
sortedProperties;
return
UUID.nameUUIDFromBytes(componentDescription.getBytes(StandardCharsets.UTF_8)).toString();
});
}
```
Here is what then happens in the scenario from the Jira:
1. The flow author upgrades their NiFi. Migration creates the service with
the deterministic ID.
2. The author commits the next version. The flow mapper keeps an existing
Versioned Component ID, so the registry stores the service under that same ID.
3. Another NiFi instance imports the flow and upgrades its NARs. Migration
creates the service with the same ID.
4. That instance upgrades to the new version. The synchronizer finds a
service with a matching ID and updates it in place. The service keeps its
state, and the group ends up with one service, not two.
The existing synchronizer logic does all the matching. There is no need to
look up the referencing component, compare property names, order chains of
services, or pick a "first" match when there are several candidates.
**2. Don't remove migration-created services that the proposed flow does not
declare.**
This handles the second case in the Jira, where the new version only adds an
unrelated processor and never mentions the service:
```java
private void removeMissingControllerServices(final ProcessGroup group, final
VersionedProcessGroup proposed, final Map<String, ControllerServiceNode>
servicesByVersionedId) {
// Controller Services created by property migration are not removed
when the proposed flow does not declare them. A versioned flow
// may never declare such a service while still containing the component
that references it, and removing the service would discard its state.
final Map<String, ControllerServiceNode> removableServicesByVersionedId
= new HashMap<>();
for (final Map.Entry<String, ControllerServiceNode> entry :
servicesByVersionedId.entrySet()) {
if
(!StandardControllerServiceFactory.MIGRATION_CREATED_COMMENT.equals(entry.getValue().getComments()))
{
removableServicesByVersionedId.put(entry.getKey(),
entry.getValue());
}
}
final BiConsumer<ProcessGroup, ControllerServiceNode> componentRemoval =
(grp, service) ->
context.getControllerServiceProvider().removeControllerService(service);
removeMissingComponents(group, proposed, removableServicesByVersionedId,
VersionedProcessGroup::getControllerServices, componentRemoval);
}
```
## Why I'd prefer this
- **It is much smaller.** The change is about a dozen lines of logic in two
existing methods. Nearly all of the new code in
`StandardVersionedComponentSynchronizer` goes away.
- **It reuses the matching NiFi already does.** Matching by Versioned
Component ID is how every other component type works, so migration-created
services stop being a special case during updates.
- **Every node in a cluster reaches the same result.** In the current
approach, the tie-break for several local services matching one declared
service depends on iteration order over
`ProcessGroup.getControllerServices(false)`, which returns a `HashSet`.
Different nodes can attach the versioned ID to different local services. The
deterministic ID removes that tie-break.
- **There are fewer edge cases.** The Jira lists a long set of edge cases,
mostly about the matching heuristics. With this approach, either the IDs match
and the service is updated in place, or they don't and the service is kept.
One limitation to note: flow versions that are already in a registry won't
benefit. A migration-created service committed before this change has an ID
derived from the author's Process Group instance ID, which no other instance
can reproduce. Only versions committed after this change get the in-place
update. I think that trade-off is well worth the reduction in complexity.
## Tests
- **System test fixture.** The version 2 snapshot hard-codes
`99999999-8888-7777-6666-555555555555` as the declared service's ID. No NiFi
instance would ever produce that ID, so the test checks a scenario that can't
happen in practice. It would be better for the test to create version 2 the way
a real flow author would: upgrade the NARs, let migration create the service,
and commit that version to the registry. Then import it into a fresh group and
upgrade that group. If that's not practical, the fixture should at least use
the ID that the deterministic algorithm produces.
- **`assertBelongsToLocalFlowOnly`** asserts that the Versioned Component ID
is derived from the instance ID. That will no longer hold, and it ties the test
to how the ID is calculated rather than to the behavior we care about. The
behavior to check is that the service survives with its state and that the
group ends up with exactly one service, which `assertStorePreserved` already
covers.
- **`testFlowUpgradePreservesMigrationCreatedControllerService`** asserts
that the comments are empty after the upgrade. That reflects the hand-written
fixture rather than anything the framework guarantees, so I'd drop that
assertion.
- **`StandardVersionedComponentSynchronizerTest`.** Most of the roughly 300
new lines test the matching heuristics, which would no longer exist. I'd
replace them with an update to an existing test in
`TestStandardControllerServiceFactory` that confirms the Versioned Component ID
is set and comes out the same for the same creator, class, and properties. I'd
also add a single synchronizer test confirming that a migration-created service
the proposed flow doesn't declare is not removed.
- Moving `FileSystemFlowRegistryClient` into its own NAR so it is available
on both sides of the NAR swap makes sense, and I'd keep it.
I have a local version of the two production changes above if it helps. I'm
happy to look again once the PR moves in this direction.
--
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]
