[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/nifi/pull/1156


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[olegz]

Github user olegz commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86839871
  
--- Diff: 
nifi-nar-bundles/nifi-standard-services/nifi-hbase-client-service-api/src/main/java/org/apache/nifi/hbase/HBaseClientService.java
 ---
@@ -67,6 +67,14 @@
 .defaultValue("1")
 .build();
 
+PropertyDescriptor PHOENIX_CLIENT_JAR_LOCATION = new 
PropertyDescriptor.Builder()
+.name("Phoenix Client JAR Location")
+.description("The full path to the Phoenix client JAR. 
Required if Phoenix is installed on top of HBase.")
+.addValidator(StandardValidators.FILE_EXISTS_VALIDATOR)
+.expressionLanguageSupported(true)
+.dynamicallyModifiesClasspath(true)
--- End diff --

Fair enough


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[bbende]

Github user bbende commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86839525
  
--- Diff: 
nifi-nar-bundles/nifi-standard-services/nifi-hbase-client-service-api/src/main/java/org/apache/nifi/hbase/HBaseClientService.java
 ---
@@ -67,6 +67,14 @@
 .defaultValue("1")
 .build();
 
+PropertyDescriptor PHOENIX_CLIENT_JAR_LOCATION = new 
PropertyDescriptor.Builder()
+.name("Phoenix Client JAR Location")
+.description("The full path to the Phoenix client JAR. 
Required if Phoenix is installed on top of HBase.")
+.addValidator(StandardValidators.FILE_EXISTS_VALIDATOR)
+.expressionLanguageSupported(true)
+.dynamicallyModifiesClasspath(true)
--- End diff --

and to Matt's point, even if it did make the property become invalid on 
upgrade, that is considered acceptable on a minor release.


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[bbende]

Github user bbende commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86839408
  
--- Diff: 
nifi-nar-bundles/nifi-standard-services/nifi-hbase-client-service-api/src/main/java/org/apache/nifi/hbase/HBaseClientService.java
 ---
@@ -67,6 +67,14 @@
 .defaultValue("1")
 .build();
 
+PropertyDescriptor PHOENIX_CLIENT_JAR_LOCATION = new 
PropertyDescriptor.Builder()
+.name("Phoenix Client JAR Location")
+.description("The full path to the Phoenix client JAR. 
Required if Phoenix is installed on top of HBase.")
+.addValidator(StandardValidators.FILE_EXISTS_VALIDATOR)
+.expressionLanguageSupported(true)
+.dynamicallyModifiesClasspath(true)
--- End diff --

I don't see why it would be considered breaking... in most of the cases 
nothing is changing for the end user. Lets take DBCP Connection Pool, the same 
property exists before and after, its just the class loading code being moved 
from in the component to handled by the framework.


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86828524
  
--- Diff: 
nifi-nar-bundles/nifi-standard-services/nifi-hbase-client-service-api/src/main/java/org/apache/nifi/hbase/HBaseClientService.java
 ---
@@ -67,6 +67,14 @@
 .defaultValue("1")
 .build();
 
+PropertyDescriptor PHOENIX_CLIENT_JAR_LOCATION = new 
PropertyDescriptor.Builder()
+.name("Phoenix Client JAR Location")
+.description("The full path to the Phoenix client JAR. 
Required if Phoenix is installed on top of HBase.")
+.addValidator(StandardValidators.FILE_EXISTS_VALIDATOR)
+.expressionLanguageSupported(true)
+.dynamicallyModifiesClasspath(true)
--- End diff --

What do you mean by "breaking"? If it changes the property such that the 
processor becomes invalid, I think we are allowed to do that (the user just has 
to remove the old property), as long as the default behavior is the same (which 
it will be in this case since the new feature defaults to false). In any case 
I'm good with writing a separate Jira to consider the other components that do 
classloading stuff


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[olegz]

Github user olegz commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86827016
  
--- Diff: 
nifi-nar-bundles/nifi-standard-services/nifi-hbase-client-service-api/src/main/java/org/apache/nifi/hbase/HBaseClientService.java
 ---
@@ -67,6 +67,14 @@
 .defaultValue("1")
 .build();
 
+PropertyDescriptor PHOENIX_CLIENT_JAR_LOCATION = new 
PropertyDescriptor.Builder()
+.name("Phoenix Client JAR Location")
+.description("The full path to the Phoenix client JAR. 
Required if Phoenix is installed on top of HBase.")
+.addValidator(StandardValidators.FILE_EXISTS_VALIDATOR)
+.expressionLanguageSupported(true)
+.dynamicallyModifiesClasspath(true)
--- End diff --

@bbende but we won't be able to do most of it for existing components until 
we are working on the new major release, correct? What I mean is that those 
would be breaking changes.


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[bbende]

Github user bbende commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86825589
  
--- Diff: 
nifi-nar-bundles/nifi-standard-services/nifi-hbase-client-service-api/src/main/java/org/apache/nifi/hbase/HBaseClientService.java
 ---
@@ -67,6 +67,14 @@
 .defaultValue("1")
 .build();
 
+PropertyDescriptor PHOENIX_CLIENT_JAR_LOCATION = new 
PropertyDescriptor.Builder()
+.name("Phoenix Client JAR Location")
+.description("The full path to the Phoenix client JAR. 
Required if Phoenix is installed on top of HBase.")
+.addValidator(StandardValidators.FILE_EXISTS_VALIDATOR)
+.expressionLanguageSupported(true)
+.dynamicallyModifiesClasspath(true)
--- End diff --

Yes what you described is correct... by "use this capability" I meant the 
following...
- Determine if the given component needs copying of resources, if so apply 
@RequiresInstanceClassLoading
- Modify the property descriptor to use dynamicallyModifiesClasspath(true)
- Remove any code in the component that does anything with the ClassLoader


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[bbende]

Github user bbende commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86822779
  
--- Diff: 
nifi-nar-bundles/nifi-standard-services/nifi-hbase-client-service-api/src/main/java/org/apache/nifi/hbase/HBaseClientService.java
 ---
@@ -67,6 +67,14 @@
 .defaultValue("1")
 .build();
 
+PropertyDescriptor PHOENIX_CLIENT_JAR_LOCATION = new 
PropertyDescriptor.Builder()
+.name("Phoenix Client JAR Location")
+.description("The full path to the Phoenix client JAR. 
Required if Phoenix is installed on top of HBase.")
+.addValidator(StandardValidators.FILE_EXISTS_VALIDATOR)
+.expressionLanguageSupported(true)
+.dynamicallyModifiesClasspath(true)
--- End diff --

They could be... my thinking was to initially get this capability in place 
and then maybe the people who worked on those components could determine if 
they should be refactored to use this capability, or if there is some reason 
why they should be left as is.


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86808718
  
--- Diff: 
nifi-nar-bundles/nifi-standard-services/nifi-hbase-client-service-api/src/main/java/org/apache/nifi/hbase/HBaseClientService.java
 ---
@@ -67,6 +67,14 @@
 .defaultValue("1")
 .build();
 
+PropertyDescriptor PHOENIX_CLIENT_JAR_LOCATION = new 
PropertyDescriptor.Builder()
+.name("Phoenix Client JAR Location")
+.description("The full path to the Phoenix client JAR. 
Required if Phoenix is installed on top of HBase.")
+.addValidator(StandardValidators.FILE_EXISTS_VALIDATOR)
+.expressionLanguageSupported(true)
+.dynamicallyModifiesClasspath(true)
--- End diff --

There are other properties (such as Module Directory in ExecuteScript and 
InvokeScriptedProcessor, and one in JoltTransformJSON) that add JARs to the 
classpath, should they be updated as part of this PR to set 
"dynamicallyModifiesClasspath" to true?


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[olegz]

Github user olegz commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86527582
  
--- Diff: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractConfiguredComponent.java
 ---
@@ -141,48 +181,74 @@ 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();
+final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(name);
+String value = null;
+if (!descriptor.isRequired() && (value = 
properties.remove(descriptor)) != null) {
 
-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);
-}
-}
+if (descriptor.getControllerServiceDefinition() != null) {
+if (value != null) {
+final ControllerServiceNode oldNode = 
serviceProvider.getControllerServiceNode(value);
+if (oldNode != null) {
+oldNode.removeReference(this);
 }
+}
+}
 
-try {
-component.onPropertyModified(descriptor, value, 
null);
-} catch (final Exception e) {
-// nothing really to do here...
-}
+try {
+component.onPropertyModified(descriptor, value, null);
+} catch (final Exception e) {
+// nothing really to do here...
--- End diff --

Same as above, just doesn't look right


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[olegz]

Github user olegz commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86527362
  
--- Diff: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractConfiguredComponent.java
 ---
@@ -90,47 +107,80 @@ public void setAnnotationData(final String data) {
 }
 
 @Override
-public void setProperty(final String name, final String value) {
-if (null == name || null == value) {
-throw new IllegalArgumentException();
+public void setProperties(Map properties) {
+if (properties == null) {
+return;
 }
 
 lock.lock();
 try {
 verifyModifiable();
 
-try (final NarCloseable narCloseable = 
NarCloseable.withComponentNarLoader(component.getClass())) {
-final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(name);
-
-final String oldValue = properties.put(descriptor, value);
-if (!value.equals(oldValue)) {
-
-if (descriptor.getControllerServiceDefinition() != 
null) {
-if (oldValue != null) {
-final ControllerServiceNode oldNode = 
serviceProvider.getControllerServiceNode(oldValue);
-if (oldNode != null) {
-oldNode.removeReference(this);
-}
-}
-
-final ControllerServiceNode newNode = 
serviceProvider.getControllerServiceNode(value);
-if (newNode != null) {
-newNode.addReference(this);
+try (final NarCloseable narCloseable = 
NarCloseable.withComponentNarLoader(component.getClass(), id)) {
+final Set modulePaths = new LinkedHashSet<>();
+for (final Map.Entry entry : 
properties.entrySet()) {
+if (entry.getKey() != null && entry.getValue() == 
null) {
+removeProperty(entry.getKey());
+} else if (entry.getKey() != null) {
+setProperty(entry.getKey(), entry.getValue());
+
+// for any properties that dynamically modify the 
classpath, attempt to evaluate them for expression language
+final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(entry.getKey());
+if (descriptor.isDynamicClasspathModifier() && 
!StringUtils.isEmpty(entry.getValue())) {
+final StandardPropertyValue propertyValue = 
new StandardPropertyValue(entry.getValue(), null, variableRegistry);
+
modulePaths.add(propertyValue.evaluateAttributeExpressions().getValue());
 }
 }
+}
 
-try {
-component.onPropertyModified(descriptor, oldValue, 
value);
-} catch (final Exception e) {
-// nothing really to do here...
-}
+final boolean requiresInstanceClassLoading = 
ExtensionManager.requiresInstanceClassLoading(component.getClass().getTypeName());
+if (requiresInstanceClassLoading) {
+processClasspathModifiers(modulePaths);
+} else if (!requiresInstanceClassLoading && 
modulePaths.size() > 0) {
+// Component has property descriptors with 
dynamicallyModifiesClasspath set to true, but the class does not have 
@RequiresInstanceClassLoading
+logger.warn("One or more property descriptors intend 
to modify the classpath, " +
+"but component does not contain the {} 
annotation, classpath will not be modified",
+new Object[] 
{RequiresInstanceClassLoading.class.getName()});
 }
 }
 } finally {
 lock.unlock();
 }
 }
 
+// Keep setProperty/removeProperty private so that all calls go 
through setProperties
+private void setProperty(final String name, final String value) {
+if (null == name || null == value) {
+throw new IllegalArgumentException();
+}
+
+final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(name);
+
+final String oldValue = properties.put(descriptor, value);
+if (!value.equals(oldValue)) {
+
+if 

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[olegz]

Github user olegz commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86527207
  
--- Diff: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractConfiguredComponent.java
 ---
@@ -90,47 +107,80 @@ public void setAnnotationData(final String data) {
 }
 
 @Override
-public void setProperty(final String name, final String value) {
-if (null == name || null == value) {
-throw new IllegalArgumentException();
+public void setProperties(Map properties) {
+if (properties == null) {
+return;
 }
 
 lock.lock();
 try {
 verifyModifiable();
 
-try (final NarCloseable narCloseable = 
NarCloseable.withComponentNarLoader(component.getClass())) {
-final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(name);
-
-final String oldValue = properties.put(descriptor, value);
-if (!value.equals(oldValue)) {
-
-if (descriptor.getControllerServiceDefinition() != 
null) {
-if (oldValue != null) {
-final ControllerServiceNode oldNode = 
serviceProvider.getControllerServiceNode(oldValue);
-if (oldNode != null) {
-oldNode.removeReference(this);
-}
-}
-
-final ControllerServiceNode newNode = 
serviceProvider.getControllerServiceNode(value);
-if (newNode != null) {
-newNode.addReference(this);
+try (final NarCloseable narCloseable = 
NarCloseable.withComponentNarLoader(component.getClass(), id)) {
+final Set modulePaths = new LinkedHashSet<>();
+for (final Map.Entry entry : 
properties.entrySet()) {
+if (entry.getKey() != null && entry.getValue() == 
null) {
+removeProperty(entry.getKey());
+} else if (entry.getKey() != null) {
+setProperty(entry.getKey(), entry.getValue());
+
+// for any properties that dynamically modify the 
classpath, attempt to evaluate them for expression language
+final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(entry.getKey());
+if (descriptor.isDynamicClasspathModifier() && 
!StringUtils.isEmpty(entry.getValue())) {
+final StandardPropertyValue propertyValue = 
new StandardPropertyValue(entry.getValue(), null, variableRegistry);
+
modulePaths.add(propertyValue.evaluateAttributeExpressions().getValue());
 }
 }
+}
 
-try {
-component.onPropertyModified(descriptor, oldValue, 
value);
-} catch (final Exception e) {
-// nothing really to do here...
-}
+final boolean requiresInstanceClassLoading = 
ExtensionManager.requiresInstanceClassLoading(component.getClass().getTypeName());
+if (requiresInstanceClassLoading) {
+processClasspathModifiers(modulePaths);
+} else if (!requiresInstanceClassLoading && 
modulePaths.size() > 0) {
+// Component has property descriptors with 
dynamicallyModifiesClasspath set to true, but the class does not have 
@RequiresInstanceClassLoading
+logger.warn("One or more property descriptors intend 
to modify the classpath, " +
+"but component does not contain the {} 
annotation, classpath will not be modified",
+new Object[] 
{RequiresInstanceClassLoading.class.getName()});
 }
 }
 } finally {
 lock.unlock();
 }
 }
 
+// Keep setProperty/removeProperty private so that all calls go 
through setProperties
+private void setProperty(final String name, final String value) {
+if (null == name || null == value) {
+throw new IllegalArgumentException();
+}
--- End diff --

I see, but I still think we have to fix it


---
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 

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[olegz]

Github user olegz commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86525918
  
--- Diff: 
nifi-commons/nifi-utils/src/main/java/org/apache/nifi/util/file/classloader/ClassLoaderUtils.java
 ---
@@ -52,23 +85,33 @@ public static ClassLoader getCustomClassLoader(String 
modulePath, ClassLoader pa
 isUrl = false;
 }
 if (!isUrl) {
-File modulePath = new File(modulePathString);
+try {
+File modulePath = new File(modulePathString);
 
-if (modulePath.exists()) {
+if (modulePath.exists()) {
 
-
additionalClasspath.add(modulePath.toURI().toURL());
+
additionalClasspath.add(modulePath.toURI().toURL());
 
-if (modulePath.isDirectory()) {
-File[] files = 
modulePath.listFiles(filenameFilter);
+if (modulePath.isDirectory()) {
+File[] files = 
modulePath.listFiles(filenameFilter);
 
-if (files != null) {
-for (File jarFile : files) {
-
additionalClasspath.add(jarFile.toURI().toURL());
+if (files != null) {
+for (File jarFile : files) {
--- End diff --

Since this one is really a stylistic comment, will leave it up to you, but. 
. .
After the change to determine if it's a directory, calling it _jarFile_ 
doesn't make much sense. Also, given that claspath can also have non-JAR 
resources (e.g., configuration, property files etc.) I think simply calling it 
something like _cpEntry_ or _classpathEntry_ would be more appropriate. 




---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[bbende]

Github user bbende commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86226932
  
--- Diff: 
nifi-commons/nifi-utils/src/main/java/org/apache/nifi/util/file/classloader/ClassLoaderUtils.java
 ---
@@ -52,23 +86,29 @@ public static ClassLoader getCustomClassLoader(String 
modulePath, ClassLoader pa
 isUrl = false;
 }
 if (!isUrl) {
-File modulePath = new File(modulePathString);
+try {
+File modulePath = new File(modulePathString);
 
-if (modulePath.exists()) {
+if (modulePath.exists()) {
 
-
additionalClasspath.add(modulePath.toURI().toURL());
+
additionalClasspath.add(modulePath.toURI().toURL());
 
-if (modulePath.isDirectory()) {
-File[] files = 
modulePath.listFiles(filenameFilter);
+if (modulePath.isDirectory()) {
+File[] files = 
modulePath.listFiles(filenameFilter);
 
-if (files != null) {
-for (File jarFile : files) {
-
additionalClasspath.add(jarFile.toURI().toURL());
+if (files != null) {
+for (File jarFile : files) {
+
additionalClasspath.add(jarFile.toURI().toURL());
--- End diff --

I see what you mean, I'll add that check to see if it is a directory or 
file and handle accordingly


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[olegz]

Github user olegz commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r8678
  
--- 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 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 --

Agree with the first scenario.
As for the ```ClassLoaderUtils.getURLsForClasspath(...)```, I've read what 
you describe and I personally lean to the side of fail fast. Don't get me 
wrong; what you describe is perfectly valid, but I approach it very differently 
where the _fatal_ condition I am referring to will most definitely be a flow 
developer's error which will be caught during the development of the flow and 
therefore is not expected to happen in prod, so delegating to the component 
developer to do custom validation in each and every new/existing component 
seems like on overhead.


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[olegz]

Github user olegz commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86220275
  
--- Diff: 
nifi-commons/nifi-utils/src/main/java/org/apache/nifi/util/file/classloader/ClassLoaderUtils.java
 ---
@@ -52,23 +86,29 @@ public static ClassLoader getCustomClassLoader(String 
modulePath, ClassLoader pa
 isUrl = false;
 }
 if (!isUrl) {
-File modulePath = new File(modulePathString);
+try {
+File modulePath = new File(modulePathString);
 
-if (modulePath.exists()) {
+if (modulePath.exists()) {
 
-
additionalClasspath.add(modulePath.toURI().toURL());
+
additionalClasspath.add(modulePath.toURI().toURL());
 
-if (modulePath.isDirectory()) {
-File[] files = 
modulePath.listFiles(filenameFilter);
+if (modulePath.isDirectory()) {
+File[] files = 
modulePath.listFiles(filenameFilter);
 
-if (files != null) {
-for (File jarFile : files) {
-
additionalClasspath.add(jarFile.toURI().toURL());
+if (files != null) {
+for (File jarFile : files) {
+
additionalClasspath.add(jarFile.toURI().toURL());
--- End diff --

That's fine and is generally expected for classpath entries. However, I 
would consider NOT adding it to the ```additionalClasspath``` while logging a 
warning stating that recursive classpath hierarchies are not supported


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[bbende]

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 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 

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[bbende]

Github user bbende commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86213277
  
--- Diff: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractConfiguredComponent.java
 ---
@@ -90,47 +107,80 @@ public void setAnnotationData(final String data) {
 }
 
 @Override
-public void setProperty(final String name, final String value) {
-if (null == name || null == value) {
-throw new IllegalArgumentException();
+public void setProperties(Map properties) {
+if (properties == null) {
+return;
 }
 
 lock.lock();
 try {
 verifyModifiable();
 
-try (final NarCloseable narCloseable = 
NarCloseable.withComponentNarLoader(component.getClass())) {
-final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(name);
-
-final String oldValue = properties.put(descriptor, value);
-if (!value.equals(oldValue)) {
-
-if (descriptor.getControllerServiceDefinition() != 
null) {
-if (oldValue != null) {
-final ControllerServiceNode oldNode = 
serviceProvider.getControllerServiceNode(oldValue);
-if (oldNode != null) {
-oldNode.removeReference(this);
-}
-}
-
-final ControllerServiceNode newNode = 
serviceProvider.getControllerServiceNode(value);
-if (newNode != null) {
-newNode.addReference(this);
+try (final NarCloseable narCloseable = 
NarCloseable.withComponentNarLoader(component.getClass(), id)) {
+final Set modulePaths = new LinkedHashSet<>();
+for (final Map.Entry entry : 
properties.entrySet()) {
+if (entry.getKey() != null && entry.getValue() == 
null) {
+removeProperty(entry.getKey());
+} else if (entry.getKey() != null) {
+setProperty(entry.getKey(), entry.getValue());
+
+// for any properties that dynamically modify the 
classpath, attempt to evaluate them for expression language
+final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(entry.getKey());
+if (descriptor.isDynamicClasspathModifier() && 
!StringUtils.isEmpty(entry.getValue())) {
+final StandardPropertyValue propertyValue = 
new StandardPropertyValue(entry.getValue(), null, variableRegistry);
+
modulePaths.add(propertyValue.evaluateAttributeExpressions().getValue());
 }
 }
+}
 
-try {
-component.onPropertyModified(descriptor, oldValue, 
value);
-} catch (final Exception e) {
-// nothing really to do here...
-}
+final boolean requiresInstanceClassLoading = 
ExtensionManager.requiresInstanceClassLoading(component.getClass().getTypeName());
+if (requiresInstanceClassLoading) {
+processClasspathModifiers(modulePaths);
+} else if (!requiresInstanceClassLoading && 
modulePaths.size() > 0) {
+// Component has property descriptors with 
dynamicallyModifiesClasspath set to true, but the class does not have 
@RequiresInstanceClassLoading
+logger.warn("One or more property descriptors intend 
to modify the classpath, " +
+"but component does not contain the {} 
annotation, classpath will not be modified",
+new Object[] 
{RequiresInstanceClassLoading.class.getName()});
 }
 }
 } finally {
 lock.unlock();
 }
 }
 
+// Keep setProperty/removeProperty private so that all calls go 
through setProperties
+private void setProperty(final String name, final String value) {
+if (null == name || null == value) {
+throw new IllegalArgumentException();
+}
--- End diff --

That does seem weird, I was preserving existing behavior here:

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[bbende]

Github user bbende commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86212943
  
--- Diff: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractConfiguredComponent.java
 ---
@@ -90,47 +107,80 @@ public void setAnnotationData(final String data) {
 }
 
 @Override
-public void setProperty(final String name, final String value) {
-if (null == name || null == value) {
-throw new IllegalArgumentException();
+public void setProperties(Map properties) {
+if (properties == null) {
+return;
 }
 
 lock.lock();
 try {
 verifyModifiable();
 
-try (final NarCloseable narCloseable = 
NarCloseable.withComponentNarLoader(component.getClass())) {
-final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(name);
-
-final String oldValue = properties.put(descriptor, value);
-if (!value.equals(oldValue)) {
-
-if (descriptor.getControllerServiceDefinition() != 
null) {
-if (oldValue != null) {
-final ControllerServiceNode oldNode = 
serviceProvider.getControllerServiceNode(oldValue);
-if (oldNode != null) {
-oldNode.removeReference(this);
-}
-}
-
-final ControllerServiceNode newNode = 
serviceProvider.getControllerServiceNode(value);
-if (newNode != null) {
-newNode.addReference(this);
+try (final NarCloseable narCloseable = 
NarCloseable.withComponentNarLoader(component.getClass(), id)) {
+final Set modulePaths = new LinkedHashSet<>();
+for (final Map.Entry entry : 
properties.entrySet()) {
+if (entry.getKey() != null && entry.getValue() == 
null) {
+removeProperty(entry.getKey());
+} else if (entry.getKey() != null) {
+setProperty(entry.getKey(), entry.getValue());
+
+// for any properties that dynamically modify the 
classpath, attempt to evaluate them for expression language
+final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(entry.getKey());
+if (descriptor.isDynamicClasspathModifier() && 
!StringUtils.isEmpty(entry.getValue())) {
+final StandardPropertyValue propertyValue = 
new StandardPropertyValue(entry.getValue(), null, variableRegistry);
+
modulePaths.add(propertyValue.evaluateAttributeExpressions().getValue());
 }
 }
+}
 
-try {
-component.onPropertyModified(descriptor, oldValue, 
value);
-} catch (final Exception e) {
-// nothing really to do here...
-}
+final boolean requiresInstanceClassLoading = 
ExtensionManager.requiresInstanceClassLoading(component.getClass().getTypeName());
+if (requiresInstanceClassLoading) {
+processClasspathModifiers(modulePaths);
+} else if (!requiresInstanceClassLoading && 
modulePaths.size() > 0) {
--- End diff --

Good point, will remove the !requiresInstanceClassLoading check.


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[bbende]

Github user bbende commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86212718
  
--- Diff: 
nifi-commons/nifi-utils/src/main/java/org/apache/nifi/util/file/classloader/ClassLoaderUtils.java
 ---
@@ -24,23 +24,57 @@
 import java.net.URL;
 import java.net.URLClassLoader;
 import java.util.Arrays;
+import java.util.LinkedHashSet;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.Set;
 import java.util.stream.Collectors;
 
 public class ClassLoaderUtils {
 
 public static ClassLoader getCustomClassLoader(String modulePath, 
ClassLoader parentClassLoader, FilenameFilter filenameFilter) throws 
MalformedURLException {
-// Split and trim the module path(s)
-List modules = (modulePath == null)
+URL[] classpaths = getURLsForClasspath(modulePath, filenameFilter, 
false);
+return createModuleClassLoader(classpaths, parentClassLoader);
+}
+
+/**
+ *
+ * @param modulePath a module path to get URLs from, the module path 
may be a comma-separated list of paths
+ * @param filenameFilter a filter to apply when a module path is a 
directory and performs a listing, a null filter will return all matches
+ * @return an array of URL instances representing all of the modules 
resolved from processing modulePath
+ * @throws MalformedURLException if a module path does not exist
+ */
+public static URL[] getURLsForClasspath(String modulePath, 
FilenameFilter filenameFilter, boolean suppressExceptions) throws 
MalformedURLException {
+Set modules = (modulePath == null)
 ? null
-: 
Arrays.stream(modulePath.split(",")).filter(StringUtils::isNotBlank).map(String::trim).collect(Collectors.toList());
+: 
Arrays.stream(modulePath.split(",")).filter(StringUtils::isNotBlank).map(String::trim).collect(Collectors.toSet());
 
-URL[] classpaths = getURLsForClasspath(modules, filenameFilter);
-return createModuleClassLoader(classpaths, parentClassLoader);
+return getURLsForClasspathHelper(modules, filenameFilter, 
suppressExceptions);
 }
 
-protected static URL[] getURLsForClasspath(List modulePaths, 
FilenameFilter filenameFilter) throws MalformedURLException {
+/**
+ *
+ * @param modulePaths one or modules paths to get URLs from, each 
module path may be a comma-separated list of paths
+ * @param filenameFilter a filter to apply when a module path is a 
directory and performs a listing, a null filter will return all matches
+ * @param suppressExceptions if true then all modules will attempt to 
be resolved even if some throw an exception, if false the first exception will 
be thrown
+ * @return an array of URL instances representing all of the modules 
resolved from processing modulePaths
+ * @throws MalformedURLException if a module path does not exist
+ */
+public static URL[] getURLsForClasspath(Set modulePaths, 
FilenameFilter filenameFilter, boolean suppressExceptions) throws 
MalformedURLException {
+Set modules = new LinkedHashSet<>();
+if (modulePaths != null) {
+for (String modulePath : modulePaths) {
+modules.addAll(Arrays.stream(modulePath.split(","))
+.filter(StringUtils::isNotBlank)
+.map(String::trim)
+.collect(Collectors.toSet()));
+}
+}
+
+return getURLsForClasspathHelper(modules, filenameFilter, 
suppressExceptions);
+}
--- End diff --

Sure I'm open to cleaning up the code here.


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[bbende]

Github user bbende commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86213356
  
--- Diff: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractConfiguredComponent.java
 ---
@@ -90,47 +107,80 @@ public void setAnnotationData(final String data) {
 }
 
 @Override
-public void setProperty(final String name, final String value) {
-if (null == name || null == value) {
-throw new IllegalArgumentException();
+public void setProperties(Map properties) {
+if (properties == null) {
+return;
 }
 
 lock.lock();
 try {
 verifyModifiable();
 
-try (final NarCloseable narCloseable = 
NarCloseable.withComponentNarLoader(component.getClass())) {
-final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(name);
-
-final String oldValue = properties.put(descriptor, value);
-if (!value.equals(oldValue)) {
-
-if (descriptor.getControllerServiceDefinition() != 
null) {
-if (oldValue != null) {
-final ControllerServiceNode oldNode = 
serviceProvider.getControllerServiceNode(oldValue);
-if (oldNode != null) {
-oldNode.removeReference(this);
-}
-}
-
-final ControllerServiceNode newNode = 
serviceProvider.getControllerServiceNode(value);
-if (newNode != null) {
-newNode.addReference(this);
+try (final NarCloseable narCloseable = 
NarCloseable.withComponentNarLoader(component.getClass(), id)) {
+final Set modulePaths = new LinkedHashSet<>();
+for (final Map.Entry entry : 
properties.entrySet()) {
+if (entry.getKey() != null && entry.getValue() == 
null) {
+removeProperty(entry.getKey());
+} else if (entry.getKey() != null) {
+setProperty(entry.getKey(), entry.getValue());
+
+// for any properties that dynamically modify the 
classpath, attempt to evaluate them for expression language
+final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(entry.getKey());
+if (descriptor.isDynamicClasspathModifier() && 
!StringUtils.isEmpty(entry.getValue())) {
+final StandardPropertyValue propertyValue = 
new StandardPropertyValue(entry.getValue(), null, variableRegistry);
+
modulePaths.add(propertyValue.evaluateAttributeExpressions().getValue());
 }
 }
+}
 
-try {
-component.onPropertyModified(descriptor, oldValue, 
value);
-} catch (final Exception e) {
-// nothing really to do here...
-}
+final boolean requiresInstanceClassLoading = 
ExtensionManager.requiresInstanceClassLoading(component.getClass().getTypeName());
+if (requiresInstanceClassLoading) {
+processClasspathModifiers(modulePaths);
+} else if (!requiresInstanceClassLoading && 
modulePaths.size() > 0) {
+// Component has property descriptors with 
dynamicallyModifiesClasspath set to true, but the class does not have 
@RequiresInstanceClassLoading
+logger.warn("One or more property descriptors intend 
to modify the classpath, " +
+"but component does not contain the {} 
annotation, classpath will not be modified",
+new Object[] 
{RequiresInstanceClassLoading.class.getName()});
 }
 }
 } finally {
 lock.unlock();
 }
 }
 
+// Keep setProperty/removeProperty private so that all calls go 
through setProperties
+private void setProperty(final String name, final String value) {
+if (null == name || null == value) {
+throw new IllegalArgumentException();
+}
+
+final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(name);
+
+final String oldValue = properties.put(descriptor, value);
+if (!value.equals(oldValue)) {
+
+if 

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[bbende]

Github user bbende commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86212641
  
--- Diff: 
nifi-commons/nifi-utils/src/main/java/org/apache/nifi/util/file/classloader/ClassLoaderUtils.java
 ---
@@ -24,23 +24,57 @@
 import java.net.URL;
 import java.net.URLClassLoader;
 import java.util.Arrays;
+import java.util.LinkedHashSet;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.Set;
 import java.util.stream.Collectors;
 
 public class ClassLoaderUtils {
 
 public static ClassLoader getCustomClassLoader(String modulePath, 
ClassLoader parentClassLoader, FilenameFilter filenameFilter) throws 
MalformedURLException {
-// Split and trim the module path(s)
-List modules = (modulePath == null)
+URL[] classpaths = getURLsForClasspath(modulePath, filenameFilter, 
false);
+return createModuleClassLoader(classpaths, parentClassLoader);
+}
+
+/**
+ *
+ * @param modulePath a module path to get URLs from, the module path 
may be a comma-separated list of paths
+ * @param filenameFilter a filter to apply when a module path is a 
directory and performs a listing, a null filter will return all matches
+ * @return an array of URL instances representing all of the modules 
resolved from processing modulePath
+ * @throws MalformedURLException if a module path does not exist
+ */
+public static URL[] getURLsForClasspath(String modulePath, 
FilenameFilter filenameFilter, boolean suppressExceptions) throws 
MalformedURLException {
+Set modules = (modulePath == null)
 ? null
-: 
Arrays.stream(modulePath.split(",")).filter(StringUtils::isNotBlank).map(String::trim).collect(Collectors.toList());
+: 
Arrays.stream(modulePath.split(",")).filter(StringUtils::isNotBlank).map(String::trim).collect(Collectors.toSet());
 
-URL[] classpaths = getURLsForClasspath(modules, filenameFilter);
-return createModuleClassLoader(classpaths, parentClassLoader);
+return getURLsForClasspathHelper(modules, filenameFilter, 
suppressExceptions);
 }
 
-protected static URL[] getURLsForClasspath(List modulePaths, 
FilenameFilter filenameFilter) throws MalformedURLException {
+/**
+ *
+ * @param modulePaths one or modules paths to get URLs from, each 
module path may be a comma-separated list of paths
+ * @param filenameFilter a filter to apply when a module path is a 
directory and performs a listing, a null filter will return all matches
+ * @param suppressExceptions if true then all modules will attempt to 
be resolved even if some throw an exception, if false the first exception will 
be thrown
+ * @return an array of URL instances representing all of the modules 
resolved from processing modulePaths
+ * @throws MalformedURLException if a module path does not exist
+ */
+public static URL[] getURLsForClasspath(Set modulePaths, 
FilenameFilter filenameFilter, boolean suppressExceptions) throws 
MalformedURLException {
+Set modules = new LinkedHashSet<>();
+if (modulePaths != null) {
+for (String modulePath : modulePaths) {
+modules.addAll(Arrays.stream(modulePath.split(","))
+.filter(StringUtils::isNotBlank)
+.map(String::trim)
+.collect(Collectors.toSet()));
+}
+}
+
+return getURLsForClasspathHelper(modules, filenameFilter, 
suppressExceptions);
+}
+
+protected static URL[] getURLsForClasspathHelper(Set 
modulePaths, FilenameFilter filenameFilter, boolean suppressExceptions) throws 
MalformedURLException {
--- End diff --

Sure that would make sense.


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[bbende]

Github user bbende commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86212571
  
--- Diff: 
nifi-commons/nifi-utils/src/main/java/org/apache/nifi/util/file/classloader/ClassLoaderUtils.java
 ---
@@ -52,23 +86,29 @@ public static ClassLoader getCustomClassLoader(String 
modulePath, ClassLoader pa
 isUrl = false;
 }
 if (!isUrl) {
-File modulePath = new File(modulePathString);
+try {
+File modulePath = new File(modulePathString);
 
-if (modulePath.exists()) {
+if (modulePath.exists()) {
 
-
additionalClasspath.add(modulePath.toURI().toURL());
+
additionalClasspath.add(modulePath.toURI().toURL());
 
-if (modulePath.isDirectory()) {
-File[] files = 
modulePath.listFiles(filenameFilter);
+if (modulePath.isDirectory()) {
+File[] files = 
modulePath.listFiles(filenameFilter);
 
-if (files != null) {
-for (File jarFile : files) {
-
additionalClasspath.add(jarFile.toURI().toURL());
+if (files != null) {
+for (File jarFile : files) {
+
additionalClasspath.add(jarFile.toURI().toURL());
--- End diff --

Valid point, the current behavior is the module path can be a file, or a 
directory with files, but it doesn't recurse further into sub-directories. This 
was existing functionality that was written for the scripting processors that I 
was just re-using: 
https://github.com/apache/nifi/blob/master/nifi-commons/nifi-utils/src/main/java/org/apache/nifi/util/file/classloader/ClassLoaderUtils.java


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[olegz]

Github user olegz commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86159055
  
--- Diff: 
nifi-commons/nifi-utils/src/main/java/org/apache/nifi/util/file/classloader/ClassLoaderUtils.java
 ---
@@ -52,23 +86,29 @@ public static ClassLoader getCustomClassLoader(String 
modulePath, ClassLoader pa
 isUrl = false;
 }
 if (!isUrl) {
-File modulePath = new File(modulePathString);
+try {
+File modulePath = new File(modulePathString);
 
-if (modulePath.exists()) {
+if (modulePath.exists()) {
 
-
additionalClasspath.add(modulePath.toURI().toURL());
+
additionalClasspath.add(modulePath.toURI().toURL());
 
-if (modulePath.isDirectory()) {
-File[] files = 
modulePath.listFiles(filenameFilter);
+if (modulePath.isDirectory()) {
+File[] files = 
modulePath.listFiles(filenameFilter);
 
-if (files != null) {
-for (File jarFile : files) {
-
additionalClasspath.add(jarFile.toURI().toURL());
+if (files != null) {
+for (File jarFile : files) {
+
additionalClasspath.add(jarFile.toURI().toURL());
--- End diff --

What is those are directories as well?


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[olegz]

Github user olegz commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86160998
  
--- Diff: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractConfiguredComponent.java
 ---
@@ -90,47 +107,80 @@ public void setAnnotationData(final String data) {
 }
 
 @Override
-public void setProperty(final String name, final String value) {
-if (null == name || null == value) {
-throw new IllegalArgumentException();
+public void setProperties(Map properties) {
+if (properties == null) {
+return;
 }
 
 lock.lock();
 try {
 verifyModifiable();
 
-try (final NarCloseable narCloseable = 
NarCloseable.withComponentNarLoader(component.getClass())) {
-final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(name);
-
-final String oldValue = properties.put(descriptor, value);
-if (!value.equals(oldValue)) {
-
-if (descriptor.getControllerServiceDefinition() != 
null) {
-if (oldValue != null) {
-final ControllerServiceNode oldNode = 
serviceProvider.getControllerServiceNode(oldValue);
-if (oldNode != null) {
-oldNode.removeReference(this);
-}
-}
-
-final ControllerServiceNode newNode = 
serviceProvider.getControllerServiceNode(value);
-if (newNode != null) {
-newNode.addReference(this);
+try (final NarCloseable narCloseable = 
NarCloseable.withComponentNarLoader(component.getClass(), id)) {
+final Set modulePaths = new LinkedHashSet<>();
+for (final Map.Entry entry : 
properties.entrySet()) {
+if (entry.getKey() != null && entry.getValue() == 
null) {
+removeProperty(entry.getKey());
+} else if (entry.getKey() != null) {
+setProperty(entry.getKey(), entry.getValue());
+
+// for any properties that dynamically modify the 
classpath, attempt to evaluate them for expression language
+final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(entry.getKey());
+if (descriptor.isDynamicClasspathModifier() && 
!StringUtils.isEmpty(entry.getValue())) {
+final StandardPropertyValue propertyValue = 
new StandardPropertyValue(entry.getValue(), null, variableRegistry);
+
modulePaths.add(propertyValue.evaluateAttributeExpressions().getValue());
 }
 }
+}
 
-try {
-component.onPropertyModified(descriptor, oldValue, 
value);
-} catch (final Exception e) {
-// nothing really to do here...
-}
+final boolean requiresInstanceClassLoading = 
ExtensionManager.requiresInstanceClassLoading(component.getClass().getTypeName());
+if (requiresInstanceClassLoading) {
+processClasspathModifiers(modulePaths);
+} else if (!requiresInstanceClassLoading && 
modulePaths.size() > 0) {
--- End diff --

Isn't ```!requiresInstanceClassLoading``` redundant here? I mean it can 
only go to else if it's NOT.


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[olegz]

Github user olegz commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86164431
  
--- 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 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 am wondering if this should really be a fatal condition. Let's say I have 
a processor that utilizes this mechanism and the additional classpath is a 
required property (e.g., like we have in new JMS). This means it can *only* 
work if such classpath is provided and successfully loaded. Letting it continue 
with simple warning message wil not accomplish anything, right?


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[olegz]

Github user olegz commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86160430
  
--- Diff: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractConfiguredComponent.java
 ---
@@ -90,47 +107,80 @@ public void setAnnotationData(final String data) {
 }
 
 @Override
-public void setProperty(final String name, final String value) {
-if (null == name || null == value) {
-throw new IllegalArgumentException();
+public void setProperties(Map properties) {
+if (properties == null) {
+return;
 }
 
 lock.lock();
 try {
 verifyModifiable();
 
-try (final NarCloseable narCloseable = 
NarCloseable.withComponentNarLoader(component.getClass())) {
-final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(name);
-
-final String oldValue = properties.put(descriptor, value);
-if (!value.equals(oldValue)) {
-
-if (descriptor.getControllerServiceDefinition() != 
null) {
-if (oldValue != null) {
-final ControllerServiceNode oldNode = 
serviceProvider.getControllerServiceNode(oldValue);
-if (oldNode != null) {
-oldNode.removeReference(this);
-}
-}
-
-final ControllerServiceNode newNode = 
serviceProvider.getControllerServiceNode(value);
-if (newNode != null) {
-newNode.addReference(this);
+try (final NarCloseable narCloseable = 
NarCloseable.withComponentNarLoader(component.getClass(), id)) {
+final Set modulePaths = new LinkedHashSet<>();
+for (final Map.Entry entry : 
properties.entrySet()) {
+if (entry.getKey() != null && entry.getValue() == 
null) {
+removeProperty(entry.getKey());
+} else if (entry.getKey() != null) {
+setProperty(entry.getKey(), entry.getValue());
+
+// for any properties that dynamically modify the 
classpath, attempt to evaluate them for expression language
+final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(entry.getKey());
+if (descriptor.isDynamicClasspathModifier() && 
!StringUtils.isEmpty(entry.getValue())) {
+final StandardPropertyValue propertyValue = 
new StandardPropertyValue(entry.getValue(), null, variableRegistry);
+
modulePaths.add(propertyValue.evaluateAttributeExpressions().getValue());
 }
--- End diff --

It is probably a style preference but for me this seems a bit cleaner as 
opposed to having both IF and ELSE containing the same condition 
(```entry.getKey() != null```).
```
if (entry.getKey() != null){
if (entry.getValue() == null){
. . .
} else {
. . .
}
}
```


---
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.
---

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[olegz]

Github user olegz commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86163462
  
--- Diff: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractConfiguredComponent.java
 ---
@@ -90,47 +107,80 @@ public void setAnnotationData(final String data) {
 }
 
 @Override
-public void setProperty(final String name, final String value) {
-if (null == name || null == value) {
-throw new IllegalArgumentException();
+public void setProperties(Map properties) {
+if (properties == null) {
+return;
 }
 
 lock.lock();
 try {
 verifyModifiable();
 
-try (final NarCloseable narCloseable = 
NarCloseable.withComponentNarLoader(component.getClass())) {
-final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(name);
-
-final String oldValue = properties.put(descriptor, value);
-if (!value.equals(oldValue)) {
-
-if (descriptor.getControllerServiceDefinition() != 
null) {
-if (oldValue != null) {
-final ControllerServiceNode oldNode = 
serviceProvider.getControllerServiceNode(oldValue);
-if (oldNode != null) {
-oldNode.removeReference(this);
-}
-}
-
-final ControllerServiceNode newNode = 
serviceProvider.getControllerServiceNode(value);
-if (newNode != null) {
-newNode.addReference(this);
+try (final NarCloseable narCloseable = 
NarCloseable.withComponentNarLoader(component.getClass(), id)) {
+final Set modulePaths = new LinkedHashSet<>();
+for (final Map.Entry entry : 
properties.entrySet()) {
+if (entry.getKey() != null && entry.getValue() == 
null) {
+removeProperty(entry.getKey());
+} else if (entry.getKey() != null) {
+setProperty(entry.getKey(), entry.getValue());
+
+// for any properties that dynamically modify the 
classpath, attempt to evaluate them for expression language
+final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(entry.getKey());
+if (descriptor.isDynamicClasspathModifier() && 
!StringUtils.isEmpty(entry.getValue())) {
+final StandardPropertyValue propertyValue = 
new StandardPropertyValue(entry.getValue(), null, variableRegistry);
+
modulePaths.add(propertyValue.evaluateAttributeExpressions().getValue());
 }
 }
+}
 
-try {
-component.onPropertyModified(descriptor, oldValue, 
value);
-} catch (final Exception e) {
-// nothing really to do here...
-}
+final boolean requiresInstanceClassLoading = 
ExtensionManager.requiresInstanceClassLoading(component.getClass().getTypeName());
+if (requiresInstanceClassLoading) {
+processClasspathModifiers(modulePaths);
+} else if (!requiresInstanceClassLoading && 
modulePaths.size() > 0) {
+// Component has property descriptors with 
dynamicallyModifiesClasspath set to true, but the class does not have 
@RequiresInstanceClassLoading
+logger.warn("One or more property descriptors intend 
to modify the classpath, " +
+"but component does not contain the {} 
annotation, classpath will not be modified",
+new Object[] 
{RequiresInstanceClassLoading.class.getName()});
 }
 }
 } finally {
 lock.unlock();
 }
 }
 
+// Keep setProperty/removeProperty private so that all calls go 
through setProperties
+private void setProperty(final String name, final String value) {
+if (null == name || null == value) {
+throw new IllegalArgumentException();
+}
+
+final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(name);
+
+final String oldValue = properties.put(descriptor, value);
+if (!value.equals(oldValue)) {
+
+if 

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[olegz]

Github user olegz commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86157718
  
--- Diff: 
nifi-commons/nifi-utils/src/main/java/org/apache/nifi/util/file/classloader/ClassLoaderUtils.java
 ---
@@ -24,23 +24,57 @@
 import java.net.URL;
 import java.net.URLClassLoader;
 import java.util.Arrays;
+import java.util.LinkedHashSet;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.Set;
 import java.util.stream.Collectors;
 
 public class ClassLoaderUtils {
 
 public static ClassLoader getCustomClassLoader(String modulePath, 
ClassLoader parentClassLoader, FilenameFilter filenameFilter) throws 
MalformedURLException {
-// Split and trim the module path(s)
-List modules = (modulePath == null)
+URL[] classpaths = getURLsForClasspath(modulePath, filenameFilter, 
false);
+return createModuleClassLoader(classpaths, parentClassLoader);
+}
+
+/**
+ *
+ * @param modulePath a module path to get URLs from, the module path 
may be a comma-separated list of paths
+ * @param filenameFilter a filter to apply when a module path is a 
directory and performs a listing, a null filter will return all matches
+ * @return an array of URL instances representing all of the modules 
resolved from processing modulePath
+ * @throws MalformedURLException if a module path does not exist
+ */
+public static URL[] getURLsForClasspath(String modulePath, 
FilenameFilter filenameFilter, boolean suppressExceptions) throws 
MalformedURLException {
+Set modules = (modulePath == null)
 ? null
-: 
Arrays.stream(modulePath.split(",")).filter(StringUtils::isNotBlank).map(String::trim).collect(Collectors.toList());
+: 
Arrays.stream(modulePath.split(",")).filter(StringUtils::isNotBlank).map(String::trim).collect(Collectors.toSet());
 
-URL[] classpaths = getURLsForClasspath(modules, filenameFilter);
-return createModuleClassLoader(classpaths, parentClassLoader);
+return getURLsForClasspathHelper(modules, filenameFilter, 
suppressExceptions);
 }
 
-protected static URL[] getURLsForClasspath(List modulePaths, 
FilenameFilter filenameFilter) throws MalformedURLException {
+/**
+ *
+ * @param modulePaths one or modules paths to get URLs from, each 
module path may be a comma-separated list of paths
+ * @param filenameFilter a filter to apply when a module path is a 
directory and performs a listing, a null filter will return all matches
+ * @param suppressExceptions if true then all modules will attempt to 
be resolved even if some throw an exception, if false the first exception will 
be thrown
+ * @return an array of URL instances representing all of the modules 
resolved from processing modulePaths
+ * @throws MalformedURLException if a module path does not exist
+ */
+public static URL[] getURLsForClasspath(Set modulePaths, 
FilenameFilter filenameFilter, boolean suppressExceptions) throws 
MalformedURLException {
+Set modules = new LinkedHashSet<>();
+if (modulePaths != null) {
+for (String modulePath : modulePaths) {
+modules.addAll(Arrays.stream(modulePath.split(","))
+.filter(StringUtils::isNotBlank)
+.map(String::trim)
+.collect(Collectors.toSet()));
+}
+}
+
+return getURLsForClasspathHelper(modules, filenameFilter, 
suppressExceptions);
+}
--- End diff --

Since we're getting more into functional programming style and in spirit of 
removing duplication consider the following improvements to both versions of 
```getURLsForClasspath(..)``` methods.
```
public static URL[] getURLsForClasspath(String modulePath, FilenameFilter 
filenameFilter, boolean suppressExceptions) throws MalformedURLException {
return getURLsForClasspath(modulePath == null ? 
Collections.emptySet() : Collections.singleton(modulePath), filenameFilter, 
suppressExceptions);
}

public static URL[] getURLsForClasspath(Set modulePaths, 
FilenameFilter filenameFilter, boolean suppressExceptions) throws 
MalformedURLException {
Set modules = modulePaths == null ? null : 
modulePaths.stream()
.flatMap(path -> Arrays.stream(path.split(",")))
.filter(StringUtils::isNotBlank)
.map(String::trim)
.collect(Collectors.toSet());
return getURLsForClasspathHelper(modules, filenameFilter, 
suppressExceptions);
}
```


---
If your project is set up for it, you can reply to this email and have your

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[olegz]

Github user olegz commented on a diff in the pull request:

https://github.com/apache/nifi/pull/1156#discussion_r86161443
  
--- Diff: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractConfiguredComponent.java
 ---
@@ -90,47 +107,80 @@ public void setAnnotationData(final String data) {
 }
 
 @Override
-public void setProperty(final String name, final String value) {
-if (null == name || null == value) {
-throw new IllegalArgumentException();
+public void setProperties(Map properties) {
+if (properties == null) {
+return;
 }
 
 lock.lock();
 try {
 verifyModifiable();
 
-try (final NarCloseable narCloseable = 
NarCloseable.withComponentNarLoader(component.getClass())) {
-final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(name);
-
-final String oldValue = properties.put(descriptor, value);
-if (!value.equals(oldValue)) {
-
-if (descriptor.getControllerServiceDefinition() != 
null) {
-if (oldValue != null) {
-final ControllerServiceNode oldNode = 
serviceProvider.getControllerServiceNode(oldValue);
-if (oldNode != null) {
-oldNode.removeReference(this);
-}
-}
-
-final ControllerServiceNode newNode = 
serviceProvider.getControllerServiceNode(value);
-if (newNode != null) {
-newNode.addReference(this);
+try (final NarCloseable narCloseable = 
NarCloseable.withComponentNarLoader(component.getClass(), id)) {
+final Set modulePaths = new LinkedHashSet<>();
+for (final Map.Entry entry : 
properties.entrySet()) {
+if (entry.getKey() != null && entry.getValue() == 
null) {
+removeProperty(entry.getKey());
+} else if (entry.getKey() != null) {
+setProperty(entry.getKey(), entry.getValue());
+
+// for any properties that dynamically modify the 
classpath, attempt to evaluate them for expression language
+final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(entry.getKey());
+if (descriptor.isDynamicClasspathModifier() && 
!StringUtils.isEmpty(entry.getValue())) {
+final StandardPropertyValue propertyValue = 
new StandardPropertyValue(entry.getValue(), null, variableRegistry);
+
modulePaths.add(propertyValue.evaluateAttributeExpressions().getValue());
 }
 }
+}
 
-try {
-component.onPropertyModified(descriptor, oldValue, 
value);
-} catch (final Exception e) {
-// nothing really to do here...
-}
+final boolean requiresInstanceClassLoading = 
ExtensionManager.requiresInstanceClassLoading(component.getClass().getTypeName());
+if (requiresInstanceClassLoading) {
+processClasspathModifiers(modulePaths);
+} else if (!requiresInstanceClassLoading && 
modulePaths.size() > 0) {
+// Component has property descriptors with 
dynamicallyModifiesClasspath set to true, but the class does not have 
@RequiresInstanceClassLoading
+logger.warn("One or more property descriptors intend 
to modify the classpath, " +
+"but component does not contain the {} 
annotation, classpath will not be modified",
+new Object[] 
{RequiresInstanceClassLoading.class.getName()});
 }
 }
 } finally {
 lock.unlock();
 }
 }
 
+// Keep setProperty/removeProperty private so that all calls go 
through setProperties
+private void setProperty(final String name, final String value) {
+if (null == name || null == value) {
+throw new IllegalArgumentException();
+}
--- End diff --

Hmm. . . No message in the exception? Also, should we even check for nulls 
here given that it's private? meaning we have full control of when/how/where it 
is invoked?


---
If your project is set up for it, you can reply to this email and have your
reply 

[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

[bbende]

GitHub user bbende opened a pull request:

https://github.com/apache/nifi/pull/1156

NIFI-2909 and NIFI-1712 Per-Instance ClassLoading

Thank you for submitting a contribution to Apache NiFi.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced 
 in the commit message?

- [X] Does your PR title start with NIFI- where  is the JIRA number 
you are trying to resolve? Pay particular attention to the hyphen "-" character.

- [X] Has your PR been rebased against the latest commit within the target 
branch (typically master)?

- [X] Is your initial contribution a single, squashed commit?

### For code changes:
- [X] Have you ensured that the full suite of tests is executed via mvn 
-Pcontrib-check clean install at the root nifi folder?
- [X] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies 
licensed in a way that is compatible for inclusion under [ASF 
2.0](http://www.apache.org/legal/resolved.html#category-a)? 
- [ ] If applicable, have you updated the LICENSE file, including the main 
LICENSE file under nifi-assembly?
- [ ] If applicable, have you updated the NOTICE file, including the main 
NOTICE file found under nifi-assembly?
- [ ] If adding new Properties, have you added .displayName in addition to 
.name (programmatic access) for each of the new properties?

### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in 
which it is rendered?

### Note:
Please ensure that once the PR is submitted, you check travis-ci for build 
issues and submit an update to your PR as soon as possible.

## Summary of Changes

- Introduced a new annotation for processors, reporting tasks, controller 
services - @RequiresInstanceClassLoading
- Added new builder method to PropertyDescriptor 
dynamicallyModifiesClasspath(boolean)
- Created a new InstanceClassLoader that maintains a child-first internal 
ClassLoader where classpath resources can be set, only used when component has 
@RequiresInstanceClassLoading
- Added support to ExtensionManager to obtain ClassLoaders based on type + 
id, before we only had by type
- If the type being requested supports instance class loading (the 
annotation described above) it will create an InstanceClassLoader which starts 
as a copy of the NAR ClassLoader for that type
- When properties are set on a component, the framework will identify the 
properties that are classpath modifiers and add those resources to the 
InstanceClassLoader's child resources

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/bbende/nifi NIFI-1712

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/nifi/pull/1156.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1156


commit 493a6097d3a44337ab08cfac9b6f8e24b350b2a6
Author: Bryan Bende 
Date:   2016-10-10T13:27:57Z

NIFI-2909 Adding per-instance class loading capability through 
@RequiresInstanceClassLoading annotation
NIFI-1712 Applying per-instance class loading to HBaseClientService to 
allow specifying Phoenix Client JAR




---
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.
---