You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by pvillard31 <gi...@git.apache.org> on 2017/08/03 21:41:30 UTC
[GitHub] nifi pull request #2056: NIFI-4262 - MergeContent - option to add merged uui...
GitHub user pvillard31 opened a pull request:
https://github.com/apache/nifi/pull/2056
NIFI-4262 - MergeContent - option to add merged uuid in original flow…
… files
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/pvillard31/nifi NIFI-4262
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2056.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 #2056
----
commit bf77df17e8e00d32e1f71922774cf68dd6ba8c2d
Author: Pierre Villard <pi...@gmail.com>
Date: 2017-08-03T21:40:48Z
NIFI-4262 - MergeContent - option to add merged uuid in original flow files
----
---
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 #2056: NIFI-4262 - MergeContent - option to add merged uui...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2056#discussion_r133019004
--- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-processor-utils/src/main/java/org/apache/nifi/processor/util/bin/BinFiles.java ---
@@ -94,6 +96,16 @@
.addValidator(StandardValidators.createTimePeriodValidator(1, TimeUnit.SECONDS, Integer.MAX_VALUE, TimeUnit.SECONDS))
.build();
+ public static final PropertyDescriptor ADD_MERGED_UUID = new PropertyDescriptor.Builder()
--- End diff --
I talked this over with @markap14 as well, it seems like just adding an attribute here is a better user experience than adding another property to offer a choice. I presume it was to offer the current behavior as well, which normally is a good thing so as not to upset existing flows, but I think in this case you could just add it (and document it). New attributes on split flowfiles might affect memory, but for merged flow files it should not be an issue. 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 #2056: NIFI-4262 - MergeContent - option to add merged uui...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2056
---
[GitHub] nifi pull request #2056: NIFI-4262 - MergeContent - option to add merged uui...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2056#discussion_r133019133
--- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-processor-utils/src/main/java/org/apache/nifi/processor/util/bin/Bin.java ---
@@ -173,4 +174,19 @@ public long getBinAge() {
final long ageInNanos = System.nanoTime() - creationMomentEpochNs;
return TimeUnit.MILLISECONDS.convert(ageInNanos, TimeUnit.NANOSECONDS);
}
+
+ /**
+ * @return the UUID of the flow file generated by the merge of this bin's content
+ */
+ public String getUuid() {
+ return uuid;
+ }
+
+ /**
+ * Set the UUID of the flow file generated by the merge of this bin's content
+ * @param uuid UUID to set
+ */
+ public void setUuid(String uuid) {
--- End diff --
Is this the only place the association can be done? Would be nicer if the UUID could be added at transfer time rather than extending the Bin API.
---
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 #2056: NIFI-4262 - MergeContent - option to add merged uui...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2056#discussion_r133466975
--- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-processor-utils/src/main/java/org/apache/nifi/processor/util/bin/BinFiles.java ---
@@ -94,6 +96,16 @@
.addValidator(StandardValidators.createTimePeriodValidator(1, TimeUnit.SECONDS, Integer.MAX_VALUE, TimeUnit.SECONDS))
.build();
+ public static final PropertyDescriptor ADD_MERGED_UUID = new PropertyDescriptor.Builder()
--- End diff --
Agree, this is just adding configuration complexity for very little (none) benefit. I just pushed a commit to remove this parameter.
---
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 #2056: NIFI-4262 - MergeContent - option to add merged uuid in or...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2056
Mind rebasing this against the latest master? I made some changes in MergeContent and I think that is causing it not to merge cleanly. Thanks in advance!
---
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 #2056: NIFI-4262 - MergeContent - option to add merged uuid in or...
Posted by zenfenan <gi...@git.apache.org>.
Github user zenfenan commented on the issue:
https://github.com/apache/nifi/pull/2056
Pulled it and built it locally. Ran the template attached in [NIFI-4028](https://issues.apache.org/jira/browse/NIFI-4028) and verified it to be working as expected. Thanks @pvillard31 @markap14 @mattyb149 Merged to master. +1
---
[GitHub] nifi issue #2056: NIFI-4262 - MergeContent - option to add merged uuid in or...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/2056
@pvillard31 the code looks good to me, though I'm not in a state to easily switch over my branch and merge. I'll try to get to it fairly soon but if anyone else has the review cycles, then please feel free to review/merge. Thanks!
---
[GitHub] nifi issue #2056: NIFI-4262 - MergeContent - option to add merged uuid in or...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the issue:
https://github.com/apache/nifi/pull/2056
Hey @markap14, I rebased against master and followed your recommandations. Let me know if I missed something! Thanks for the review!
---
[GitHub] nifi issue #2056: NIFI-4262 - MergeContent - option to add merged uuid in or...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the issue:
https://github.com/apache/nifi/pull/2056
Done, thanks @mattyb149 !
---
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 #2056: NIFI-4262 - MergeContent - option to add merged uuid in or...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the issue:
https://github.com/apache/nifi/pull/2056
Rebased against master
---
[GitHub] nifi pull request #2056: NIFI-4262 - MergeContent - option to add merged uui...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2056#discussion_r133467718
--- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-processor-utils/src/main/java/org/apache/nifi/processor/util/bin/Bin.java ---
@@ -173,4 +174,19 @@ public long getBinAge() {
final long ageInNanos = System.nanoTime() - creationMomentEpochNs;
return TimeUnit.MILLISECONDS.convert(ageInNanos, TimeUnit.NANOSECONDS);
}
+
+ /**
+ * @return the UUID of the flow file generated by the merge of this bin's content
+ */
+ public String getUuid() {
+ return uuid;
+ }
+
+ /**
+ * Set the UUID of the flow file generated by the merge of this bin's content
+ * @param uuid UUID to set
+ */
+ public void setUuid(String uuid) {
--- End diff --
I just had another look and I'm not sure to see a way to do it nicely. If you have an idea, let me know, I'll be happy to give it a try.
---
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 #2056: NIFI-4262 - MergeContent - option to add merged uui...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2056#discussion_r160995049
--- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-processor-utils/src/main/java/org/apache/nifi/processor/util/bin/Bin.java ---
@@ -173,4 +174,19 @@ public long getBinAge() {
final long ageInNanos = System.nanoTime() - creationMomentEpochNs;
return TimeUnit.MILLISECONDS.convert(ageInNanos, TimeUnit.NANOSECONDS);
}
+
+ /**
+ * @return the UUID of the flow file generated by the merge of this bin's content
+ */
+ public String getUuid() {
+ return uuid;
+ }
+
+ /**
+ * Set the UUID of the flow file generated by the merge of this bin's content
+ * @param uuid UUID to set
+ */
+ public void setUuid(String uuid) {
--- End diff --
After reviewing this again, I think we've got 2 approaches that I would recommend. My preferred approach would be to update the `boolean processBin(Bin, ProcessContext)` method to return a `BinProcessingResult` object, where `BinProcessingResult` would be a new class that has the boolean of whether or not the bin was committed and also has a Map<String, String> of attributes that should be added to the 'original' FlowFiles. This is more flexible and allows us to make other modifications more cleanly in the future. This is not considered part of our API but rather is just a convenience class, so we can change it in any minor version of NiFi.
The second option would be to keep as-is, but at least just check if the bin uuid is null. If so, just ignore it instead of trying to add it to the FlowFiles. This option, though, I feel is a little less clean and requires that implementations know that this 'magical' UUID exists on the bin. It's also less flexible for future updates.
---
[GitHub] nifi pull request #2056: NIFI-4262 - MergeContent - option to add merged uui...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2056#discussion_r143371633
--- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-processor-utils/src/main/java/org/apache/nifi/processor/util/bin/Bin.java ---
@@ -173,4 +174,19 @@ public long getBinAge() {
final long ageInNanos = System.nanoTime() - creationMomentEpochNs;
return TimeUnit.MILLISECONDS.convert(ageInNanos, TimeUnit.NANOSECONDS);
}
+
+ /**
+ * @return the UUID of the flow file generated by the merge of this bin's content
+ */
+ public String getUuid() {
+ return uuid;
+ }
+
+ /**
+ * Set the UUID of the flow file generated by the merge of this bin's content
+ * @param uuid UUID to set
+ */
+ public void setUuid(String uuid) {
--- End diff --
I'm a bit concerned about this approach as well... As is, in this PR, the abstract class is assuming that this UUID has been set by the concrete implementation. If it is not, then it will throw a NPE, I do believe, from looking at the code. We definitely need to address that. I do believe that a cleaner way may be possible, but off the top of my head i don't have one. I'd have to look at the code more. I'll be out of town for the next week, without my computer. But I'd be happy to look into this a bit more when I can, if you guys haven't resolved this by that time.
---