You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by zenfenan <gi...@git.apache.org> on 2018/04/24 15:44:13 UTC
[GitHub] nifi pull request #2653: NIFI-5073: JMSConnectionFactoryProvider now resolve...
GitHub user zenfenan opened a pull request:
https://github.com/apache/nifi/pull/2653
NIFI-5073: JMSConnectionFactoryProvider now resolves EL Expression
JMSConnectionFactoryProvider now resolves EL Expression from VariableRegistry.
**Summary**
- Replaced custom file validator with `StandardValidators.createURLorFileValidator`
- `BROKER_URI` was not properly evaluated. Fixed it.
- Added a unit test to confirm that EL expressions are properly evaluated and validated
---
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:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] 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.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
- [ ] 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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zenfenan/nifi NIFI-5073
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2653.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 #2653
----
commit ab297714e64536625bbbe2f60262fd56be26d64a
Author: zenfenan <si...@...>
Date: 2018-04-24T15:35:05Z
NIFI-5073: JMSConnectionFactoryProvider now resolves EL Expression from VariableRegistry
----
---
[GitHub] nifi pull request #2653: NIFI-5073: JMSConnectionFactoryProvider now resolve...
Posted by zenfenan <gi...@git.apache.org>.
Github user zenfenan commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2653#discussion_r184338421
--- Diff: nifi-nar-bundles/nifi-jms-bundle/nifi-jms-processors/src/main/java/org/apache/nifi/jms/cf/JMSConnectionFactoryProvider.java ---
@@ -97,7 +96,7 @@
.description("Path to the directory with additional resources (i.e., JARs, configuration files etc.) to be added "
+ "to the classpath. Such resources typically represent target MQ client libraries for the "
+ "ConnectionFactory implementation.")
- .addValidator(new ClientLibValidator())
+ .addValidator(StandardValidators.createListValidator(true, true, StandardValidators.createURLorFileValidator()))
--- End diff --
Yep. Thanks for pointing out. I have modified the processor to accept a comma separated list of paths that can be added to Classpath and moreover it would leverage `ClassLoaderUtils` thereby avoiding duplicate.
---
[GitHub] nifi pull request #2653: NIFI-5073: JMSConnectionFactoryProvider now resolve...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2653#discussion_r184179013
--- Diff: nifi-nar-bundles/nifi-jms-bundle/nifi-jms-processors/src/main/java/org/apache/nifi/jms/cf/JMSConnectionFactoryProvider.java ---
@@ -97,7 +96,7 @@
.description("Path to the directory with additional resources (i.e., JARs, configuration files etc.) to be added "
+ "to the classpath. Such resources typically represent target MQ client libraries for the "
+ "ConnectionFactory implementation.")
- .addValidator(new ClientLibValidator())
+ .addValidator(StandardValidators.createListValidator(true, true, StandardValidators.createURLorFileValidator()))
--- End diff --
I don't think this is the proper validation here. Changing it to support a list of files/directories/urls is probably a good idea. However, at present this processor expects that the configured value be a directory. I think we need to either update the processor to accept the list, or otherwise just use `StandardValidators.createDirectoryExistsValidator(true, false);`
---
[GitHub] nifi pull request #2653: NIFI-5073: JMSConnectionFactoryProvider now resolve...
Posted by zenfenan <gi...@git.apache.org>.
Github user zenfenan commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2653#discussion_r187790872
--- Diff: nifi-nar-bundles/nifi-jms-bundle/nifi-jms-processors/src/main/java/org/apache/nifi/jms/cf/JMSConnectionFactoryProvider.java ---
@@ -159,13 +159,15 @@ public void enable(ConfigurationContext context) throws InitializationException
if (logger.isInfoEnabled()) {
logger.info("Configuring " + this.getClass().getSimpleName() + " for '"
+ context.getProperty(CONNECTION_FACTORY_IMPL).evaluateAttributeExpressions().getValue() + "' to be connected to '"
- + BROKER_URI + "'");
+ + context.getProperty(BROKER_URI).evaluateAttributeExpressions().getValue() + "'");
}
+
// will load user provided libraries/resources on the classpath
- Utils.addResourcesToClasspath(context.getProperty(CLIENT_LIB_DIR_PATH).evaluateAttributeExpressions().getValue());
+ final String clientLibPath = context.getProperty(CLIENT_LIB_DIR_PATH).evaluateAttributeExpressions().getValue();
+ ClassLoader customClassLoader = ClassLoaderUtils.getCustomClassLoader(clientLibPath, this.getClass().getClassLoader(), null);
+ Thread.currentThread().setContextClassLoader(customClassLoader);
--- End diff --
Understood. I have updated `CLIENT_LIB_DIR_PATH` to include `.dynamicallyModifiesClasspath(true)`
---
[GitHub] nifi pull request #2653: NIFI-5073: JMSConnectionFactoryProvider now resolve...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2653
---
[GitHub] nifi pull request #2653: NIFI-5073: JMSConnectionFactoryProvider now resolve...
Posted by zenfenan <gi...@git.apache.org>.
Github user zenfenan commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2653#discussion_r190296123
--- Diff: nifi-nar-bundles/nifi-jms-bundle/nifi-jms-processors/src/main/java/org/apache/nifi/jms/cf/JMSConnectionFactoryProvider.java ---
@@ -159,13 +159,15 @@ public void enable(ConfigurationContext context) throws InitializationException
if (logger.isInfoEnabled()) {
logger.info("Configuring " + this.getClass().getSimpleName() + " for '"
+ context.getProperty(CONNECTION_FACTORY_IMPL).evaluateAttributeExpressions().getValue() + "' to be connected to '"
- + BROKER_URI + "'");
+ + context.getProperty(BROKER_URI).evaluateAttributeExpressions().getValue() + "'");
}
+
// will load user provided libraries/resources on the classpath
- Utils.addResourcesToClasspath(context.getProperty(CLIENT_LIB_DIR_PATH).evaluateAttributeExpressions().getValue());
+ final String clientLibPath = context.getProperty(CLIENT_LIB_DIR_PATH).evaluateAttributeExpressions().getValue();
+ ClassLoader customClassLoader = ClassLoaderUtils.getCustomClassLoader(clientLibPath, this.getClass().getClassLoader(), null);
+ Thread.currentThread().setContextClassLoader(customClassLoader);
--- End diff --
@markap14 Appreciate if you could take a look at this.
---
[GitHub] nifi pull request #2653: NIFI-5073: JMSConnectionFactoryProvider now resolve...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2653#discussion_r187131152
--- Diff: nifi-nar-bundles/nifi-jms-bundle/nifi-jms-processors/src/main/java/org/apache/nifi/jms/cf/JMSConnectionFactoryProvider.java ---
@@ -159,13 +159,15 @@ public void enable(ConfigurationContext context) throws InitializationException
if (logger.isInfoEnabled()) {
logger.info("Configuring " + this.getClass().getSimpleName() + " for '"
+ context.getProperty(CONNECTION_FACTORY_IMPL).evaluateAttributeExpressions().getValue() + "' to be connected to '"
- + BROKER_URI + "'");
+ + context.getProperty(BROKER_URI).evaluateAttributeExpressions().getValue() + "'");
}
+
// will load user provided libraries/resources on the classpath
- Utils.addResourcesToClasspath(context.getProperty(CLIENT_LIB_DIR_PATH).evaluateAttributeExpressions().getValue());
+ final String clientLibPath = context.getProperty(CLIENT_LIB_DIR_PATH).evaluateAttributeExpressions().getValue();
+ ClassLoader customClassLoader = ClassLoaderUtils.getCustomClassLoader(clientLibPath, this.getClass().getClassLoader(), null);
+ Thread.currentThread().setContextClassLoader(customClassLoader);
--- End diff --
The problem with this approach I think is that we're now creating the ClassLoader and using it to create the connection factory. However, when we do that, we would need to ensure that any access to the connection factory instance also is performed using the ClassLoader. Since all calls into this Controller Service could come from different threads, I think this is going to cause a problem.
I think the typical pattern here is to update the property descriptor of CLIENT_LIB_DIR_PATH to include .dynamicallyModifiesClassPath(true). In this case, the framework will automatically handle creating the appropriate ClassLoader for each instance of the controller service and will also ensure that the appropriate ClassLoader is set when any method on this Controller Service is invoked.
---
[GitHub] nifi issue #2653: NIFI-5073: JMSConnectionFactoryProvider now resolves EL Ex...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/2653
@zenfenan thanks for the update! The only thing I think that's missing is that you left in the code to change the ClassLoader, and that can be removed now because the framework is now managing the classloader. I removed it locally, tested, and all looks good. Good looks good. +1 merged to master. Thanks for the fix - and sorry it took so long to get back to it!
---