NIFI-6403: Adding Elasticsearch7 support to HTTP processors by gresockj · Pull Request #4153 · apache/nifi

gresockj · 2020-03-19T17:28:26Z

Thank you for submitting a contribution to Apache NiFi.

Please provide a short description of the PR here:

Description of PR

Allows Elasticsearch 7 in Elasticsearch HTTP processors.

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)?
[x Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not squash or use --force when pushing to allow for clean monitoring of changes.

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?
Have you verified that the full build is successful on both JDK 8 and JDK 11?
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.

MikeThomsen · 2020-03-25T11:32:33Z

@gresockj Are you aware of the "REST API Bundle" processors that NiFi has that use the official Elastic REST client?

ijokarumawak

@gresockj I took a look the changes and left several comments. Please check. Thanks for your contribution!

ijokarumawak · 2020-05-15T13:35:59Z

...rocessors/src/main/java/org/apache/nifi/processors/elasticsearch/FetchElasticsearchHttp.java

-            .required(false)
+            .description("The type of this document (if empty, the first document matching the identifier across all types will be retrieved).  "
+                    + "This must be empty (check 'Set empty string') or '_doc' for Elasticsearch 7.0+.")
+            .required(true)


Do we need to make it mandatory?? If so, would you elaborate why? I guess ElasticsearchTypeValidator should be enough. Probably you want it to align with other processors. If so, I'd suggest make it optional in all this processor family because ElasticsearchTypeValidator can handle it properly. And users will not have to set empty string.

I agree this should be optional everywhere and a sensible (cross-ES version) operation/value provided where this is unset, e.g. omit _type from bulk operations, no effect for queries, use _all for GET (Fetch) for backward compatability (users using ES 7+ will need to set this to an appropriate value, e.g. _doc)

ijokarumawak · 2020-05-15T13:42:19Z

...c/main/java/org/apache/nifi/processors/elasticsearch/AbstractElasticsearchHttpProcessor.java

+                    new AllowableValue(ElasticsearchVersion.ES_7.name(), "7.x", "Elasticsearch version 7.x"))
+            .defaultValue(ElasticsearchVersion.ES_LESS_THAN_7.name())
+            .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+            .build();


I understand that using allowable value vs supporting Expression language doesn't go well together, but users would expect this property supports Expression language so that they can change ES versions in different environments. Especially in case they want to upgrade ES while NiFi workflow has lots of processors.

I'd probably suggest not having this property at all.

Instead, change the processors so that they can support users calling different versions of ES and let ES deal with its own specific logic.

For example, call ES 7 with a type (other than _doc) - it's actually still allowed but is deprecated and is warned about on the ES side (but would recommend not trying to relocate that logic in nifi as it'll be different again in later versions). This also means the change becomes a lot simpler - make _type optional and omit it from relevant ES calls. Maybe add/update documentation where appropriate (e.g. GET needs _doc or _source)

ijokarumawak · 2020-05-15T13:46:14Z

...ocessors/src/main/java/org/apache/nifi/processors/elasticsearch/ScrollElasticsearchHttp.java

-            .required(false)
+            .description("The type of this document (if empty, searches across all types).  "
+                    + "This must be empty (check 'Set empty string') or '_doc' for Elasticsearch 7.0+.")
+            .required(true)


This may not be mandatory. Please see the previous comment.

ijokarumawak · 2020-05-15T13:54:56Z

@MikeThomsen it's been a while for me to review NiFi processors.. So please forgive my ignorance . But what did you mean by the "REST API Bundle processors"?? #4153 (comment)

ChrisSamo632 · 2020-11-13T22:41:00Z

@MikeThomsen it's been a while for me to review NiFi processors.. So please forgive my ignorance . But what did you mean by the "REST API Bundle processors"?? #4153 (comment)

See processors in https://github.com/apache/nifi/tree/main/nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-restapi-processors, which use https://github.com/apache/nifi/tree/main/nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service

Think these may need updating too, although a quick look suggests that _type is already optional, so maybe all that's needed is to confirm that and ensure there are tests covering the scenario for each processor

I raised NIFI-8001 to add the ability to specify dynamic properties for query string parameters on the REST API ES processors (as this is something I believe to be missing but useful)

NIFI-8002 also to provide a scroll/pit/search_after processor using the REST API ES approach (although users could create this in a flow already with a combination of existing processors, but would need some relatively complex loop logic); and NIFI-8003 to create an _update_by_query (like the existing _delete_by_query)

ChrisSamo632 · 2020-11-15T20:02:21Z

...c/main/java/org/apache/nifi/processors/elasticsearch/AbstractElasticsearchHttpProcessor.java

+            ElasticsearchVersion esVersion = ElasticsearchVersion.valueOf(context
+                    .getProperty(ES_VERSION).getValue());
+            if (esVersion == ElasticsearchVersion.ES_7) {
+                return new ValidationResult.Builder().valid(org.apache.commons.lang3.StringUtils.isBlank(input) || "_doc".equals(input))


_source is also Valid here for ES 7 in a GET document request (i.e. the Fetch processor)

ChrisSamo632 · 2020-11-15T20:10:32Z

...c/main/java/org/apache/nifi/processors/elasticsearch/AbstractElasticsearchHttpProcessor.java

-            sb.append("\", \"_type\": \"");
-            sb.append(StringEscapeUtils.escapeJson(docType));
            sb.append("\"");
+            if (!(StringUtils.isEmpty(docType) | docType == null)){


Should probably use isBlank rather than isEmpty her and no need for the additional null (also use ||, not |)

Same comment for similar changes before

ChrisSamo632 · 2020-11-15T20:14:01Z

...c/main/java/org/apache/nifi/processors/elasticsearch/AbstractElasticsearchHttpProcessor.java

+
+    protected String getFieldIncludeParameter(ElasticsearchVersion esVersion) {
+        return esVersion.equals(ElasticsearchVersion.ES_LESS_THAN_7)
+                ? FIELD_INCLUDE_QUERY_PARAM : FIELD_INCLUDE_QUERY_PARAM_ES7;


This change was actually added in ES 6, not 7

Suggest simply using _source parameter instead as that's the same for all versions. If users want to use the _source_include(s) (or excludes) parameters instead, they can use dynamic properties on the processors

ChrisSamo632 · 2020-11-15T20:14:27Z

...c/main/java/org/apache/nifi/processors/elasticsearch/AbstractElasticsearchHttpProcessor.java

+                ? FIELD_INCLUDE_QUERY_PARAM : FIELD_INCLUDE_QUERY_PARAM_ES7;
+    }
+
+    static class ElasticsearchTypeValidator implements Validator {


Not needed if the ES version parameter goes away

ChrisSamo632 · 2020-11-16T13:57:17Z

Created PR #4667 to address the above comments, in case the alternate approach is favoured.

This closes apache#4153. Signed-off-by: Koji Kawamura <ijokarumawak@apache.org>

NIFI-6403: Adding Elasticsearch7 support to HTTP processors

1592542

ijokarumawak reviewed May 15, 2020

View reviewed changes

ijokarumawak mentioned this pull request May 15, 2020

NIFI-6403: Fixed FIELD_INCLUDE_QUERY_PARAM value for Elastic 6.6+ #3556

Closed

11 tasks

ChrisSamo632 reviewed Nov 15, 2020

View reviewed changes

ChrisSamo632 mentioned this pull request Nov 16, 2020

NIFI-6403 and NIFI-6404 #4667

Closed

13 tasks

asfgit closed this in a3d845a Nov 26, 2020

driesva pushed a commit to driesva/nifi that referenced this pull request Mar 19, 2021

NIFI-6403: Adding Elasticsearch7 support to HTTP processors

d7e0792

This closes apache#4153. Signed-off-by: Koji Kawamura <ijokarumawak@apache.org>

krisztina-zsihovszki pushed a commit to krisztina-zsihovszki/nifi that referenced this pull request Jun 28, 2022

NIFI-6403: Adding Elasticsearch7 support to HTTP processors

d77a926

This closes apache#4153. Signed-off-by: Koji Kawamura <ijokarumawak@apache.org>

Conversation

gresockj commented Mar 19, 2020

Description of PR

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

MikeThomsen commented Mar 25, 2020

Uh oh!

ijokarumawak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijokarumawak commented May 15, 2020

Uh oh!

ChrisSamo632 commented Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisSamo632 Nov 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisSamo632 commented Nov 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ChrisSamo632 commented Nov 13, 2020 •

edited

Loading

ChrisSamo632 Nov 15, 2020 •

edited

Loading