You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nifi.apache.org by jfrazee <gi...@git.apache.org> on 2016/07/06 17:22:41 UTC
[GitHub] nifi pull request #609: NIFI-2142 Cache compiled XSLT in TransformXml
GitHub user jfrazee opened a pull request:
https://github.com/apache/nifi/pull/609
NIFI-2142 Cache compiled XSLT in TransformXml
Two other tidbits:
1. This also modifies the XSLT_FILENAME attribute to support expression language.
2. This updated the test files to have 3 space indents because Saxon defaults to that and AFAICT the free version doesn't let you change that -- it's either no indent or 3 space. Could be wrong though.
See https://github.com/apache/nifi/pull/591 for PR against 0.x and past discussion.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jfrazee/nifi NIFI-2142
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/609.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 #609
----
commit f595df477e971fd09c3a734f18a882af0d72818c
Author: Joey Frazee <jo...@icloud.com>
Date: 2016-06-28T22:29:43Z
Added Guava cache for XSLT stylesheets in TransformXml
----
---
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 #609: NIFI-2142 Cache compiled XSLT in TransformXml
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi/pull/609#discussion_r70738325
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java ---
@@ -76,25 +84,59 @@
.name("XSLT file name")
.description("Provides the name (including full path) of the XSLT file to apply to the flowfile XML content.")
.required(true)
+ .expressionLanguageSupported(true)
.addValidator(StandardValidators.FILE_EXISTS_VALIDATOR)
.build();
+ public static final PropertyDescriptor INDENT_OUTPUT = new PropertyDescriptor.Builder()
+ .name("indent-output")
+ .displayName("Indent")
+ .description("Whether or not to indent the output.")
+ .required(true)
+ .defaultValue("true")
+ .allowableValues("true", "false")
+ .addValidator(StandardValidators.BOOLEAN_VALIDATOR)
+ .build();
+
+ public static final PropertyDescriptor CACHE_SIZE = new PropertyDescriptor.Builder()
+ .name("cache-size")
+ .displayName("Cache size")
+ .description("Maximum size of the stylesheet cache.")
+ .required(true)
+ .defaultValue("100")
+ .addValidator(StandardValidators.NON_NEGATIVE_INTEGER_VALIDATOR)
+ .build();
+
+ public static final PropertyDescriptor CACHE_DURATION = new PropertyDescriptor.Builder()
+ .name("cache-duration")
+ .displayName("Cache duration")
+ .description("How long to keep stylesheets in the cache.")
--- End diff --
This property is misleading. What I initially thought it to be was how long the stylesheets will exist in the cache (deleted after 60s). What it actually is, is an expiration time where after 60s of it not being used it will be removed.
---
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 #609: NIFI-2142 Cache compiled XSLT in TransformXml
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/609
Hey @jfrazee, looks like you have contrib check issues that cause the build to fail:
[WARNING] src/main/java/org/apache/nifi/processors/standard/TransformXml.java[24:8] (imports) UnusedImports: Unused import - java.util.Collection.
[WARNING] src/test/java/org/apache/nifi/processors/standard/TestTransformXml.java[19] (imports) AvoidStarImport: Using the '.*' form of import should be avoided - java.io.*.
[WARNING] src/test/java/org/apache/nifi/processors/standard/TestTransformXml.java[36:8] (imports) UnusedImports: Unused import - org.junit.Ignore.
---
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 #609: NIFI-2142 Cache compiled XSLT in TransformXml
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/609
@jfrazee the image bundle is no longer in master (replaced by the media bundle). Do a "git clean -i -d" to remove any old bundles.
---
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 #609: NIFI-2142 Cache compiled XSLT in TransformXml
Posted by jfrazee <gi...@git.apache.org>.
Github user jfrazee commented on a diff in the pull request:
https://github.com/apache/nifi/pull/609#discussion_r71950331
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java ---
@@ -76,25 +84,59 @@
.name("XSLT file name")
.description("Provides the name (including full path) of the XSLT file to apply to the flowfile XML content.")
.required(true)
+ .expressionLanguageSupported(true)
.addValidator(StandardValidators.FILE_EXISTS_VALIDATOR)
.build();
+ public static final PropertyDescriptor INDENT_OUTPUT = new PropertyDescriptor.Builder()
+ .name("indent-output")
+ .displayName("Indent")
+ .description("Whether or not to indent the output.")
+ .required(true)
+ .defaultValue("true")
+ .allowableValues("true", "false")
+ .addValidator(StandardValidators.BOOLEAN_VALIDATOR)
+ .build();
+
+ public static final PropertyDescriptor CACHE_SIZE = new PropertyDescriptor.Builder()
+ .name("cache-size")
+ .displayName("Cache size")
+ .description("Maximum size of the stylesheet cache.")
+ .required(true)
+ .defaultValue("100")
+ .addValidator(StandardValidators.NON_NEGATIVE_INTEGER_VALIDATOR)
+ .build();
+
+ public static final PropertyDescriptor CACHE_DURATION = new PropertyDescriptor.Builder()
+ .name("cache-duration")
+ .displayName("Cache duration")
+ .description("How long to keep stylesheets in the cache.")
--- End diff --
@JPercivall It can be either. Do you have thoughts on which is preferable. If kept as is, how does "How long to keep stylesheets in the cache after last time of access" sound?
---
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 #609: NIFI-2142 Cache compiled XSLT in TransformXml
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi/pull/609#discussion_r72658835
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java ---
@@ -101,16 +101,16 @@
public static final PropertyDescriptor CACHE_SIZE = new PropertyDescriptor.Builder()
.name("cache-size")
.displayName("Cache size")
- .description("Maximum size of the stylesheet cache.")
+ .description("Maximum number of stylesheets to cache. Zero disables the cache.")
.required(true)
.defaultValue("100")
.addValidator(StandardValidators.NON_NEGATIVE_INTEGER_VALIDATOR)
.build();
- public static final PropertyDescriptor CACHE_DURATION = new PropertyDescriptor.Builder()
- .name("cache-duration")
- .displayName("Cache duration")
- .description("How long to keep stylesheets in the cache.")
+ public static final PropertyDescriptor CACHE_TTL_AFTER_LAST_ACCESS = new PropertyDescriptor.Builder()
+ .name("cache-ttl-after-last-access")
+ .displayName("Cache TTL after last access")
+ .description("How long to keep stylesheets in the cache after last access.")
--- End diff --
I like using TTL in the display name in order to make it more compact but not every user may know what TTL means so it'd nice to have the explanation in the description.
---
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 #609: NIFI-2142 Cache compiled XSLT in TransformXml
Posted by jfrazee <gi...@git.apache.org>.
Github user jfrazee commented on the issue:
https://github.com/apache/nifi/pull/609
1. @JPercivall I addressed all your comments above and added a template to https://gist.github.com/jfrazee/a501336826aa77d0daa74e8e0aa3afd0.
2. There is one change in behavior now -- a cache size of 0 disables the cache whereas it was just allowing for an arbitrarily large cache.
3. There's a screenshot of the example data flow added to https://issues.apache.org/jira/browse/NIFI-2142 which illustrates the difference between running with and without caching. It's not 100% apples to apples with the older TransformXml, but behaves the same way. The queues back up and there's a stark contrast in Tasks/Time.
4. I rebased on the latest master, but contrib-check is failing because of the commenting style on AbstractSNMPProcessor.
---
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 #609: NIFI-2142 Cache compiled XSLT in TransformXml
Posted by jfrazee <gi...@git.apache.org>.
Github user jfrazee commented on a diff in the pull request:
https://github.com/apache/nifi/pull/609#discussion_r71950397
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java ---
@@ -76,25 +84,59 @@
.name("XSLT file name")
.description("Provides the name (including full path) of the XSLT file to apply to the flowfile XML content.")
.required(true)
+ .expressionLanguageSupported(true)
.addValidator(StandardValidators.FILE_EXISTS_VALIDATOR)
.build();
+ public static final PropertyDescriptor INDENT_OUTPUT = new PropertyDescriptor.Builder()
+ .name("indent-output")
+ .displayName("Indent")
+ .description("Whether or not to indent the output.")
+ .required(true)
+ .defaultValue("true")
+ .allowableValues("true", "false")
+ .addValidator(StandardValidators.BOOLEAN_VALIDATOR)
+ .build();
+
+ public static final PropertyDescriptor CACHE_SIZE = new PropertyDescriptor.Builder()
+ .name("cache-size")
+ .displayName("Cache size")
+ .description("Maximum size of the stylesheet cache.")
+ .required(true)
+ .defaultValue("100")
+ .addValidator(StandardValidators.NON_NEGATIVE_INTEGER_VALIDATOR)
--- End diff --
@JPercivall Yes, that's correct. So "Maximum number of stylesheets to store in the cache"?
---
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 #609: NIFI-2142 Cache compiled XSLT in TransformXml
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/609
@jfrazee I've made a couple comments. Looks good overall, mainly just documentation comments. Do you by chance have a template and/or some example xml/XSLT that I could use for testing to verify the 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 pull request #609: NIFI-2142 Cache compiled XSLT in TransformXml
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/609
---
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 #609: NIFI-2142 Cache compiled XSLT in TransformXml
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi/pull/609#discussion_r70735673
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java ---
@@ -76,25 +84,59 @@
.name("XSLT file name")
.description("Provides the name (including full path) of the XSLT file to apply to the flowfile XML content.")
.required(true)
+ .expressionLanguageSupported(true)
.addValidator(StandardValidators.FILE_EXISTS_VALIDATOR)
.build();
+ public static final PropertyDescriptor INDENT_OUTPUT = new PropertyDescriptor.Builder()
+ .name("indent-output")
+ .displayName("Indent")
+ .description("Whether or not to indent the output.")
+ .required(true)
+ .defaultValue("true")
+ .allowableValues("true", "false")
+ .addValidator(StandardValidators.BOOLEAN_VALIDATOR)
+ .build();
+
+ public static final PropertyDescriptor CACHE_SIZE = new PropertyDescriptor.Builder()
+ .name("cache-size")
+ .displayName("Cache size")
+ .description("Maximum size of the stylesheet cache.")
+ .required(true)
+ .defaultValue("100")
+ .addValidator(StandardValidators.NON_NEGATIVE_INTEGER_VALIDATOR)
--- End diff --
Is this property a max character length or data size limit?
---
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 #609: NIFI-2142 Cache compiled XSLT in TransformXml
Posted by jfrazee <gi...@git.apache.org>.
Github user jfrazee commented on the issue:
https://github.com/apache/nifi/pull/609
@JPercivall Oh, right. I think these are corrected now but just noticed the image bundle has a bunch of license violations (even after rebasing).
---
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 #609: NIFI-2142 Cache compiled XSLT in TransformXml
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/609
Sorry for taking so long @jfrazee, I got caught up with something I thought was an issue but turned out it was just me messing up.
+1
Visually verified code, and did a contrib check build. In standalone and a secure 3 node cluster tested the caching using the provided template. Even with just 5 stylesheets being used I saw speed improvements of ~10x! Thanks @jfrazee, I will merge it in.
---
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 #609: NIFI-2142 Cache compiled XSLT in TransformXml
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi/pull/609#discussion_r70735844
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java ---
@@ -76,25 +84,59 @@
.name("XSLT file name")
.description("Provides the name (including full path) of the XSLT file to apply to the flowfile XML content.")
.required(true)
+ .expressionLanguageSupported(true)
.addValidator(StandardValidators.FILE_EXISTS_VALIDATOR)
.build();
+ public static final PropertyDescriptor INDENT_OUTPUT = new PropertyDescriptor.Builder()
+ .name("indent-output")
+ .displayName("Indent")
+ .description("Whether or not to indent the output.")
+ .required(true)
+ .defaultValue("true")
+ .allowableValues("true", "false")
+ .addValidator(StandardValidators.BOOLEAN_VALIDATOR)
+ .build();
+
+ public static final PropertyDescriptor CACHE_SIZE = new PropertyDescriptor.Builder()
+ .name("cache-size")
+ .displayName("Cache size")
+ .description("Maximum size of the stylesheet cache.")
+ .required(true)
+ .defaultValue("100")
+ .addValidator(StandardValidators.NON_NEGATIVE_INTEGER_VALIDATOR)
--- End diff --
Also, this is a integer validator but the property is interpreted as a long on line 171
---
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 #609: NIFI-2142 Cache compiled XSLT in TransformXml
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi/pull/609#discussion_r70736401
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java ---
@@ -76,25 +84,59 @@
.name("XSLT file name")
.description("Provides the name (including full path) of the XSLT file to apply to the flowfile XML content.")
.required(true)
+ .expressionLanguageSupported(true)
.addValidator(StandardValidators.FILE_EXISTS_VALIDATOR)
.build();
+ public static final PropertyDescriptor INDENT_OUTPUT = new PropertyDescriptor.Builder()
+ .name("indent-output")
+ .displayName("Indent")
+ .description("Whether or not to indent the output.")
+ .required(true)
+ .defaultValue("true")
+ .allowableValues("true", "false")
+ .addValidator(StandardValidators.BOOLEAN_VALIDATOR)
+ .build();
+
+ public static final PropertyDescriptor CACHE_SIZE = new PropertyDescriptor.Builder()
+ .name("cache-size")
+ .displayName("Cache size")
+ .description("Maximum size of the stylesheet cache.")
+ .required(true)
+ .defaultValue("100")
+ .addValidator(StandardValidators.NON_NEGATIVE_INTEGER_VALIDATOR)
--- End diff --
Actually my new guess is that this is the maximum number of stylesheets stored (the description could be added to, lol)
---
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 #609: NIFI-2142 Cache compiled XSLT in TransformXml
Posted by jfrazee <gi...@git.apache.org>.
Github user jfrazee commented on the issue:
https://github.com/apache/nifi/pull/609
@JPercivall Addressed your latest comments and added a test with the cache disabled since the logic varies depending on that value now.
---
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 #609: NIFI-2142 Cache compiled XSLT in TransformXml
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi/pull/609#discussion_r72659297
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java ---
@@ -101,16 +101,16 @@
public static final PropertyDescriptor CACHE_SIZE = new PropertyDescriptor.Builder()
.name("cache-size")
.displayName("Cache size")
- .description("Maximum size of the stylesheet cache.")
+ .description("Maximum number of stylesheets to cache. Zero disables the cache.")
.required(true)
.defaultValue("100")
--- End diff --
This seems super high as a default number of stylesheets. I don't have much experience with xml and stylesheets but I would think that the average user of a default value wouldn't need to cache use more than 10. I will defer to your experience with the xml and stylesheets if you think otherwise though.
---
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 #609: NIFI-2142 Cache compiled XSLT in TransformXml
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi/pull/609#discussion_r72658633
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/TransformXml.java ---
@@ -168,15 +168,18 @@ protected PropertyDescriptor getSupportedDynamicPropertyDescriptor(final String
@OnScheduled
public void onScheduled(final ProcessContext context) {
- final Long cacheSize = context.getProperty(CACHE_SIZE).asLong();
- final Long cacheDuration = context.getProperty(CACHE_DURATION).asTimePeriod(TimeUnit.SECONDS);
-
- CacheBuilder cacheBuilder = CacheBuilder.newBuilder();
- if (cacheSize > 0) {
- cacheBuilder = cacheBuilder.maximumSize(cacheSize);
- if (cacheDuration > 0) {
- cacheBuilder = cacheBuilder.expireAfterAccess(cacheDuration, TimeUnit.SECONDS);
- }
+ final ComponentLog logger = getLogger();
+ final Integer cacheSize = context.getProperty(CACHE_SIZE).asInteger();
+ final Long cacheTTL = context.getProperty(CACHE_TTL_AFTER_LAST_ACCESS).asTimePeriod(TimeUnit.SECONDS);
+
+ CacheBuilder cacheBuilder = CacheBuilder.newBuilder().maximumSize(cacheSize);
+
+ if (cacheSize <= 0) {
+ logger.warn("Stylesheet cache disabled because cache size is set to 0");
--- End diff --
The cache will still be created and on line 213 it will try to use it to look up an xslt if the cache size is 0. I'd suggest not even creating it in that 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.
---