NIFI-16310 Preserve Controller Services created by migrateProperties during versioned flow synchronization by awelless · Pull Request #11670 · apache/nifi

awelless · 2026-09-11T14:31:53Z

Summary

Controller Services created by migrateProperties method were always recreated during a versioned process group upgrade, causing the trigger of @OnRemoved hook and loss of component state.
Now migration-created services are matched with their counterparts in a versioned process group. If match is successful, a local controller service assumes a versioned id without being recreated.

If no match happens, a controller services is kept unversioned, but not removed.

Verification

Synchronizer unit tests.
System tests.

Additional changes

FileSystemFlowRegistryClient which is used in system tests was moved into a separate NAR, to be available before and after NAR swap.

Suggested review order

MigrationCreatedControllerServiceVersioningIT and its dependencies to see the relevant use cases.
StandardVersionedComponentSynchronizer for actual implementation.
StandardVersionedComponentSynchronizerTest to see the desired behavior for edge cases.

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Apache NiFi Jira issue created

Pull Request Tracking

Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000
Pull request contains commits signed with a registered key indicating Verified status

Pull Request Formatting

Pull Request based on current revision of the main branch
Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

Build completed using ./mvnw clean install -P contrib-check
- JDK 21
- JDK 25

Licensing

New dependencies are compatible with the Apache License 2.0 according to the License Policy
New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

Documentation formatting appears as expected in rendered files

awelless · 2026-09-11T14:33:10Z

+ * can observe whether store contents survive flow and runtime upgrades.
+ */
+@InputRequirement(Requirement.INPUT_FORBIDDEN)
+public class MigrateToControllerService extends AbstractProcessor {


Initially I wanted to reuse MigrateProperties processor, but some of its modifications blocked flow upgrade, so I proceeded with a dedicated processor - controller service pair

awelless · 2026-09-11T14:34:37Z

+                    LOG.info("Keeping {} in {} because it was created by property migration and is not present in the proposed flow",
+                            service, group);


I reckon that we shouldn't just drop migration services if they're absent in a versioned flow. So that if we run into any edge case, the service is preserved and a user has a chance to resolve the issue.

exceptionfactory

Thanks for working on this issue @awelless. The Synchronizer changes have a number of nuances, and although the general direction seems sound, I plan on taking another pass through the approach. I noted a handful of recommendations, mostly focusing on the tests.

exceptionfactory · 2026-09-21T03:15:09Z

+/**
+ * Pre-upgrade shape of a processor that keeps its store location in a plain property.
+ * The post-upgrade shape of the same processor, in the alternate-config extensions bundle,
+ * migrates that property into a Controller Service.
+ */


It would be better to use this as the Capability Description annotation, instead of the class comment for semantic definition.

exceptionfactory · 2026-09-21T03:17:30Z

+ * declares the service, so the flow relies entirely on property migration to create it, and the later version only
+ * adds an unrelated processor.
+ */
+public class MigrationCreatedControllerServiceVersioningIT extends AbstractNarSwapMigrationIT {


For a new test class, the public modifiers are not needed at the class and method level.

exceptionfactory · 2026-09-21T03:19:59Z

+     * Polls the given condition until it holds, failing the test once the timeout elapses. Conditions query a NiFi that
+     * may still be starting up or replacing components, so a failing query is treated as the condition not holding yet.
+     */
+    private void waitForCondition(final ExceptionalBooleanSupplier condition, final String conditionDescription) throws InterruptedException {


This method appears to be duplicative of the waitFor() method in the base class.

Replaced waitForCondition with waitFor.
The difference is that we'll be waiting for up to 5 minutes for a condition instead of 30 seconds, but I don't think this is a concern.

exceptionfactory · 2026-09-21T03:22:12Z

+        getClientUtil().assertFlowUpToDate(flow.groupId());
+
+        final ControllerServiceEntity serviceAfterUpgrade = waitForSingleStoreService(flow.groupId());
+        assertEquals(DECLARED_SERVICE_VERSIONED_ID, serviceAfterUpgrade.getComponent().getVersionedComponentId(),


Many of these assertion methods are more verbose than necessary and should be shortened.

I cleaned up the assertions descriptions. I still left some of them to provide more details on what a failed assertions means

exceptionfactory · 2026-09-21T03:23:04Z

+    }
+
+    @Override
+    public synchronized void append(final String row) {


Why is this synchronized, is it necessary?

It was synchronized to ensure we don't have any race condition when incrementing ROW_COUNT_KEY.
Provided this is a test-only CS, and we don't access it concurrently in tests, I removed this modifier.

exceptionfactory · 2026-09-21T03:23:46Z

+        when(serviceNode.getCanonicalClassName()).thenReturn("ControllerServiceImpl");
+        when(serviceNode.getProperties()).thenReturn(Map.of(descriptor, new PropertyConfiguration("123", null, null, null)));
+        when(serviceNode.getRawPropertyValues()).thenReturn(Map.of(descriptor, "123"));
+        when(serviceNode.getVersionedComponentId()).thenReturn(Optional.empty());


Repeated literals in multiple methods should be declared as static final variables and reused.

I replaced literals with the existing constants.
I also introduced constants in the nested MigrationCreatedControllerService test

exceptionfactory · 2026-09-21T03:25:37Z

+     */
+    private void assignVersionedIdsToMigrationCreatedControllerServices(final ProcessGroup group, final VersionedProcessGroup proposed) {
+        final Collection<ControllerServiceNode> groupServices = group.getControllerServices(false);
+        if (groupServices == null || groupServices.isEmpty()) {


This method should never return null

Removed the null check

exceptionfactory · 2026-09-21T03:27:14Z

+            localService.getVersionedComponentId().ifPresent(claimedVersionedIds::add);
+        }
+
+        final Map<String, VersionedConfigurableExtension> proposedComponentsByVersionedId = indexByVersionedId(proposed.getControllerServices(), proposed.getProcessors());


This method is on the longer side, it looks an opportunity to break it up into two methods after the initial checks.

I extracted longer parts into separate methods. Listing migrationCreatedServices, claimedVersionedIds as well as version id assignment for migrationCreatedServices. Looks more readable now

…during versioned flow synchronization

awelless commented Sep 11, 2026

View reviewed changes

exceptionfactory requested changes Sep 21, 2026

View reviewed changes

awelless force-pushed the NIFI-16310-preserve-migration-created-controller-services branch 2 times, most recently from 46c85e0 to fe1d436 Compare September 22, 2026 14:24

awelless requested a review from exceptionfactory September 22, 2026 14:24

awelless added 2 commits September 23, 2026 09:49

NIFI-16310 Preserve Controller Services created by migrateProperties …

7dee95e

…during versioned flow synchronization

NIFI-16310 Address review comments

681fcf9

awelless force-pushed the NIFI-16310-preserve-migration-created-controller-services branch from fe1d436 to 681fcf9 Compare September 23, 2026 07:51

NIFI-16310 Align flow registry test module versions with 2.13.0-SNAPSHOT

0cdd262

awelless force-pushed the NIFI-16310-preserve-migration-created-controller-services branch from a471cf3 to 0cdd262 Compare September 23, 2026 11:22

		LOG.info("Keeping {} in {} because it was created by property migration and is not present in the proposed flow",
		service, group);

Conversation

awelless commented Sep 11, 2026

Summary

Verification

Additional changes

Suggested review order

Tracking

Issue Tracking

Pull Request Tracking

Pull Request Formatting

Verification

Build

Licensing

Documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exceptionfactory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants