markap14 commented on code in PR #11543:
URL: https://github.com/apache/nifi/pull/11543#discussion_r3786823911
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/StandardProcessorNode.java:
##########
@@ -2029,59 +2030,97 @@ public List<ConnectorMethod> getConnectorMethods() {
@Override
public String invokeConnectorMethod(final String methodName, final
Map<String, String> jsonArguments, final ProcessContext processContext) throws
InvocationFailedException {
- final ConfigurableComponent component = getComponent();
-
- try (final NarCloseable ignored =
NarCloseable.withComponentNarLoader(getExtensionManager(),
component.getClass(), getIdentifier())) {
- final Method implementationMethod =
discoverConnectorMethod(component.getClass(), methodName);
- final MethodArgument[] methodArguments =
getConnectorMethodArguments(methodName, implementationMethod, component);
- final List<Object> argumentValues = new ArrayList<>();
-
- for (final MethodArgument methodArgument : methodArguments) {
- if (ProcessContext.class.equals(methodArgument.type())) {
- continue;
- }
-
- final String jsonValue =
jsonArguments.get(methodArgument.name());
- if (jsonValue == null && methodArgument.required()) {
- throw new IllegalArgumentException("Cannot invoke
Connector Method '" + methodName + "' on " + this + " because the required
argument '"
- + methodArgument.name() + "' was not provided");
+ final boolean classpathDifferent =
isClasspathDifferent(processContext.getProperties());
Review Comment:
I don't think this check can do what it's intended to do on this code path,
and the one case where it does return `true` looks like a bug rather than a
feature.
`isClasspathDifferent` was written for configuration verification, where the
caller hands the framework a *proposed* set of property values, so asking
whether those differ from what is currently live is a meaningful question.
Connector method invocation has no proposed configuration:
`StandaloneProcessorFacade.invokeConnectorMethod` builds the context via
`componentContextProvider.createProcessContext(processorNode,
parameterContext)` with no property overrides. So this is comparing the live
configuration against itself, and the answer should always be "no difference."
There is one exception, and it is accidental.
`AbstractComponentNode.getEffectivePropertyValues()` seeds an entry for every
supported descriptor using `descriptor.getDefaultValue()`, while
`isClasspathDifferent` looks the current value up through
`getEffectivePropertyValue`, which returns `null` for a property that was never
explicitly set (`getProperty` returns `PropertyConfiguration.EMPTY`). An unset
property that carries a default therefore compares as changed: default value on
one side, `null` on the other.
The practical effect is that any component with a
`dynamicallyModifiesClasspath` property that has a default value and has not
been explicitly configured will be judged "classpath is different" on *every*
invocation, and will always be dispatched to a throwaway instance -- including
while the processor is running or the service is enabled.
`DefaultedDynamicallyModifyClasspath` in the system test extensions is exactly
that shape.
That matters beyond wasted work, because the temporary instance never has
`@OnScheduled` / `@OnEnabled` / `@OnConfigurationRestored` invoked, yet
`ConnectorMethod.allowedStates` defaults to include `RUNNING`. A method
explicitly declared as callable while running would silently be handed a fresh,
never-started instance.
Suggestion: either plumb real property overrides through the facade so the
comparison means something, or drop this half of the condition and let
`isReloadAdditionalResourcesNecessary()` make the decision on its own.
The same applies verbatim to the corresponding line in
`StandardControllerServiceNode.invokeConnectorMethod`.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/service/StandardControllerServiceNode.java:
##########
@@ -939,59 +940,95 @@ public void migrateConfiguration(final Map<String,
String> originalPropertyValue
@Override
public String invokeConnectorMethod(final String methodName, final
Map<String, String> jsonArguments, final ConfigurationContext
configurationContext) throws InvocationFailedException {
- final ConfigurableComponent component = getComponent();
-
- try (final NarCloseable ignored =
NarCloseable.withComponentNarLoader(getExtensionManager(),
component.getClass(), getIdentifier())) {
- final Method implementationMethod =
discoverConnectorMethod(component.getClass(), methodName);
- final MethodArgument[] methodArguments =
getConnectorMethodArguments(methodName, implementationMethod, component);
- final List<Object> argumentValues = new ArrayList<>();
-
- for (final MethodArgument methodArgument : methodArguments) {
- if (ConfigurationContext.class.equals(methodArgument.type())) {
- continue;
- }
-
- final String jsonValue =
jsonArguments.get(methodArgument.name());
- if (jsonValue == null && methodArgument.required()) {
- throw new IllegalArgumentException("Cannot invoke
Connector Method '" + methodName + "' on " + this + " because the required
argument '"
- + methodArgument.name()
+ "' was not provided");
+ final boolean classpathDifferent =
isClasspathDifferent(configurationContext.getProperties());
+
+ if (classpathDifferent || isReloadAdditionalResourcesNecessary()) {
+ LOG.debug("Classpath reload required for Connector Method
invocation. Create temporary InstanceClassLoader for {}", this);
+ final ExtensionManager extensionManager = getExtensionManager();
+ final Bundle bundle =
extensionManager.getBundle(getBundleCoordinate());
+ final Set<URL> classpathUrls =
getAdditionalClasspathResources(configurationContext.getProperties().keySet(),
+ descriptor ->
configurationContext.getProperty(descriptor).getValue());
+ final String classLoaderIsolationKey =
getClassLoaderIsolationKey(configurationContext);
+
+ final ClassLoader currentClassLoader =
Thread.currentThread().getContextClassLoader();
+ final InstanceClassLoader detectedClassLoader =
extensionManager.createInstanceClassLoader(getCanonicalClassName(),
getIdentifier(), bundle, classpathUrls, false,
+ classLoaderIsolationKey);
+ try {
+
Thread.currentThread().setContextClassLoader(detectedClassLoader);
+ final ControllerService tempControllerService =
componentInstanceFactory.createControllerServiceInstance(this,
detectedClassLoader);
Review Comment:
This failure escapes past every caller's error handling.
`createControllerServiceInstance` throws
`ControllerServiceInstantiationException`, which extends `RuntimeException`,
and nothing here catches it. The processor path does the right thing -- it
catches `ProcessorInstantiationException` and wraps it in
`InvocationFailedException` -- but there is no equivalent here.
That is unfortunate because failing to instantiate the service under the
temporary classloader is the single most likely failure mode of this feature: a
driver JAR that is missing, wrong, or incomplete, which is the exact scenario
NIFI-16201 describes. Callers are written against `InvocationFailedException`
(see `ClasspathVerifyConnector.invokeLoadClassMethod` and
`CalculateConnector`), so they would be bypassed entirely and the error would
surface as an unhandled runtime exception rather than a reported invocation
failure.
Suggestion: catch `ControllerServiceInstantiationException` here and wrap it
in `InvocationFailedException` with the method name and component, mirroring
the processor path.
--
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]
