You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by bbende <gi...@git.apache.org> on 2016/10/24 18:14:08 UTC
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
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-XXXX where XXXX 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 <bb...@apache.org>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on the issue:
https://github.com/apache/nifi/pull/1156
@olegz cool thanks! regarding the per-instance vs. per-lifecycle, in this first pass we are basically loading the resources at the time the properties are applied in order to be available during validation, I think in the future we can determine if we need something additional that is more tied to the component lifecycle.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
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<String, String> 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<String> modulePaths = new LinkedHashSet<>();
+ for (final Map.Entry<String, String> 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 not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by asfgit <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on the issue:
https://github.com/apache/nifi/pull/1156
@bbende thanks, will start looking
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on the issue:
https://github.com/apache/nifi/pull/1156
@bbende just left a few minor comments which probably all fall in the category of stylistic, so it's up to you to decide if you want to change anything.
Now I need to spend some time and play with it to see exactly how it works. Will do it over the weekend.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by bbende <gi...@git.apache.org>.
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<String, String> 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<String> modulePaths = new LinkedHashSet<>();
+ for (final Map.Entry<String, String> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on the issue:
https://github.com/apache/nifi/pull/1156
Reviewing. . .
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
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<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 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by bbende <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
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<String> 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<String> 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<String> 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<String> modulePaths, FilenameFilter filenameFilter, boolean suppressExceptions) throws MalformedURLException {
+ Set<String> 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<String> modulePaths, FilenameFilter filenameFilter, boolean suppressExceptions) throws MalformedURLException {
Set<String> 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
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by b-long <gi...@git.apache.org>.
Github user b-long commented on the issue:
https://github.com/apache/nifi/pull/1156
@olegz @bbende , et al. Any insight on this -> https://stackoverflow.com/questions/42938158/nifi-springcontextprocessor-using-configuration-instead-of-xml#comment84380723_42946116 ?
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by bbende <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by mattyb149 <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by bbende <gi...@git.apache.org>.
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<String, String> 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<String> modulePaths = new LinkedHashSet<>();
+ for (final Map.Entry<String, String> 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 (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 {
+ component.onPropertyModified(descriptor, oldValue, value);
+ } catch (final Exception e) {
+ // nothing really to do here...
--- End diff --
Same comment as above https://github.com/apache/nifi/blob/d838f61291d2582592754a37314911b701c6891b/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractConfiguredComponent.java#L125
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
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<String, String> 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<String> modulePaths = new LinkedHashSet<>();
+ for (final Map.Entry<String, String> 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 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1156#discussion_r86158326
--- 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<String> 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<String> 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<String> 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<String> modulePaths, FilenameFilter filenameFilter, boolean suppressExceptions) throws MalformedURLException {
+ Set<String> 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<String> modulePaths, FilenameFilter filenameFilter, boolean suppressExceptions) throws MalformedURLException {
--- End diff --
The name of the method is rather confusing. Have you thought of something like ```toURLs(..)```?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on the issue:
https://github.com/apache/nifi/pull/1156
@olegz just pushed up a commit with some changes from the last round of feedback (renaming jarFile, fixing empty catch block, fixing empty IllegalArgumentException).
Have you had a chance to test this out?
Also let me know if/when you want me to squash the commits
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by bbende <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by bbende <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by mattyb149 <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
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<String, String> 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<String> modulePaths = new LinkedHashSet<>();
+ for (final Map.Entry<String, String> 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 (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 {
+ component.onPropertyModified(descriptor, oldValue, value);
+ } catch (final Exception e) {
+ // nothing really to do here...
--- End diff --
Hmm. . . So if user implements this method in Processor and it throws exception don't we have to at the very least notify the user that it failed?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by bbende <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on the issue:
https://github.com/apache/nifi/pull/1156
@bbende I am actually in the process of testing it now, but giving that this is class loading, give till tomorrow and if all is good I'll merge. You can squash now, I am good with the changes you've made.
Also, probably lost in the maze of comments, but i don't think we ever closed the loop on per-instance vs, per-lifecycle point i've made earlier. What's your take on that?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by bbende <gi...@git.apache.org>.
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<String> 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<String> 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<String> 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<String> modulePaths, FilenameFilter filenameFilter, boolean suppressExceptions) throws MalformedURLException {
+ Set<String> 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<String> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by bbende <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on the issue:
https://github.com/apache/nifi/pull/1156
@olegz thanks for reviewing, I posted a couple of replies to the inline comments.
I also wanted to mention that after submitting this PR, I realized that a slight modification to the approach might open this feature up to a wider variety of use cases...
In the current PR, when the component is annotated with @RequiresInstanceClassLoading it creates an InstanceClassLoader which copies all of the URL resources from the NAR ClassLoader. This was necessary to solve the HBase + Phoenix problem, but it many other cases it may be acceptable to just make a new InstanceClassLoader with a parent of the NAR ClassLoader.
So I started working on a change where every single processor, cs, reporting task will get an InstanceClassLoader and by default it will be empty and have a parent of the NAR ClassLoader. Only components with the @RequiresInstanceClassLoading will force a full copy of the resources.
This should lets the HBaseControllerService use @RequiresInstanceClassLoading, and other components can still have properties that modify the classpath without making full copies.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by bbende <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on the issue:
https://github.com/apache/nifi/pull/1156
@olegz I pushed up a second commit that with the change that I described above, and also some of the changes from your feedback. Unfortunately I had already rebased my branch before you had started reviewing, so i ended up having to force push, but I left it as two commits where the first one has the stuff you already looked at, and the second commit has changes after that.
I think the remaining discussion point is mostly around how restrictive we get regarding what happens when one or more resources can't be found. I understand the point about what happens if something that is necessary can't be loaded, but I also want to give flexibility to the developers using this feature. There may be cases where someone wants to provide a couple of directories that are common locations for a given library, and if found then load whatever is found, but if not then maybe they don't care. One option might be to provide a validator that uses ClassLoader.getURLsforClasspath to validate everything resolves, and then document that this is available, but not enforce that it is always used.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by bbende <gi...@git.apache.org>.
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<String, String> 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<String> modulePaths = new LinkedHashSet<>();
+ for (final Map.Entry<String, String> 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:
https://github.com/apache/nifi/blob/d838f61291d2582592754a37314911b701c6891b/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractConfiguredComponent.java#L95
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
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<String, String> 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<String> modulePaths = new LinkedHashSet<>();
+ for (final Map.Entry<String, String> 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 (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 {
+ component.onPropertyModified(descriptor, oldValue, value);
+ } catch (final Exception e) {
+ // nothing really to do here...
--- End diff --
I see, but I still think we have to fix it (i know, it's the same comment as above ;) )
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on the issue:
https://github.com/apache/nifi/pull/1156
@bbende While i am still reviewing and testing, I think it is worth clarifying what per-instance really means. What I am trying to say is the fact that property values can be changed multiple times per single instance of the component and while it's ok for most properties, changing values for things like additional classpath will most likely result in unpredictable type resolution exceptions.
For example; If a JAR provided as property value is changed after a component already had a chance to run and certain classes were already loaded from the previous JAR while new versions (compatible or not) of the same classes are coming with the new JAR.
Alternatively, we can take a slightly different approach and address it similarly to the way it is addressed in SpringContextProcessor where additional classpath has no relation to the instance of the component and only exists while component is in active state (e.g., CS-enabled, Proc-started, etc.)
To summarize; we either have to document the limitations and side-effects of the per-instance approach or change it to per-activation.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
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<String, String> 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<String> modulePaths = new LinkedHashSet<>();
+ for (final Map.Entry<String, String> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by bbende <gi...@git.apache.org>.
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<String> 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<String> 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<String> 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<String> modulePaths, FilenameFilter filenameFilter, boolean suppressExceptions) throws MalformedURLException {
+ Set<String> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1156#discussion_r86222278
--- 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 --
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
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<String, String> 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<String> modulePaths = new LinkedHashSet<>();
+ for (final Map.Entry<String, String> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on the issue:
https://github.com/apache/nifi/pull/1156
@bbende I now tend to agree. Per-lifecycle class loading is an edge case and we only use it right now in SpringContextProcessor where it is required due to the fact that Spring itself is a developer framework and it's class path heavily depends on volatile artifacts (e.g., configuration files, application code etc.). So while most JARs will remain the same, the overall classpath may change after re-starts. For the cases where one deals with JMS ConectionFactories and JDBS Drivers that most likely never be the case.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Posted by olegz <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---