NIFI-15696 - Add Rebase operation for versioned process groups#10994
NIFI-15696 - Add Rebase operation for versioned process groups#10994pvillard31 wants to merge 3 commits into
Conversation
|
Automated review is marking this PR as stale due to lack of updates in the past four months. This PR will be closed in 15 days if the stale label is not removed. This stale label and automated closure does not indicate a judgement of the PR, just lack of reviewer bandwidth and helps us keep the PR queue more manageable. If you would like this PR re-opened you can do so and a committer can remove the stale label. Or you can open a new PR. Try to help review other PRs to increase PR review bandwidth which in turn helps yours. |
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for putting together this new feature @pvillard31. The general implementation pattern looks good, and the Handler strategy for various types of changes appears to provide a good foundation for iteration.
I noted a handful of syntactic recommendations, and raised a couple questions about concurrency.
| description = "For a Process Group that is under Version Control, this will perform a rebase analysis by comparing " | ||
| + "local modifications against upstream changes between the current version and the specified target version. " | ||
| + "The analysis determines whether a rebase is allowed or if there are conflicts.", |
There was a problem hiding this comment.
Multiline strings can be used instead of concatenation
| @SecurityRequirement(name = "Write - /process-groups/{uuid}"), | ||
| @SecurityRequirement(name = "Read - /{component-type}/{uuid} - For all encapsulated components"), | ||
| @SecurityRequirement(name = "Write - /{component-type}/{uuid} - For all encapsulated components"), | ||
| @SecurityRequirement(name = "Write - if the template contains any restricted components - /restricted-components"), |
There was a problem hiding this comment.
This reference to restricted components is no longer applicable and should be removed
| return new ClassifiedDifference(difference, RebaseClassification.COMPATIBLE, null, null); | ||
| } | ||
|
|
||
| public static ClassifiedDifference conflicting(final FlowDifference difference, final String conflictCode, final String conflictDetail) { |
There was a problem hiding this comment.
Should the conflictCode be declared as an enum for reuse across the implementation?
| import java.util.SortedSet; | ||
| import java.util.TreeSet; | ||
|
|
||
| public class RebaseEngine { |
There was a problem hiding this comment.
I recommend defining an interface and making this the standard implementation to clarify the public contract for engine operations
| case PROPERTY_CHANGED -> type.name() + ":" + componentId + ":" + fieldName.orElse(""); | ||
| case POSITION_CHANGED, SIZE_CHANGED, COMMENTS_CHANGED, DESCRIPTION_CHANGED -> type.name() + ":" + componentId; | ||
| case BENDPOINTS_CHANGED -> type.name() + ":" + componentId; | ||
| default -> type.name() + ":" + componentId + ":" + fieldName.orElse(""); |
There was a problem hiding this comment.
The concatenation here is a bit hard to read, recommend defining one or more format strings and reusing
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
|
|
||
| public class TestPropertyChangedRebaseHandler { |
There was a problem hiding this comment.
Since these are net-new classes, recommend using Test as the suffix instead of the prefix for the class. The public modifiers can also be removed
|
|
||
| @Test | ||
| public void testNoUpstreamConflict() { | ||
| final VersionedProcessor processor = createProcessorWithProperty("proc-a", "propX", "oldVal"); |
There was a problem hiding this comment.
There are a number of repeated string values in this test that should be moved to static final variables
| </dependency> | ||
| <dependency> | ||
| <groupId>com.fasterxml.jackson.core</groupId> | ||
| <artifactId>jackson-databind</artifactId> |
There was a problem hiding this comment.
Is this dependency needed if the ObjectMapper deep copy is removed?
| private static final int VALIDATION_WAIT_MILLIS = 50; | ||
| private static final String ROOT_PROCESS_GROUP = "RootProcessGroup"; | ||
|
|
||
| private final Map<String, VersionedProcessGroup> rebaseCleanSnapshots = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Are there any concerns about concurrent rebase requests for the same Process Group?
| final ProcessGroupEntity result = serviceFacade.updateProcessGroupContents(revision, groupId, versionControlInfo, flowSnapshot, idGenerationSeed, | ||
| verifyNotModified, false, updateDescendantVersionedFlows); | ||
|
|
||
| serviceFacade.resetVersionControlSnapshotAfterRebase(groupId); |
There was a problem hiding this comment.
It seems like a failure to update the Process Group contents would fail to reset the snapshot, leaving the snapshot entry in the Map within the Standard Service Facade. Perhaps putting this in a finally block would be sufficient?
Signed-off-by: Pierre Villard <pierre.villard.fr@gmail.com>
…rebase Review feedback (exceptionfactory): - Introduce RebaseEngine interface with StandardRebaseEngine implementation - Replace String conflict codes with RebaseConflictCode enum - Remove Jackson round-trip deep clone; engine now merges onto a target snapshot supplied by the caller, and the facade fetches a separate clean target copy for the VCI reset (drops jackson-databind dependency) - Guarantee the retained clean rebase snapshot is always released via a finally block; clearRebaseSnapshot separates cleanup from VCI reset - Use text blocks for the rebase endpoint operation descriptions - Reuse format-string constants for conflict keys and analysis fingerprint - Remove the non-applicable restricted-components security requirement - Fail fast when a component identifier cannot be resolved - Rename new test classes to the Test suffix, drop public modifiers, and extract repeated test string literals into constants Rebase adaptation: - Use VersionedComponentFlowMapper and the current StandardFlowComparator constructor; drop the removed ProcessGroup.getAncestorServiceIds usage Signed-off-by: Pierre Villard <pierre.villard.fr@gmail.com>
- Add a read-only RebaseEngine.classify() path so analysis does not mutate the target snapshot while execution still builds the merged snapshot - Reject local changes when the target version removed the affected component, and treat upstream property removal on the same component/property as a property conflict - Remove the request-node-only rebaseCleanSnapshots map and reset the Version Control Information to the clean target on every node using a dedicated rebase replication endpoint - Add system coverage for removed components, preserved local modifications, and the full rebase suite on a two-node cluster - Restore update request polling annotations and add defensive handler guards Signed-off-by: Pierre Villard <pierre.villard.fr@gmail.com>
|
Thanks for the review @exceptionfactory - it had been a while since I submitted the PR so I addressed your feedback + the rebase on a commit and I made some additional changes in another commit on top. |
Summary
NIFI-15696 - Add Rebase operation for versioned process groups
When a versioned process group is at version N with local modifications (state LOCALLY_MODIFIED_AND_STALE), upgrading to a newer version requires reverting all local changes first, then changing version, then manually re-applying every customization. This is error-prone, causes unnecessary downtime, and discourages users from upgrading.
This change adds a "Rebase" operation that upgrades a versioned process group to a newer registry version while automatically preserving compatible local changes — analogous to git rebase.
How it works:
The rebase operation has two phases:
Supported change types (for this first iteration, this will evolve over time):
Unsupported change types (examples: COMPONENT_ADDED, COMPONENT_REMOVED) block the rebase with a clear message. The handler architecture is extensible — adding support for a new change type requires only implementing a single RebaseHandler class.
We follow a similar approach as what already exists for the new API endpoints:
returns the three-way analysis
initiates async rebase execution
polling and cleanup (reuses FlowUpdateResource)
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000VerifiedstatusPull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation