Skip to content

Conversation

mosermw
Copy link
Member

@mosermw mosermw commented May 21, 2025

Summary

NIFI-14442

This PR collects bundle version differences between a local Versioned Process Group and its FlowRegistry version.
These differences will only appear after another local change has been made. Bundle version differences still do not
count when determining a Versioned Process Group's state.

This does not handle the case when a Versioned Process Group is imported from a FlowRegistry and then local changes
are immediately made. In this scenario, there will be no bundle version differences. This is because that Process Group's
versioned snapshot is modified from what is in the FlowRegistry, to update bundle versions and references to controller services
and parameter providers. However, if you restart NiFi, the PG's versioned snapshot is pulled from the FlowRegisry on
startup, and then bundle version differences will be shown.

Tracking

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

Issue Tracking

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 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 mvn clean install -P contrib-check
    • JDK 21

Licensing

  • [n/a] New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • [n/a] New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

@NissimShiman
Copy link
Contributor

Reviewing...

@joewitt
Copy link
Contributor

joewitt commented Jun 19, 2025

@bbende Can you take a look from a review perspective?

@NissimShiman
Copy link
Contributor

I think letting users know that a component bundle update is occurring is a nice addition. Especially the way it is being done conditionally.
I've noticed that the list of local changes has a "Go To" option next to the "Component Bundle Changed" which doesn't lead anywhere. Would it be a big deal to remove that option for this?

Also noticed that if a versioned PG is in flow, NiFi is restarted, PG is updated (and new version pushed to registry) and then is rolled back and then a new change is made (but not committed yet) , the component bundle update does not show up as a local change, but if NiFi is again restarted it will. Is this expected?

@mosermw
Copy link
Member Author

mosermw commented Jun 25, 2025

Thanks for having a look at this PR @NissimShiman

I've noticed that the list of local changes has a "Go To" option next to the "Component Bundle Changed" which doesn't lead anywhere. Would it be a big deal to remove that option for this?

I was able to get something to work, then I noticed there is an open PR for NIFI-14637 that does almost exactly what I was going to do. So I think we can wait for that PR to merge, then I can modify this PR to rebase and take advantage of that code.

Also noticed that if a versioned PG is in flow, NiFi is restarted, PG is updated (and new version pushed to registry) and then is rolled back and then a new change is made (but not committed yet) , the component bundle update does not show up as a local change, but if NiFi is again restarted it will. Is this expected?

Yes it's expected. This is because the ProcessGroup stores a copy of its versioned snapshot, which typically has already been modified from what's in the FlowRegistry.

@mosermw
Copy link
Member Author

mosermw commented Jun 25, 2025

I've noticed that the list of local changes has a "Go To" option next to the "Component Bundle Changed" which doesn't lead anywhere. Would it be a big deal to remove that option for this?

I was able to get something to work, then I noticed there is an open PR for NIFI-14637 that does almost exactly what I was going to do. So I think we can wait for that PR to merge, then I can modify this PR to rebase and take advantage of that code.

The PR 10028 got merged, so I rebased this PR and added the condition to prevent showing the "Go To" action for bundle version differences.

@NissimShiman
Copy link
Contributor

NissimShiman commented Jul 1, 2025

I have tested the "Go To" option will not appear for these changes (and looked over the code as well).
Thank you @mosermw for resolving that.

This now performs as described in the jira (with the known edge case in the comment still present).

This lets the user know in many cases that a bundle version change is about to be persisted to registry, but maybe the edge case could be worked out as well.

More work would be needed so a process group's versioned snapshot should not be modified from what is in the FlowRegistry to make this edge case work as well, but that is a lot of work.

@bbende Is this something that would make sense to be done as part of this PR? or this something that should be left alone?
(and, if it should be left alone, is the PR good as is? If we are OK leaving the edge case out, I am +1 on this.)

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for proposing this improvement to highlighting bundle version differences @mosermw. The concept seems useful in some scenarios, but I'm not sure about the implementation, particularly reformatting the structure in the DtoFactory. It seems like this should be implemented elsewhere as part of the comparison process.

return differencesByComponent.keySet();
}

private static final Pattern DIFFERENCE_DESCRIPTION_PATTERN = Pattern.compile(".*artifact='(.*)'.*version='(.*)'].*version='(.*)']'$");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introducing this pattern to reformat the description seems potentially brittle. It would be better to either change it elsewhere, or build the description using values available from the FlowDifference.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate your eyes on this PR @exceptionfactory. This was the least intrusive approach I could think of at the time, but I agree it is potentially brittle. I will see what I can do to make improvements.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into alternative approaches, sounds good.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this code to the ConciseEvolvingDifferenceDescriptor class where some other difference descriptions are generated, and I do think this made the code cleaner.

Should I rebase this PR onto main, to try to resolve the macos build/test issues in the CI checks? Or can those checks be ignored for now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, rebasing would be a good next step.

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @mosermw. I noted a few minor questions and comments, but this looks closer to completion.

|| isLogFileSuffixChange(difference);
}

public static boolean isBundleChange(final FlowDifference difference) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only seems to be used once, is it necessary?

Comment on lines +2854 to +2856
// bundle difference could apply to many components, but use the Process Group identifier as
// the Component Identifier here so that many similar component differences can be easily deduped
dto.setComponentId(component.getGroupIdentifier());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems questionable, would it hide multiple component version differences in a given Process Group?


if (!differencesByComponent.isEmpty()) {
// differences were found, so now let's add back in any BUNDLE_CHANGED differences
// since they were initially filtered out as an environment-specific change
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that bundle changes are no longer considered environment-specific changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants