-
Notifications
You must be signed in to change notification settings - Fork 2.9k
NIFI-15365: Allow Temporary Processor and Controller Services to verify. #10688
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
base: main
Are you sure you want to change the base?
Conversation
* Use new InstanceClassLoader to load additional classpath urls * Initialize component using reload component to set references from existing component * Allow Instance and InstanceClassLoader to be GC and closed by calling OnRemove
bac0f3b to
8be3d54
Compare
exceptionfactory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this improvement @bobpaulin.
On a structural review, I'm not sure that ReloadComponent is the right place to add the new methods. It might make sense to implement some the loading logic in the ProcessorNode and ControllerServiceNode classes themselves. That should also avoid the need for duplicative implementations in the Stateless and Standard ReloadComponents.
One fundamental note is that the ExtensionManager interface and corresponding implementation have methods for creating temporary components, including logic for handling Python Processors and working with the bundle ClassLoader. Decoupling the creation logic from the initialize call also seems important for clarity of behavior.
I can take a closer look soon, but those are some initial elements to consider.
I am also skeptical about ReloadComponent. However that is what the temporary component is doing. Its create a temporary instance to verify that will eventually be replace when the configuration is applied and the component is reloaded. It has all the object references required to swap in all the parts of the existing component.
These would all need to be wired into both of the above classes which in my opinion might introduce unintended coupling in the future. So while I do agree ReloadComponent is a little awkward I think it is better than opening up access to all these objects directly in the ProcessorNode and ControllerServiceNode classes.
Yes I noticed the TempComponents there first. My original plan was to go in that direction. The ExtensionManager faces similar challenges as StandardProcessorNode and StandardControllerServiceNode where there are additional classes that would need to be wired in to get access to state, keberos and FlowController. The current "TempComponents" use a MockControllerServiceInitializationContext for these pieces which might produce inconsistent behavior in the verify method as all 3 can be accessed within the ControllerService or Processor verify methods. This seems like a more appropriate place for the behavior but I was concerned with confusing these TempComponents with the existing ones. Not sure if just better naming could do the trick here but this seemed like a more significant change than using the ReloadComponent.
Appreciate the feedback @exceptionfactory! Not sure if there is another path to consider here but I think there are trade offs with all 3 possibilities above:
The options above are in my current preferred order but I'm open to changing. |
Summary
NIFI-15365
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Verified by calling verification api on DBCPConnectionPool Controller Service
Then verifying classes are unloaded via a heap dump and jmx.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation