sfc-gh-bbende commented on code in PR #11574:
URL: https://github.com/apache/nifi/pull/11574#discussion_r3831249597
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/components/connector/facades/standalone/StandaloneParameterContextFacade.java:
##########
@@ -95,8 +95,14 @@ public void updateParameters(final
Collection<ParameterValue> updatedValues) {
final Map<String, Parameter> updatedParameters =
createParameterMap(updatedValues);
managedProcessGroup.getParameterContext().setParameters(updatedParameters);
- allReferencingProcessors.forEach(ProcessorNode::resetValidationState);
+ // A component whose additional classpath resources are supplied by a
Parameter must be reloaded so that its ClassLoader reflects the
+ // updated Parameter values. Components that are not running are not
restarted below, so reloading here is the only point at which their
+ // ClassLoader is rebuilt.
+
allReferencingServices.forEach(ControllerServiceNode::reloadAdditionalResourcesIfNecessary);
+
allReferencingProcessors.forEach(ProcessorNode::reloadAdditionalResourcesIfNecessary);
Review Comment:
This reloads the directly referencing components, which is narrower than the
equivalent property-change path. `StandardControllerServiceNode.setProperties`
reloads recursively, because changing a Controller Service property can alter
the classloader isolation key of components that reference it:
```java
// It's possible that changing the properties of this Controller Service
could alter the Classloader Isolation Key of a referencing
// component so reload any referencing component as necessary.
getReferences().findRecursiveReferences(ComponentNode.class).forEach(ComponentNode::reloadAdditionalResourcesIfNecessary);
```
A Parameter update can change that same isolation-key-relevant service
property, so a stopped Processor that references one of these services will
still keep a stale `InstanceClassLoader` after this call. Active referencing
components are covered, since `activeSet` includes service references and gets
restarted below, but stopped ones are not. Worth applying the same recursive
reload for `allReferencingServices`.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/StandardProcessorNode.java:
##########
@@ -2082,6 +2082,11 @@ public String invokeConnectorMethod(final String
methodName, final Map<String, S
} catch (final Exception e) {
throw new InvocationFailedException(e);
}
+ } catch (final LinkageError e) {
+ // Argument types, return types, and the invoked method body can
all reference classes supplied by additional classpath
+ // resources. Those failures are LinkageError, not Exception, so
they would otherwise escape as an uncaught Error.
+ throw new InvocationFailedException("Failed to invoke Connector
Method '" + methodName + "' on " + this
+ + " because a class required by the component could not be
loaded from the component's ClassLoader", e);
Review Comment:
This `catch (LinkageError)` is mostly unreachable, and the comment describes
coverage that it doesn't actually provide:
- The `getDeclaredMethods()` path that this JIRA is about now throws from
`discoverConnectorMethod`, which this PR changed to wrap `LinkageError` into
`InvocationFailedException` itself. That error is converted one layer down and
never reaches here.
- "the invoked method body" is already handled today: a
`NoClassDefFoundError` thrown by the target method is wrapped by
`Method.invoke` into an `InvocationTargetException`, which is an `Exception`
and is caught by the existing inner `catch (final Exception e)` just above.
- The gap that does remain is not a `LinkageError`. Resolving
`methodArgument.type()` when that class is missing from the component's
ClassLoader throws `TypeNotPresentException`, a `RuntimeException` produced by
annotation `Class`-member resolution, so it still escapes unwrapped. The
`AsyncRequestManager` change keeps that from hanging the request, but the
caller gets a generic failure rather than an `InvocationFailedException`.
Suggest either dropping this block as redundant, or broadening it to also
cover `TypeNotPresentException` and tightening the comment to describe what it
really catches. The same applies to the identical block added to
`StandardControllerServiceNode`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
