Github user bbende commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1156#discussion_r86216168
--- Diff:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractConfiguredComponent.java
---
@@ -141,48 +191,64 @@ public void setProperty(final String name, final
String value) {
* @return true if removed; false otherwise
* @throws java.lang.IllegalArgumentException if the name is null
*/
- @Override
- public boolean removeProperty(final String name) {
+ private boolean removeProperty(final String name) {
if (null == name) {
throw new IllegalArgumentException();
}
- lock.lock();
- try {
- verifyModifiable();
-
- try (final NarCloseable narCloseable =
NarCloseable.withComponentNarLoader(component.getClass())) {
- final PropertyDescriptor descriptor =
component.getPropertyDescriptor(name);
- String value = null;
- if (!descriptor.isRequired() && (value =
properties.remove(descriptor)) != null) {
-
- if (descriptor.getControllerServiceDefinition() !=
null) {
- if (value != null) {
- final ControllerServiceNode oldNode =
serviceProvider.getControllerServiceNode(value);
- if (oldNode != null) {
- oldNode.removeReference(this);
- }
- }
- }
+ final PropertyDescriptor descriptor =
component.getPropertyDescriptor(name);
+ String value = null;
+ if (!descriptor.isRequired() && (value =
properties.remove(descriptor)) != null) {
- try {
- component.onPropertyModified(descriptor, value,
null);
- } catch (final Exception e) {
- // nothing really to do here...
+ if (descriptor.getControllerServiceDefinition() != null) {
+ if (value != null) {
+ final ControllerServiceNode oldNode =
serviceProvider.getControllerServiceNode(value);
+ if (oldNode != null) {
+ oldNode.removeReference(this);
}
-
- return true;
}
}
- } finally {
- lock.unlock();
+
+ try {
+ component.onPropertyModified(descriptor, value, null);
+ } catch (final Exception e) {
+ // nothing really to do here...
+ }
+
+ return true;
}
+
return false;
}
+ /**
+ * Adds all of the modules identified by the given module paths to the
InstanceClassLoader for this component.
+ *
+ * @param modulePaths a list of module paths where each entry can be a
comma-separated list of multiple module paths
+ */
+ private void processClasspathModifiers(final Set<String> modulePaths) {
+ try {
+ final URL[] urls =
ClassLoaderUtils.getURLsForClasspath(modulePaths, null, true);
+
+ final ClassLoader classLoader =
Thread.currentThread().getContextClassLoader();
+ if (!(classLoader instanceof InstanceClassLoader)) {
+ // Really shouldn't happen, but if we somehow got here and
don't have an InstanceClassLoader then log a warning and move on
+ logger.warn("Unable to modify the classpath for {},
expected InstanceClassLoader, but found {}",
+ new Object[] { name,
classLoader.getClass().getName()});
+ return;
--- End diff --
I think there are two different scenarios to consider, and the important
thing to keep in mind is that this getting called from setProperties which is
when you hit Apply from the Configure screen of a processor, which it has to be
done this way because the classpath needs to be modified before any validation
calls which might rely on the classpath...
The logging statement above would indicate a bug in the code where somehow
`setProperties` got called and the framework didn't correctly set the context
ClassLoader prior to calling it. We could throw an exception here, but really
that doesn't do anything more than logger.warn/logger.error which both generate
a bulletin in the UI telling the user the classpath wasn't modified, at which
point they could choose not to start the processor.
The second scenario is when ClassLoaderUtils.getURLsForClasspath(...) is
called... right now it skips over resources it can't resolve. My thinking here
was that in the general case we should be more lenient and not blow up just
because 1 out of possibly many resources can't be found. If someone absolutely
needs every resource loaded, I believe they should implement a custom validator
for their property that could call getURLsForClasspath with suppression set to
false, so that it fails validation if it can't find something.
What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
