Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NIFI-13720: Component is not reloaded when the isolation key depends on service property #9236

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mark-bathori
Copy link
Contributor

@mark-bathori mark-bathori commented Sep 6, 2024

Summary

To be able to create a system test for the PR the system-test-extension had to be restructured because when a component is annotated with @RequiresInstanceClassLoading and contains a controller service property then the framework does not allow the component to be in the same bundle as the service API.

NIFI-13720

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

  • 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

@turcsanyip turcsanyip self-requested a review September 10, 2024 14:03
Comment on lines 793 to 794
if (!StringUtils.equals(componentFingerprint, newFingerprint)) {
setComponentFingerprint(newFingerprint);
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest not renaming the variable/method from additionalResourcesFingerprint to componentFingerprint but leaving it as it is now.
To be fully consistent, the interface methods (isReloadAdditionalResourcesNecessary(), reloadAdditionalResourcesIfNecessary()) should be renamed too. However, I don't think it is worth changing these API methods.
Also, the isolation key was added in the "additional resources fingerprint" earlier so it was already a bit inconsistent name. It will be no better or worse due to the change in this PR.

Copy link
Contributor

@turcsanyip turcsanyip 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 fix @mark-bathori!

+1 LGTM

@markap14 Do you have any other idea to overcome the issue mentioned in the PR summary? (other than restructuring the system tests module)

To be able to create a system test for the PR the system-test-extension had to be restructured because when a component is annotated with @RequiresInstanceClassLoading and contains a controller service property then the framework does not allow the component to be in the same bundle as the service API.

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.

2 participants