ijokarumawak commented on a change in pull request #4153:
URL: https://github.com/apache/nifi/pull/4153#discussion_r425806325
##########
File path:
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-processors/src/main/java/org/apache/nifi/processors/elasticsearch/FetchElasticsearchHttp.java
##########
@@ -120,11 +121,11 @@
public static final PropertyDescriptor TYPE = new
PropertyDescriptor.Builder()
.name("fetch-es-type")
.displayName("Type")
- .description("The (optional) type of this document, used by
Elasticsearch for indexing and searching. If the property is empty, "
- + "the first document matching the identifier across all
types will be retrieved.")
- .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)
Review comment:
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.
##########
File path:
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-processors/src/main/java/org/apache/nifi/processors/elasticsearch/ScrollElasticsearchHttp.java
##########
@@ -135,12 +137,11 @@
public static final PropertyDescriptor TYPE = new
PropertyDescriptor.Builder()
.name("scroll-es-type")
.displayName("Type")
- .description(
- "The (optional) type of this query, used by Elasticsearch
for indexing and searching. If the property is empty, "
- + "the the query will match across all types.")
- .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
- .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)
Review comment:
This may not be mandatory. Please see the previous comment.
##########
File path:
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-processors/src/main/java/org/apache/nifi/processors/elasticsearch/AbstractElasticsearchHttpProcessor.java
##########
@@ -138,6 +150,18 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static final PropertyDescriptor ES_VERSION = new
PropertyDescriptor.Builder()
+ .name("elasticsearch-http-version")
+ .displayName("Elasticsearch Version")
+ .description("The major version of elasticsearch (this affects
some HTTP query parameters and the way responses are parsed).")
+ .required(true)
+ .allowableValues(
+ new
AllowableValue(ElasticsearchVersion.ES_LESS_THAN_7.name(), "< 7.0", "Any
version of Elasticsearch less than 7.0"),
+ 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();
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
