You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nifi.apache.org by brianghig <gi...@git.apache.org> on 2015/07/24 04:53:51 UTC
[GitHub] nifi pull request: [NIFI-784]
GitHub user brianghig opened a pull request:
https://github.com/apache/nifi/pull/71
[NIFI-784]
Moving dynamic propertyMap to onScheduled method and updating logger statements to use lazy evaluation with String formatter
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/brianghig/nifi NIFI-784
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/71.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 #71
----
commit e5444aa3cf6ca941827a3110195424c73cf445f4
Author: Brian Ghigiarelli <br...@gmail.com>
Date: 2015-06-22T13:21:58Z
Merge pull request #5 from apache/develop
Merging latest Apache NiFi develop branch
commit 97f33d5c51dfd5178c10ad88fb3701f363ae39a4
Author: Brian Ghigiarelli <br...@gmail.com>
Date: 2015-06-26T15:21:49Z
Merge remote-tracking branch 'apache/develop' into develop
commit a1af29eca06fa56903ca2fa28945a0f922d6e8e9
Author: Brian Ghigiarelli <br...@gmail.com>
Date: 2015-07-24T01:43:06Z
Merge pull request #6 from apache/develop
Merging in latest Apache NiFi develop branch
commit 4a43e81343b4eaf967a7e5a5417fd7bb35226503
Author: Brian Ghigiarelli <br...@gmail.com>
Date: 2015-07-24T02:52:35Z
[NIFI-784] Moving dynamic propertyMap to onScheduled method and updating logger statements to use lazy evaluation with String formatter
----
---
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: [NIFI-784] RouteOnAttribute Performance Improve...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the pull request:
https://github.com/apache/nifi/pull/71#issuecomment-130315725
Ugh, sorry, @taftster you are absolutely right - in my head, I was thinking that the @OnScheduled was creating a new HashMap each time, rather than clearing on @OnStopped. I just merged this to develop, so I will change it to use a new HashMap each time.
---
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: [NIFI-784] RouteOnAttribute Performance Improve...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the pull request:
https://github.com/apache/nifi/pull/71#issuecomment-124630695
No problem. Glad to have you jumping in on this!
---
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: [NIFI-784] RouteOnAttribute Performance Improve...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/71#discussion_r35454037
--- Diff: nifi/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/RouteOnAttribute.java ---
@@ -103,6 +105,8 @@
private volatile String configuredRouteStrategy = ROUTE_STRATEGY.getDefaultValue();
private volatile Set<String> dynamicPropertyNames = new HashSet<>();
+ private final Map<Relationship, PropertyValue> propertyMap = new HashMap<>();
--- End diff --
Map is both modified and read by multiple threads. This is a thread-safety issue. If we change the 'final' modifier to 'volatile' it should address this (assuming that the @OnUnscheduled annotation is changed to @OnStopped), since it is modified only in the @OnScheduled and @OnStopped method, and no other threads can be in this class at that time.
---
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: [NIFI-784] RouteOnAttribute Performance Improve...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/71
---
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: [NIFI-784] RouteOnAttribute Performance Improve...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/71#discussion_r35454093
--- Diff: nifi/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/RouteOnAttribute.java ---
@@ -216,7 +233,7 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
}
if (destinationRelationships.isEmpty()) {
- logger.info(this + " routing " + flowFile + " to unmatched");
+ logger.info("{} routing {} to unmatched", new Object[]{ this, flowFile });
--- End diff --
We should just remove the 'this' all together because the ProcessorLogger will automatically prepend it for us anyway.
---
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: [NIFI-784] RouteOnAttribute Performance Improve...
Posted by taftster <gi...@git.apache.org>.
Github user taftster commented on the pull request:
https://github.com/apache/nifi/pull/71#issuecomment-130314880
Why is propertyMap marked volatile? The value is only ever set once at construction time.
If the answer is because of thread safety, the contents of the HashMap are not "protected" just because the reference to the map is marked volatile. puts/gets to the map do not inherit the memory barrier protections associated to the volatile reference. c.f. http://stackoverflow.com/questions/10357823/volatile-hashmap-vs-concurrenthashmap
Maybe a review of the concurrency issues of this processor is in order before accepting this merge request? I'm pretty sure that, even though the class will mostly behave correctly since values are set during OnScheduled and OnStopped, these are not "safely published" to the map. While unlikely, other threads could potentially see stale values in this map.
Either this class should likely be using ConcurrentHashMap here, or it should republish an entirely fresh map by calling "new HashMap()" instead of "clear()".
---
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: [NIFI-784] RouteOnAttribute Performance Improve...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/nifi/pull/71#discussion_r36870124
--- Diff: nifi/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/RouteOnAttribute.java ---
@@ -236,7 +259,7 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
// now transfer any clones generated
for (final Map.Entry<Relationship, FlowFile> entry : transferMap.entrySet()) {
- logger.info(this + " cloned " + flowFile + " into " + entry.getValue() + " and routing clone to relationship " + entry.getKey());
+ logger.info("Cloned {} into {} and routing clone to relationship {}", new Object[]{ flowFile, entry.getValue(), entry.getKey() });
--- End diff --
Are we moving away from logging 'this'? I've seen those messages be incredibly useful in the past...
---
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: [NIFI-784] RouteOnAttribute Performance Improve...
Posted by brianghig <gi...@git.apache.org>.
Github user brianghig commented on the pull request:
https://github.com/apache/nifi/pull/71#issuecomment-124630190
Sounds good, @markap14, and thanks for the feedback. I'll update the PR shortly.
---
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: [NIFI-784] RouteOnAttribute Performance Improve...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/71#discussion_r36872857
--- Diff: nifi/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/RouteOnAttribute.java ---
@@ -236,7 +259,7 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
// now transfer any clones generated
for (final Map.Entry<Relationship, FlowFile> entry : transferMap.entrySet()) {
- logger.info(this + " cloned " + flowFile + " into " + entry.getValue() + " and routing clone to relationship " + entry.getKey());
+ logger.info("Cloned {} into {} and routing clone to relationship {}", new Object[]{ flowFile, entry.getValue(), entry.getKey() });
--- End diff --
@madrob the ProcessorLog that is used here automatically adds it in so that it is done consistently and without the developers having to remember to log it. So if we add 'this' to the beginning, it ends up getting logged twice.
---
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: [NIFI-784] RouteOnAttribute Performance Improve...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/71#discussion_r35454101
--- Diff: nifi/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/RouteOnAttribute.java ---
@@ -236,7 +253,7 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
// now transfer any clones generated
for (final Map.Entry<Relationship, FlowFile> entry : transferMap.entrySet()) {
- logger.info(this + " cloned " + flowFile + " into " + entry.getValue() + " and routing clone to relationship " + entry.getKey());
+ logger.info("{} cloned {} into {} and routing clone to relationship {}", new Object[]{ this, flowFile, entry.getValue(), entry.getKey() });
--- End diff --
We should just remove the 'this' all together because the ProcessorLogger will automatically prepend it for us anyway.
---
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: [NIFI-784] RouteOnAttribute Performance Improve...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/71#discussion_r35453796
--- Diff: nifi/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/RouteOnAttribute.java ---
@@ -166,6 +170,27 @@ public void onPropertyModified(final PropertyDescriptor descriptor, final String
this.relationships.set(newRelationships);
}
+ /**
+ * When this processor is
+ * @param context
--- End diff --
This will fail checkstyle, I believe, because there's a @param without any documentation
---
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: [NIFI-784] RouteOnAttribute Performance Improve...
Posted by brianghig <gi...@git.apache.org>.
Github user brianghig commented on the pull request:
https://github.com/apache/nifi/pull/71#issuecomment-130308592
We've been running this for a couple weeks and all looks well. Is there anything else you'd like me to address with this ticket?
---
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: [NIFI-784] RouteOnAttribute Performance Improve...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/71#discussion_r35453740
--- Diff: nifi/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/RouteOnAttribute.java ---
@@ -166,6 +170,27 @@ public void onPropertyModified(final PropertyDescriptor descriptor, final String
this.relationships.set(newRelationships);
}
+ /**
+ * When this processor is
+ * @param context
+ */
+ @OnScheduled
+ public void onScheduled(final ProcessContext context) {
+ for (final PropertyDescriptor descriptor : context.getProperties().keySet()) {
+ if (!descriptor.isDynamic()) {
+ continue;
+ }
+ getLogger().debug("Adding new dynamic property: {}", new Object[]{descriptor});
+ propertyMap.put(new Relationship.Builder().name(descriptor.getName()).build(), context.getProperty(descriptor));
+ }
+ }
+
+ @OnUnscheduled
--- End diff --
Can we changed this to @OnStopped? As it is right now, we have a race condition - map could get cleared just after a thread enters onTrigger, which would result in it running with an empty map
---
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: [NIFI-784] RouteOnAttribute Performance Improve...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the pull request:
https://github.com/apache/nifi/pull/71#issuecomment-130320701
Created Ticket https://issues.apache.org/jira/browse/NIFI-843 to address that and submitted a patch.
---
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.
---