NIFI-5052 Added DeleteByQuery ElasticSearch processor. #2616

MikeThomsen · 2018-04-07T10:09:52Z

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?
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.

zenfenan

There is one checkstyle violation. Hard to find it from travis log though. Doubt if it has something to do with the comments in POM

zenfenan · 2018-04-07T13:59:03Z

Reviewing..

MikeThomsen · 2018-04-10T11:54:28Z

Anything @zenfenan?

zenfenan · 2018-04-10T15:37:57Z

I'm extremely sorry for the delay. Got held up by some unexpected works. Will try to complete by this week. Is that okay? Meanwhile, the code looks good. I just wanted to try it before approving. :)

MikeThomsen · 2018-04-10T16:21:56Z

@zenfenan sounds like a plan. After the change to expressionLanguageSupport I might have to rebase this today FYI.

zenfenan · 2018-04-11T16:20:25Z

...ssors/src/main/java/org/apache/nifi/processors/elasticsearch/ElasticSearchRestProcessor.java

+    PropertyDescriptor QUERY = new PropertyDescriptor.Builder()
+            .name("el-rest-query")
+            .displayName("Query")
+            .description("A query in JSON syntax, not Lucene syntax. Ex: " +


description can be more descriptive. If we don't give the query in QUERY then it reads from the flowfile content. I think that can be added to the description.

zenfenan · 2018-04-11T16:22:04Z

nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/pom.xml

+            <!--<groupId>org.elasticsearch.client</groupId>-->
+            <!--<artifactId>rest</artifactId>-->
+            <!--<version>5.6.6</version>-->
+        <!--</dependency>-->


Comments to be deleted?

zenfenan · 2018-04-11T16:22:27Z

nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/pom.xml

@@ -137,7 +149,7 @@
                            <httpPort>9400</httpPort>
                            <version>5.6.2</version>
                            <timeout>90</timeout>
-                            <pathInitScript>${project.basedir}/src/test/resources/setup.script</pathInitScript>
+                            <!--<pathInitScript>${project.basedir}/src/test/resources/setup.script</pathInitScript>-->


Same here. Can they be removed?

zenfenan · 2018-04-11T16:40:44Z

...ssors/src/main/java/org/apache/nifi/processors/elasticsearch/ElasticSearchRestProcessor.java

+    PropertyDescriptor INDEX = new PropertyDescriptor.Builder()
+            .name("el-rest-fetch-index")
+            .displayName("Index")
+            .description("The name of the index to read from")


I know this is not introduced by you, rather moved over from JsonQueryElasticSearch but for JsonQueryElasticSearch, the description matched the intention, because it was just reading but since now the delete processor is using this property, changing it to something like "The name of the ElasticSearch Index to use" makes sense

zenfenan · 2018-04-11T16:44:55Z

...ssors/src/main/java/org/apache/nifi/processors/elasticsearch/ElasticSearchRestProcessor.java

+            .name("el-rest-query")
+            .displayName("Query")
+            .description("A query in JSON syntax, not Lucene syntax. Ex: " +
+                    "{\n" +


Same here - not something you introduced but moved and mostly a cosmetic change. Feel free to ignore this. The \t and \n won't be rendered in the UI so simply writing { \"query\": { \"match\": { \"name\": \"John Smith\" } } }" is enough.

zenfenan

@MikeThomsen I think we are close.. Only one more typo to be fixed.

zenfenan · 2018-04-12T12:40:57Z

...ssors/src/main/java/org/apache/nifi/processors/elasticsearch/ElasticSearchRestProcessor.java

-                    "\t}\n" +
-                    "}")
+            .description("A query in JSON syntax, not Lucene syntax. Ex: {\"query\":{\"match\":{\"somefield\":\"somevalue\"}}}. " +
+                    "If this parameter is not set, the query will be ready from the flowfile content.")


Typo : 'read'

MikeThomsen · 2018-04-12T13:18:51Z

@zenfenan Done. Do you have commit privileges? Had to ask because I'm not sure how up to date the team page is on apache.org.

zenfenan · 2018-04-12T14:14:36Z

@MikeThomsen Nope. I don't have commit privileges.

BTW, I have ran the build and tested locally.

Tested the new DeleteByQueryElasticsearch processor with both flowfile content as well as Query property
Tested the JsonQueryElasticsearch to make sure that the new changes doesn't cause any new issues to it.

Everything works fine and looks good to me. Thanks, @MikeThomsen

MikeThomsen · 2018-04-12T14:22:11Z

Thanks @zenfenan
@mattyb149 @pvillard31 What do you think? Ready to merge?

MikeThomsen · 2018-04-23T11:13:13Z

@JPercivall if you have any time, do you think you could take a look at this?

JPercivall · 2018-04-23T14:01:18Z

Hey @MikeThomsen sorry to leave you hanging but I just got super busy with my day job. I may be able to help in a couple weeks can't at the moment.

MikeThomsen · 2018-04-23T21:39:24Z

@mattyb149 @pvillard31 Got any time for a review?

mattyb149 · 2018-04-26T15:27:23Z

Reviewing...

mattyb149 · 2018-04-26T15:29:39Z

...ssors/src/main/java/org/apache/nifi/processors/elasticsearch/DeleteByQueryElasticsearch.java

+        "or from the Query parameter.")
+@Tags({ "elastic", "elasticsearch", "delete", "query"})
+@WritesAttributes({
+    @WritesAttribute(attribute = "elasticsearch.delete.took"),


These should have descriptions as they will be displayed in the processor documentation

mattyb149 · 2018-04-26T15:30:30Z

nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/pom.xml

+            <groupId>org.elasticsearch</groupId>
+            <artifactId>elasticsearch</artifactId>
+            <version>5.6.8</version>
+            <scope>compile</scope>


Not sure these need to be explicitly scoped? But if it works, no harm no foul...

mattyb149 · 2018-04-26T15:31:33Z

...e/src/test/java/org/apache/nifi/elasticsearch/integration/ElasticSearchClientService_IT.java

+
+    @Test
+    public void testDeleteByQuery() throws Exception {
+        String query = "{\n" +


Nitpick, but if ES doesn't require the query be pretty-printed, it might be easier to read if it were just a one-line string.

MikeThomsen · 2018-04-28T13:07:06Z

@mattyb149 made those changes.

MikeThomsen · 2018-05-03T09:47:53Z

@mattyb149 ready to merge?

MikeThomsen · 2018-05-23T12:28:53Z

@mattyb149 can we close this out? It's blocking me from fleshing out the rest of the CRUD capabilities around the new client service.

mattyb149 · 2018-05-23T12:37:05Z

Sorry I lost track of this, was reviewing your other ones. I'll try to take a look ASAP

MikeThomsen · 2018-05-23T12:54:43Z

Thanks and np. We need more committers :)

mattyb149 · 2018-05-23T13:14:26Z

+1 LGTM, ran the build with tests and contrib-check, also tested various situations (with/out incoming connections, valid/invalid queries, etc.) and all looks good. Thanks for this feature! Merging to master

MikeThomsen force-pushed the NIFI-5052 branch from c179eb5 to a7cc6b6 Compare April 7, 2018 11:12

zenfenan reviewed Apr 7, 2018

View reviewed changes

MikeThomsen added 2 commits April 10, 2018 12:27

NIFI-5052 Added DeleteByQuery ElasticSearch processor.

57f137d

NIFI-5052 fixed checkstyle issue.

0e13854

zenfenan reviewed Apr 11, 2018

View reviewed changes

NIFI-5052 Added changes requested in a code review.

3b8810f

MikeThomsen force-pushed the NIFI-5052 branch from 0b0ce7e to 3b8810f Compare April 11, 2018 19:29

zenfenan reviewed Apr 12, 2018

View reviewed changes

NIFI-5052 Fixed a typo.

c397a71

mattyb149 reviewed Apr 26, 2018

View reviewed changes

NIFI-5052 Added changes requested in a code review.

1e5dc53

asfgit closed this in 3a29c1e May 23, 2018

NIFI-5052 Added DeleteByQuery ElasticSearch processor. #2616

NIFI-5052 Added DeleteByQuery ElasticSearch processor. #2616

Conversation

MikeThomsen commented Apr 7, 2018 • edited

For all changes:

For code changes:

For documentation related changes:

Note:

zenfenan left a comment

Choose a reason for hiding this comment

zenfenan commented Apr 7, 2018

MikeThomsen commented Apr 10, 2018

zenfenan commented Apr 10, 2018 • edited

MikeThomsen commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zenfenan left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeThomsen commented Apr 12, 2018

zenfenan commented Apr 12, 2018 • edited

MikeThomsen commented Apr 12, 2018

MikeThomsen commented Apr 23, 2018

JPercivall commented Apr 23, 2018

MikeThomsen commented Apr 23, 2018

mattyb149 commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeThomsen commented Apr 28, 2018

MikeThomsen commented May 3, 2018

MikeThomsen commented May 23, 2018

mattyb149 commented May 23, 2018

MikeThomsen commented May 23, 2018

mattyb149 commented May 23, 2018

MikeThomsen commented Apr 7, 2018 •

edited

zenfenan commented Apr 10, 2018 •

edited

zenfenan left a comment •

edited

zenfenan commented Apr 12, 2018 •

edited