You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by MikeThomsen <gi...@git.apache.org> on 2018/01/29 19:31:04 UTC
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
GitHub user MikeThomsen opened a pull request:
https://github.com/apache/nifi/pull/2443
NIFI-4827 Added support for reading queries from the flowfile body to…
… GetMongo.
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/MikeThomsen/nifi NIFI-4827
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2443.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 #2443
----
commit 53ccfec329c09a18cf87d562482195ddf179b73d
Author: Mike Thomsen <mi...@...>
Date: 2018-01-29T19:18:28Z
NIFI-4827 Added support for reading queries from the flowfile body to GetMongo.
----
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r170033540
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/test/java/org/apache/nifi/processors/mongodb/GetMongoTest.java ---
@@ -69,6 +70,7 @@
@Before
public void setup() {
runner = TestRunners.newTestRunner(GetMongo.class);
+ runner.setProperty(GetMongo.QUERY, "{}");
--- End diff --
Does this need to be here? Seems like it might hide the cases when Query is empty, which would be the existing behavior. If it's because of the validator, then it should accept an empty query as valid since the property is not required. Also, can you add a unit test (or update the existing one) for when the Query is empty and there is no incoming flow file (but there is an incoming connection)? That would test the other half of the conditions for the exception block you added at the top of onTrigger.
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r170043364
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/GetMongo.java ---
@@ -226,6 +242,11 @@ private ObjectWriter getObjectWriter(ObjectMapper mapper, String ppSetting) {
@Override
public void onTrigger(final ProcessContext context, final ProcessSession session) throws ProcessException {
+ FlowFile input = session.get();
+ if (!context.hasIncomingConnection() && (context.getProperty(QUERY) == null)) {
+ throw new RuntimeException("Without an incoming connection, the Query property must be set.");
--- End diff --
I just realized that this block to check for an incoming connection and the Query property will get executed on each onTrigger. I believe it should be done during setup (with an OnScheduled method), check ExecuteSQL for an example. Otherwise this processor can generate an exception on each execution, and if the Run Schedule is zero seconds...
---
[GitHub] nifi issue #2443: NIFI-4827 Added support for reading queries from the flowf...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2443
@mattyb149 Any chance you can take a look?
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2443
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r164577827
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/test-flows/NIFI_4827.xml ---
@@ -0,0 +1,635 @@
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
--- End diff --
I believe this file needs to be removed.
---
[GitHub] nifi issue #2443: NIFI-4827 Added support for reading queries from the flowf...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2443
@mattyb149 Rebased and pushed. Building now.
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r170034475
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/test/java/org/apache/nifi/processors/mongodb/GetMongoTest.java ---
@@ -296,4 +298,55 @@ public void testQueryAttribute() {
Assert.assertEquals("Value was wrong", val, "{}");
}
}
+ public void testQueryLocationConfig() {
+ //Test EL
+ Map attributes = new HashMap();
+ attributes.put("field", "c");
+ attributes.put("value", "4");
+ String query = "{ \"${field}\": { \"$gte\": ${value}}}";
+ runner.setProperty(GetMongo.QUERY, query);
+ runner.setProperty(GetMongo.RESULTS_PER_FLOWFILE, "10");
+ runner.setValidateExpressionUsage(true);
+ runner.enqueue("test", attributes);
+ runner.run(1, true, true);
+
+ runner.assertTransferCount(GetMongo.REL_FAILURE, 0);
+ runner.assertTransferCount(GetMongo.REL_ORIGINAL, 1);
+ runner.assertTransferCount(GetMongo.REL_SUCCESS, 1);
+
+ runner.clearTransferState();
+
+ query = "{ \"c\": { \"$gte\": 4 }}";
+ runner.setProperty(GetMongo.QUERY, query);
+ runner.run(1, true, true);
+ runner.assertTransferCount(GetMongo.REL_FAILURE, 0);
+ runner.assertTransferCount(GetMongo.REL_ORIGINAL, 0);
+ runner.assertTransferCount(GetMongo.REL_SUCCESS, 1);
+
+ runner.clearTransferState();
+
+ runner.removeProperty(GetMongo.QUERY);
+ runner.enqueue(query);
+ runner.run(1, true, true);
+
+ runner.assertTransferCount(GetMongo.REL_FAILURE, 0);
+ runner.assertTransferCount(GetMongo.REL_ORIGINAL, 1);
+ runner.assertTransferCount(GetMongo.REL_SUCCESS, 1);
+
+ runner.clearTransferState();
+
+ boolean exception = false;
+ try {
+ runner.assertValid();
+ runner.setIncomingConnection(false);
+ runner.run(1, true, true);
+ } catch (Throwable pe) {
+ exception = true;
+ }
+
+ Assert.assertTrue("No exception was thrown!", exception);
--- End diff --
After changing to throw a ProcessException, you could instead have an Exception object set to null (rather than a boolean set to false), then you can assertNotNull(exception) as well as assertTrue(exception instanceof ProcessException).
---
[GitHub] nifi issue #2443: NIFI-4827 Added support for reading queries from the flowf...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2443
Thanks. I'll rebase the other one and get started there.
---
[GitHub] nifi issue #2443: NIFI-4827 Added support for reading queries from the flowf...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2443
Ok. I just pushed some changes that should fix things with the query location issue. I took your advice on following the SQL approach. Unit tests are there and batter the heck out of it.
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r164607830
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/GetMongo.java ---
@@ -89,6 +99,17 @@ public ValidationResult validate(final String subject, final String value, final
.expressionLanguageSupported(true)
.addValidator(DOCUMENT_VALIDATOR)
.build();
+
+ static final AllowableValue LOC_BODY = new AllowableValue("body", "Body");
+ static final AllowableValue LOC_PARAM = new AllowableValue("param", "Query Parameter");
+ static final PropertyDescriptor QUERY_LOC = new PropertyDescriptor.Builder()
--- End diff --
We can ask the users list to get UX and other feedback, for ExecuteSQL it was natural because the query used to be required, then we relaxed it and added doc to the property saying if left blank, the contents of the flow file are expected to have the SQL to execute. In this case, a blank setting is already meaningful, so we may need two properties. "Explicit is better" is a good rule of thumb, but now we have two properties that interact with each other in different ways, which can also be confusing (see my other comments on when things are set to various value combos)
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r170676899
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/GetMongo.java ---
@@ -76,13 +77,28 @@
return builder.explanation(reason).valid(reason == null).build();
};
- static final PropertyDescriptor QUERY = new PropertyDescriptor.Builder()
- .name("Query")
- .description("The selection criteria; must be a valid MongoDB Extended JSON format; if omitted the entire collection will be queried")
- .required(false)
- .expressionLanguageSupported(true)
- .addValidator(DOCUMENT_VALIDATOR)
+ static final Relationship REL_SUCCESS = new Relationship.Builder().name("success").description("All files are routed to success").build();
+ static final Relationship REL_FAILURE = new Relationship.Builder()
+ .name("failure")
+ .description("All input flowfiles that are part of a failed query execution go here.")
+ .build();
+
+ static final Relationship REL_ORIGINAL = new Relationship.Builder()
+ .name("original")
+ .description("All input flowfiles that are part of a successful query execution go here.")
.build();
+
+ static final PropertyDescriptor QUERY = new PropertyDescriptor.Builder()
+ .name("Query")
+ .description("The selection criteria to do the lookup. If the field is left blank, it will look for input from" +
+ " an incoming connection from another processor to provide the query as a valid JSON document inside of " +
+ "the flowfile's body. If this field is left blank and a timer is enabled instead of an incoming connection, " +
--- End diff --
This needs to be reverted to the original description of behavior when blank, that the entire collection will be queried. If this is not the intent, and an error should be thrown, then you'll need to change the logic around line 264
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r170600330
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/GetMongo.java ---
@@ -236,12 +261,33 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
context.getProperty(QUERY).evaluateAttributeExpressions().getValue());
}
- final Document query = context.getProperty(QUERY).isSet()
- ? Document.parse(context.getProperty(QUERY).evaluateAttributeExpressions().getValue()) : null;
+ final Document query;
+ if (context.getProperty(QUERY).isSet()) {
+ String queryStr = context.getProperty(QUERY).evaluateAttributeExpressions(input).getValue();
+ query = Document.parse(queryStr);
+ } else if (!context.getProperty(QUERY).isSet() && input == null) {
+ query = Document.parse("{}");
--- End diff --
The documentation for the Query property says that if there is no incoming connection and Query is not set, then it results in an error. But this like issues an empty query which appears to fetch all results.
Also please check line 234 above, looks like we need to do that later, after "queryStr" is populated for example. Currently the Query Attribute will not be populated if there is an incoming flow file.
---
[GitHub] nifi issue #2443: NIFI-4827 Added support for reading queries from the flowf...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2443
+1 LGTM, ran unit tests and contrib-check, checked documentation and verified behavior on a NiFi instance, with and without incoming connections, Query parameter set, self-loops, etc. Thanks for the improvement! Merging to master
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r170616361
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/GetMongo.java ---
@@ -236,12 +261,33 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
context.getProperty(QUERY).evaluateAttributeExpressions().getValue());
}
- final Document query = context.getProperty(QUERY).isSet()
- ? Document.parse(context.getProperty(QUERY).evaluateAttributeExpressions().getValue()) : null;
+ final Document query;
+ if (context.getProperty(QUERY).isSet()) {
+ String queryStr = context.getProperty(QUERY).evaluateAttributeExpressions(input).getValue();
+ query = Document.parse(queryStr);
+ } else if (!context.getProperty(QUERY).isSet() && input == null) {
+ query = Document.parse("{}");
--- End diff --
Ok. I think what I'll do is just put the query attribute on there no matter what if the query attribute is set because that'll help people figure out what happened in every scenario where a query was sent to Mongo.
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r170687690
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/GetMongo.java ---
@@ -76,13 +77,28 @@
return builder.explanation(reason).valid(reason == null).build();
};
- static final PropertyDescriptor QUERY = new PropertyDescriptor.Builder()
- .name("Query")
- .description("The selection criteria; must be a valid MongoDB Extended JSON format; if omitted the entire collection will be queried")
- .required(false)
- .expressionLanguageSupported(true)
- .addValidator(DOCUMENT_VALIDATOR)
+ static final Relationship REL_SUCCESS = new Relationship.Builder().name("success").description("All files are routed to success").build();
+ static final Relationship REL_FAILURE = new Relationship.Builder()
+ .name("failure")
+ .description("All input flowfiles that are part of a failed query execution go here.")
+ .build();
+
+ static final Relationship REL_ORIGINAL = new Relationship.Builder()
+ .name("original")
+ .description("All input flowfiles that are part of a successful query execution go here.")
.build();
+
+ static final PropertyDescriptor QUERY = new PropertyDescriptor.Builder()
+ .name("Query")
+ .description("The selection criteria to do the lookup. If the field is left blank, it will look for input from" +
+ " an incoming connection from another processor to provide the query as a valid JSON document inside of " +
+ "the flowfile's body. If this field is left blank and a timer is enabled instead of an incoming connection, " +
--- End diff --
Done. Sorry about that.
---
[GitHub] nifi issue #2443: NIFI-4827 Added support for reading queries from the flowf...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2443
NP. I'll try to get that done in a little bit.
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r164608075
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/test-flows/NIFI_4827.xml ---
@@ -0,0 +1,635 @@
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
--- End diff --
I like the idea of having test flows, but maybe not in the codebase since they aren't executed? I often put test flows up as a Gist on GitHub and refer to it in the PR, so the reviewer can go there for a sample flow (which I try to document inline with labels to explain what's going on)
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r164718633
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/test-flows/NIFI_4827.xml ---
@@ -0,0 +1,635 @@
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
--- End diff --
Ok. Removed this for now since it can go on the Wiki.
---
[GitHub] nifi issue #2443: NIFI-4827 Added support for reading queries from the flowf...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2443
Two of the 3 builds succeeded, and there are now five tests that cover the "read query" behavior so I think the tires have been pretty well kicked on this one.
---
[GitHub] nifi issue #2443: NIFI-4827 Added support for reading queries from the flowf...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2443
@mattyb149 Ready for review.
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r164607381
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/GetMongo.java ---
@@ -81,6 +82,15 @@ public ValidationResult validate(final String subject, final String value, final
};
static final Relationship REL_SUCCESS = new Relationship.Builder().name("success").description("All files are routed to success").build();
+ static final Relationship REL_FAILURE = new Relationship.Builder()
+ .name("failure")
+ .description("All input flowfiles that are part of a failed query execution go here.")
--- End diff --
Correct, just thinking of other processors where this happens, their doc says something like what I put above, with no incoming connections the relationship is unused.
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r170044639
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/test/java/org/apache/nifi/processors/mongodb/GetMongoTest.java ---
@@ -296,4 +298,55 @@ public void testQueryAttribute() {
Assert.assertEquals("Value was wrong", val, "{}");
}
}
+ public void testQueryLocationConfig() {
+ //Test EL
+ Map attributes = new HashMap();
+ attributes.put("field", "c");
+ attributes.put("value", "4");
+ String query = "{ \"${field}\": { \"$gte\": ${value}}}";
+ runner.setProperty(GetMongo.QUERY, query);
+ runner.setProperty(GetMongo.RESULTS_PER_FLOWFILE, "10");
+ runner.setValidateExpressionUsage(true);
+ runner.enqueue("test", attributes);
+ runner.run(1, true, true);
+
+ runner.assertTransferCount(GetMongo.REL_FAILURE, 0);
+ runner.assertTransferCount(GetMongo.REL_ORIGINAL, 1);
+ runner.assertTransferCount(GetMongo.REL_SUCCESS, 1);
+
+ runner.clearTransferState();
+
+ query = "{ \"c\": { \"$gte\": 4 }}";
+ runner.setProperty(GetMongo.QUERY, query);
+ runner.run(1, true, true);
+ runner.assertTransferCount(GetMongo.REL_FAILURE, 0);
+ runner.assertTransferCount(GetMongo.REL_ORIGINAL, 0);
+ runner.assertTransferCount(GetMongo.REL_SUCCESS, 1);
+
+ runner.clearTransferState();
+
+ runner.removeProperty(GetMongo.QUERY);
+ runner.enqueue(query);
+ runner.run(1, true, true);
+
+ runner.assertTransferCount(GetMongo.REL_FAILURE, 0);
+ runner.assertTransferCount(GetMongo.REL_ORIGINAL, 1);
+ runner.assertTransferCount(GetMongo.REL_SUCCESS, 1);
+
+ runner.clearTransferState();
+
+ boolean exception = false;
+ try {
+ runner.assertValid();
+ runner.setIncomingConnection(false);
+ runner.run(1, true, true);
+ } catch (Throwable pe) {
+ exception = true;
+ }
+
+ Assert.assertTrue("No exception was thrown!", exception);
--- End diff --
Done
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r170032821
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/GetMongo.java ---
@@ -226,6 +242,11 @@ private ObjectWriter getObjectWriter(ObjectMapper mapper, String ppSetting) {
@Override
public void onTrigger(final ProcessContext context, final ProcessSession session) throws ProcessException {
+ FlowFile input = session.get();
+ if (!context.hasIncomingConnection() && (context.getProperty(QUERY) == null)) {
+ throw new RuntimeException("Without an incoming connection, the Query property must be set.");
--- End diff --
The RuntimeException here and on line 276 should be ProcessException, it's a NiFi-specific runtime exception that indicates we realize something went wrong but we don't know how to handle any input/output we might have (like routing to failure). Also rather than `context.getProperty(QUERY) == null` since there will be a PropertyValue object returned, you should use `context.getProperty(QUERY).isSet()`.
---
[GitHub] nifi issue #2443: NIFI-4827 Added support for reading queries from the flowf...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2443
@mattyb149 I have time today that if you can get to this I can switch over to 4838 and get that ready for close out too.
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r164602936
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/GetMongo.java ---
@@ -89,6 +99,17 @@ public ValidationResult validate(final String subject, final String value, final
.expressionLanguageSupported(true)
.addValidator(DOCUMENT_VALIDATOR)
.build();
+
+ static final AllowableValue LOC_BODY = new AllowableValue("body", "Body");
+ static final AllowableValue LOC_PARAM = new AllowableValue("param", "Query Parameter");
+ static final PropertyDescriptor QUERY_LOC = new PropertyDescriptor.Builder()
--- End diff --
Everything is tied to that parameter (or should be tied to it). One of the things I'm learning from having to train someone on NiFi is that "explicit is better" for a lot of people. I think the meaning of a blank query wouldn't be obvious to a lot of users, and you cannot trust that they'll read the description closely rather than cursing out the product because it seems to be "mysteriously misbehaving."
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r164602170
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/test-flows/NIFI_4827.xml ---
@@ -0,0 +1,635 @@
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
--- End diff --
It's a miniature test flow that's meant to show the behavior if someone has questions. I got the idea when I was reviewing the PutInfluxDb processor. On nifi-dev, I think @joewitt mentioned that a major problem with some of these bundles is the people who actually know a particular system might be highly limited in the community. I think getting into a habit of sharing test flows that use something like GenerateFlowFile could ease the learning curve on others. Particularly if a docker-compose file or something like that is added with it.
I can remove it, but I think it might be a good practice.
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r164711784
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/GetMongo.java ---
@@ -89,6 +99,17 @@ public ValidationResult validate(final String subject, final String value, final
.expressionLanguageSupported(true)
.addValidator(DOCUMENT_VALIDATOR)
.build();
+
+ static final AllowableValue LOC_BODY = new AllowableValue("body", "Body");
+ static final AllowableValue LOC_PARAM = new AllowableValue("param", "Query Parameter");
+ static final PropertyDescriptor QUERY_LOC = new PropertyDescriptor.Builder()
--- End diff --
Actually, I think allowing a blank field was the wrong decision because it is not a valid query w/ MongoDB. So I'll make that change.
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r164576841
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/GetMongo.java ---
@@ -81,6 +82,15 @@ public ValidationResult validate(final String subject, final String value, final
};
static final Relationship REL_SUCCESS = new Relationship.Builder().name("success").description("All files are routed to success").build();
+ static final Relationship REL_FAILURE = new Relationship.Builder()
+ .name("failure")
+ .description("All input flowfiles that are part of a failed query execution go here.")
--- End diff --
Failure and Original only apply when there are incoming flow files, I see that you mention "input flowfiles" here but I wonder if it should be even more explicit, such as pointing out that if there are no incoming connections then the relationship will be ignored/unused?
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r170070960
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/test/java/org/apache/nifi/processors/mongodb/GetMongoTest.java ---
@@ -69,6 +70,7 @@
@Before
public void setup() {
runner = TestRunners.newTestRunner(GetMongo.class);
+ //runner.setProperty(GetMongo.QUERY, "{}");
--- End diff --
This can now be removed
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r164713456
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/test-flows/NIFI_4827.xml ---
@@ -0,0 +1,635 @@
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
--- End diff --
Agreed, I often make the Gist available for the reviewer, then once merged, the template could be added to the Wiki of sample templates for all to have
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r164708858
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/test-flows/NIFI_4827.xml ---
@@ -0,0 +1,635 @@
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
--- End diff --
If not there, it should be some place that is owned by the NiFi team so it can easily survive us if the infamous "hit by a bus" scenario happens.
---
[GitHub] nifi issue #2443: NIFI-4827 Added support for reading queries from the flowf...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2443
Merging #2180 caused a conflict on this PR, mind rebasing against the latest master? Please and thanks!
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r170042887
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/test/java/org/apache/nifi/processors/mongodb/GetMongoTest.java ---
@@ -69,6 +70,7 @@
@Before
public void setup() {
runner = TestRunners.newTestRunner(GetMongo.class);
+ runner.setProperty(GetMongo.QUERY, "{}");
--- End diff --
Ok. What I'm going to do there is make it so that empty query is only valid when there is no incoming connection.
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r170070552
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/GetMongo.java ---
@@ -236,12 +254,33 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
context.getProperty(QUERY).evaluateAttributeExpressions().getValue());
}
- final Document query = context.getProperty(QUERY).isSet()
- ? Document.parse(context.getProperty(QUERY).evaluateAttributeExpressions().getValue()) : null;
+ final Document query;
+ if (!context.hasIncomingConnection() && !context.getProperty(QUERY).isSet()) {
+ query = Document.parse("{}");
--- End diff --
I thought this was going to be a validation error? It can be done in OnScheduled, see ExecuteSQL for an example. Otherwise how would the user know that his/her configuration won't actually perform any work?
---
[GitHub] nifi issue #2443: NIFI-4827 Added support for reading queries from the flowf...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2443
@mattyb149 Passed the test and it says it's ready.
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r170043742
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/test/java/org/apache/nifi/processors/mongodb/GetMongoTest.java ---
@@ -69,6 +70,7 @@
@Before
public void setup() {
runner = TestRunners.newTestRunner(GetMongo.class);
+ runner.setProperty(GetMongo.QUERY, "{}");
--- End diff --
I don't think you have access to connection information during validation (all you get is a ValidationContext), I was looking into adding it at one point, but never got around to it.
---
[GitHub] nifi issue #2443: NIFI-4827 Added support for reading queries from the flowf...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2443
@mattyb149 I broke the query location unit test down into three distinct cases:
* Verify that an empty query w/ no flowfile defaults to timer-driven behavior with `{}` as the query.
* Verify that a query param will work when set.
* Verify that it can read from the body w/ EL support.
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r170039382
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/GetMongo.java ---
@@ -226,6 +242,11 @@ private ObjectWriter getObjectWriter(ObjectMapper mapper, String ppSetting) {
@Override
public void onTrigger(final ProcessContext context, final ProcessSession session) throws ProcessException {
+ FlowFile input = session.get();
+ if (!context.hasIncomingConnection() && (context.getProperty(QUERY) == null)) {
+ throw new RuntimeException("Without an incoming connection, the Query property must be set.");
--- End diff --
Done.
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r170070317
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/GetMongo.java ---
@@ -226,6 +242,8 @@ private ObjectWriter getObjectWriter(ObjectMapper mapper, String ppSetting) {
@Override
public void onTrigger(final ProcessContext context, final ProcessSession session) throws ProcessException {
+ FlowFile input = session.get();
--- End diff --
It's not checking for input == null here, that would be one of the requirements for the "flowfile-driven behavior" rather than the "timer-driven behavior". It might cause an NPE at line 266, and otherwise if null you may be treating it as flowfile-driven when really it is more like timer-driven. ExecuteSQL has a good example of how to handle this.
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r164563821
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/GetMongo.java ---
@@ -302,15 +348,19 @@ public void process(OutputStream out) throws IOException {
}
}
- session.commit();
+ if (input != null) {
+ session.transfer(input, REL_ORIGINAL);
--- End diff --
If you set Query Location to Query Parameter, but there are input flow files, then the "original" relationship might be a bit confusing. If I schedule GetMongo to run once a second, and upstream it is generating flow files once every 2 seconds, then if using Query Parameter, the first GetMongo will not output along the "original" relationship, but the second one will, causing twice as many success FlowFiles as originals. This is in line with the documented behavior, but it seems a bit awkward to me, what do you think?
An alternative is to not run if there is an incoming connection but no flow file, basically the processor behaves in an event-driven fashion if there is an incoming connection, or a timer-driven fashion if there is no incoming (non-loop) connection. ExecuteSQL has this code at the beginning of onTrigger():
```
FlowFile fileToProcess = null;
if (context.hasIncomingConnection()) {
fileToProcess = session.get();
// If we have no FlowFile, and all incoming connections are self-loops then we can continue on.
// However, if we have no FlowFile and we have connections coming from other Processors, then
// we know that we should run only if we have a FlowFile.
if (fileToProcess == null && context.hasNonLoopConnection()) {
return;
}
}
```
You could do other runtime checking (unfortunately, connections are not currently known to the `ValidationContext` or `ProcessorInitializationContext` so must be done in onTrigger) such as making sure that if there is an incoming connection, then the Query Location must be Body, if that's the kind of behavior you want. Thoughts?
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r164602408
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/GetMongo.java ---
@@ -81,6 +82,15 @@ public ValidationResult validate(final String subject, final String value, final
};
static final Relationship REL_SUCCESS = new Relationship.Builder().name("success").description("All files are routed to success").build();
+ static final Relationship REL_FAILURE = new Relationship.Builder()
+ .name("failure")
+ .description("All input flowfiles that are part of a failed query execution go here.")
--- End diff --
So you're saying it should just be more explicit in the documentation, and not be some sort of dynamic relationship a la RouteOnAttribute, right?
---
[GitHub] nifi pull request #2443: NIFI-4827 Added support for reading queries from th...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2443#discussion_r164561209
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/GetMongo.java ---
@@ -89,6 +99,17 @@ public ValidationResult validate(final String subject, final String value, final
.expressionLanguageSupported(true)
.addValidator(DOCUMENT_VALIDATOR)
.build();
+
+ static final AllowableValue LOC_BODY = new AllowableValue("body", "Body");
+ static final AllowableValue LOC_PARAM = new AllowableValue("param", "Query Parameter");
+ static final PropertyDescriptor QUERY_LOC = new PropertyDescriptor.Builder()
--- End diff --
It's a bummer we need an additional property here to specify where the query is coming from; seems to me that's only because you can leave Query blank and it means something. In other processors like ExecuteSQL, you either provide a Query in the property (and it is used), or you leave it blank and then it is expected that the body of the incoming flow file contains the query.
What is the behavior when Query is filled in but the Query Location is Body? Will Query Parameter be ignored or should the processor be marked invalid? It looks from the doc that it will be ignored, but I wonder if the description below should add something like
> If 'body' is selected, it will come from the flowfile instead of the query parameter, whether the query parameter is set or not
That might just be me though, if it seems clear as written then I'm ok
---