NIFI-15509: Enable ControllerService reload additional classpath resources if needed#10812
NIFI-15509: Enable ControllerService reload additional classpath resources if needed#10812bbende merged 1 commit intoapache:NIFI-15258from
Conversation
bobpaulin
force-pushed
the
NIFI-15509
branch
2 times, most recently
from
b1f45f7 to
0613a7e
Compare
| } | ||
|
|
||
| serviceNode.verifyCanEnable(); | ||
| serviceNode.reloadAdditionalResourcesIfNecessary(); |
There was a problem hiding this comment.
I think we can also remove a similar line for processors here:
|
|
||
| LOG.info("Starting {}", procNode); | ||
|
|
||
| if (procNode.isReloadAdditionalResourcesNecessary()) { |
There was a problem hiding this comment.
We shouldn't need to check isReloadAdditionalResourcesNecessary() because inside reloadAdditionalResourcesIfNecessary() it is doing the same logic and only reloading if necessary
There was a problem hiding this comment.
I agree we do not need to. I added the check because I wanted to avoid switching the thread classloader when it is not required. Performance wise probably very similar. There are only a few processors that use dynamic classloading so the idea was to skip the logic for the majority that don't need this functionality to reduce risk.
There was a problem hiding this comment.
That makes sense, I guess it ties to the other comment of whether we need the NarCloseable or not
There was a problem hiding this comment.
Oddly I could not reproduce so I've removed the NARCloseable and the if conditional. Thank you for the feedback @bbende the error must have originated from something else.
| LOG.info("Starting {}", procNode); | ||
|
|
||
| if (procNode.isReloadAdditionalResourcesNecessary()) { | ||
| try (final NarCloseable ignored = NarCloseable.withComponentNarLoader(extensionManager, procNode.getComponent().getClass(), procNode.getIdentifier())) { |
There was a problem hiding this comment.
I don't think we want to use the NarCloseable here because inside reloadAdditionalResourcesIfNecessary() it is going to be closing this ClassLoader and creating a new one
There was a problem hiding this comment.
I had originally written the code without it and I got an error that the code could not find one of the DBCP classes. I can remove it to provide the exact error but it does not appear to work properly without it.
There was a problem hiding this comment.
Ok I would be interested to see the stacktrace just to understand... if we do need to use NarCloseable somewhere, it seems like it should be further inside the reload method, since otherwise we would be wrapping the whole reload with the ClassLoader that is being removed half way through
There was a problem hiding this comment.
OK. Will dig into this a bit more and provide the error. I understand the concern let me see if perhaps there something unexpected about the current context classloader that's causing it.
| if (service.isReloadAdditionalResourcesNecessary()) { | ||
| try (final NarCloseable ignored = NarCloseable.withComponentNarLoader(extensionManager, service.getComponent().getClass(), service.getIdentifier())) { | ||
| service.reloadAdditionalResourcesIfNecessary(); | ||
| } |
There was a problem hiding this comment.
Same two comments from the procNode section
resources if needed
bbende
left a comment
bbende
left a comment
There was a problem hiding this comment.
+1 Thanks for looking into the issue and addressing the feedback, it looks good to me
4 participants
Summary
NIFI-15509
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