[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6544: NIFI-9398 add verification to ElasticSearchClientService (with integration tests) and Elasticsearch REST API processors
ChrisSamo632 commented on code in PR #6544: URL: https://github.com/apache/nifi/pull/6544#discussion_r1014646933 ## nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-test-utils/pom.xml: ## @@ -0,0 +1,70 @@ + + +http://maven.apache.org/POM/4.0.0; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance; xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd;> +4.0.0 + +nifi-elasticsearch-bundle +org.apache.nifi +1.19.0-SNAPSHOT + + +nifi-elasticsearch-test-utils +jar + + + +org.apache.nifi +nifi-mock +1.19.0-SNAPSHOT +compile + + + +org.apache.httpcomponents +httpclient + + +com.fasterxml.jackson.core +jackson-databind + + + +org.elasticsearch.client +elasticsearch-rest-client + + + +org.junit.jupiter +junit-jupiter-api + + +org.testcontainers +testcontainers + + +org.testcontainers +elasticsearch Review Comment: Seemed to work for me, maybe I'd changed the scope and not rebuilt the main pom or something, no matter, will add the compile scope -- 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. To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6544: NIFI-9398 add verification to ElasticSearchClientService (with integration tests) and Elasticsearch REST API processors
ChrisSamo632 commented on code in PR #6544:
URL: https://github.com/apache/nifi/pull/6544#discussion_r1014633597
##
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-restapi-processors/src/main/java/org/apache/nifi/processors/elasticsearch/ElasticsearchRestProcessor.java:
##
@@ -117,13 +123,66 @@ default String getQuery(final FlowFile input, final
ProcessContext context, fina
}
default Map getUrlQueryParameters(final ProcessContext
context, final FlowFile flowFile) {
+return getUrlQueryParameters(context, flowFile != null ?
flowFile.getAttributes() : null);
+}
+
+default Map getUrlQueryParameters(final ProcessContext
context, final Map attributes) {
return context.getProperties().entrySet().stream()
// filter non-null dynamic properties
.filter(e -> e.getKey().isDynamic() && e.getValue() != null)
// convert to Map of URL parameter keys and values
.collect(Collectors.toMap(
-e -> e.getKey().getName(),
-e ->
context.getProperty(e.getKey()).evaluateAttributeExpressions(flowFile).getValue()
+e -> e.getKey().getName(),
+e ->
context.getProperty(e.getKey()).evaluateAttributeExpressions(attributes).getValue()
));
}
+
+String VERIFICATION_STEP_INDEX_EXISTS = "Elasticsearch Index Exists";
+
+@Override
+default List verify(final ProcessContext
context, final ComponentLog verificationLogger, final Map
attributes) {
Review Comment:
To be fair, maybe it should be a `FAILED` status for processors that
wouldn't create the index, e.g. Get or Query
--
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.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6544: NIFI-9398 add verification to ElasticSearchClientService (with integration tests) and Elasticsearch REST API processors
ChrisSamo632 commented on code in PR #6544: URL: https://github.com/apache/nifi/pull/6544#discussion_r1014601308 ## nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-test-utils/pom.xml: ## @@ -0,0 +1,70 @@ + + +http://maven.apache.org/POM/4.0.0; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance; xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd;> +4.0.0 + +nifi-elasticsearch-bundle +org.apache.nifi +1.19.0-SNAPSHOT + + +nifi-elasticsearch-test-utils +jar + + + +org.apache.nifi +nifi-mock +1.19.0-SNAPSHOT +compile + + + +org.apache.httpcomponents +httpclient + + +com.fasterxml.jackson.core +jackson-databind + + + +org.elasticsearch.client +elasticsearch-rest-client + + + +org.junit.jupiter +junit-jupiter-api + + +org.testcontainers +testcontainers + + +org.testcontainers +elasticsearch + + +org.testcontainers Review Comment: As per https://github.com/apache/nifi/pull/6544/files#r1014601265 -- 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. To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6544: NIFI-9398 add verification to ElasticSearchClientService (with integration tests) and Elasticsearch REST API processors
ChrisSamo632 commented on code in PR #6544: URL: https://github.com/apache/nifi/pull/6544#discussion_r1014601265 ## nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-test-utils/pom.xml: ## @@ -0,0 +1,70 @@ + + +http://maven.apache.org/POM/4.0.0; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance; xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd;> +4.0.0 + +nifi-elasticsearch-bundle +org.apache.nifi +1.19.0-SNAPSHOT + + +nifi-elasticsearch-test-utils +jar + + + +org.apache.nifi +nifi-mock +1.19.0-SNAPSHOT +compile + + + +org.apache.httpcomponents +httpclient + + +com.fasterxml.jackson.core +jackson-databind + + + +org.elasticsearch.client +elasticsearch-rest-client + + + +org.junit.jupiter +junit-jupiter-api + + +org.testcontainers +testcontainers + + +org.testcontainers +elasticsearch Review Comment: Default `scope` in Maven is `compile`, so don't need to be explicit (although we can if wanted for all dependencies) The `version` is taken from the `testcontainers` in the top-level `nifi/pom.xml`'s `dependencyManagement` for all subsequent child POMs using `testcontainer` imports -- 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. To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6544: NIFI-9398 add verification to ElasticSearchClientService (with integration tests) and Elasticsearch REST API processors
ChrisSamo632 commented on code in PR #6544:
URL: https://github.com/apache/nifi/pull/6544#discussion_r1014601052
##
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-restapi-processors/src/main/java/org/apache/nifi/processors/elasticsearch/ElasticsearchRestProcessor.java:
##
@@ -117,13 +123,66 @@ default String getQuery(final FlowFile input, final
ProcessContext context, fina
}
default Map getUrlQueryParameters(final ProcessContext
context, final FlowFile flowFile) {
+return getUrlQueryParameters(context, flowFile != null ?
flowFile.getAttributes() : null);
+}
+
+default Map getUrlQueryParameters(final ProcessContext
context, final Map attributes) {
return context.getProperties().entrySet().stream()
// filter non-null dynamic properties
.filter(e -> e.getKey().isDynamic() && e.getValue() != null)
// convert to Map of URL parameter keys and values
.collect(Collectors.toMap(
-e -> e.getKey().getName(),
-e ->
context.getProperty(e.getKey()).evaluateAttributeExpressions(flowFile).getValue()
+e -> e.getKey().getName(),
+e ->
context.getProperty(e.getKey()).evaluateAttributeExpressions(attributes).getValue()
));
}
+
+String VERIFICATION_STEP_INDEX_EXISTS = "Elasticsearch Index Exists";
+
+@Override
+default List verify(final ProcessContext
context, final ComponentLog verificationLogger, final Map
attributes) {
+final List results = new ArrayList<>();
+final ConfigVerificationResult.Builder indexExistsResult = new
ConfigVerificationResult.Builder()
+.verificationStepName(VERIFICATION_STEP_INDEX_EXISTS);
+
+ElasticSearchClientService verifyClientService = null;
+String index = null;
+boolean indexExists = false;
+if (context.getProperty(CLIENT_SERVICE).isSet()) {
+verifyClientService =
context.getProperty(CLIENT_SERVICE).asControllerService(ElasticSearchClientService.class);
+if (context.getProperty(INDEX).isSet()) {
+index =
context.getProperty(INDEX).evaluateAttributeExpressions(attributes).getValue();
+try {
+if (verifyClientService.exists(index,
getUrlQueryParameters(context, attributes))) {
+
indexExistsResult.outcome(ConfigVerificationResult.Outcome.SUCCESSFUL)
+.explanation(String.format("Index [%s]
exists", index));
+indexExists = true;
+} else {
+
indexExistsResult.outcome(ConfigVerificationResult.Outcome.SUCCESSFUL)
Review Comment:
Correct, I've tried to describe this further in
https://github.com/apache/nifi/pull/6544#discussion_r1014601005
--
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.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6544: NIFI-9398 add verification to ElasticSearchClientService (with integration tests) and Elasticsearch REST API processors
ChrisSamo632 commented on code in PR #6544:
URL: https://github.com/apache/nifi/pull/6544#discussion_r1014601005
##
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-restapi-processors/src/main/java/org/apache/nifi/processors/elasticsearch/ElasticsearchRestProcessor.java:
##
@@ -117,13 +123,66 @@ default String getQuery(final FlowFile input, final
ProcessContext context, fina
}
default Map getUrlQueryParameters(final ProcessContext
context, final FlowFile flowFile) {
+return getUrlQueryParameters(context, flowFile != null ?
flowFile.getAttributes() : null);
+}
+
+default Map getUrlQueryParameters(final ProcessContext
context, final Map attributes) {
return context.getProperties().entrySet().stream()
// filter non-null dynamic properties
.filter(e -> e.getKey().isDynamic() && e.getValue() != null)
// convert to Map of URL parameter keys and values
.collect(Collectors.toMap(
-e -> e.getKey().getName(),
-e ->
context.getProperty(e.getKey()).evaluateAttributeExpressions(flowFile).getValue()
+e -> e.getKey().getName(),
+e ->
context.getProperty(e.getKey()).evaluateAttributeExpressions(attributes).getValue()
));
}
+
+String VERIFICATION_STEP_INDEX_EXISTS = "Elasticsearch Index Exists";
+
+@Override
+default List verify(final ProcessContext
context, final ComponentLog verificationLogger, final Map
attributes) {
Review Comment:
Sounds like you might have been confused by the NiFi `verify` process as I
was the first time I reviewed this sort of thing (I think there was a
documentation Jira raised off the back of a Slack conversation I started, but
not sure it's been done yet)
Verification in NiFi is an optional thing for users to test their
processor/controller settings before enabling/starting their components to
verify whether they've got things like connection details setup correctly.
Here, I've set the "Index Exists Check" to `SUCCEED` whether the index exists
or not, provided the connection to Elasticsearch works and the expected
not/exists response is returned. So it's OK if the index doesn't exist (and I'd
see that happening not just for templated indices but any index - the index
could be created by the first use of `PutElasticsearchRecord`, for example)
Verification in NiFi is **not** mandatory and if a verify check fails, it
doesn't stop the user from enabling/starting their components - but here, it
should pass whether or not the index exists provided the connection to
Elasticsearch succeeds and there are no authentication issues - the user is
given the opportunity to provide attribute values via the verification window,
e.g. if they're using NiFi Expression Language to specify an index name at
runtime from FlowFile attributes, they can simulate that in the UI for
verification
--
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.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6544: NIFI-9398 add verification to ElasticSearchClientService (with integration tests) and Elasticsearch REST API processors
ChrisSamo632 commented on code in PR #6544:
URL: https://github.com/apache/nifi/pull/6544#discussion_r1014600503
##
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java:
##
@@ -416,6 +495,26 @@ public void refresh(final String index, final Map requestParamet
}
}
+@Override
+public boolean exists(final String index, final Map
requestParameters) {
+try {
+final Response response = performRequest("HEAD", "/" + index,
requestParameters, null);
Review Comment:
I think this is a common thing throughout the methods in this class and was
wondering the same myself (but then forgot) - I'll make a general change to
ensure that a leading `/` is always present but that we don't end up with
duplicates (e.g. `host:port//index`)
--
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.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6544: NIFI-9398 add verification to ElasticSearchClientService (with integration tests) and Elasticsearch REST API processors
ChrisSamo632 commented on code in PR #6544:
URL: https://github.com/apache/nifi/pull/6544#discussion_r1014600354
##
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-restapi-processors/src/main/java/org/apache/nifi/processors/elasticsearch/GetElasticsearch.java:
##
@@ -213,7 +273,7 @@ private void handleElasticsearchException(final
ElasticsearchException ese, Flow
getLogger().error(msg, ese);
if (input != null) {
session.penalize(input);
-input = session.putAttribute(input, "elasticsearch.get.error",
ese.getMessage());
+session.putAttribute(input, "elasticsearch.get.error",
ese.getMessage());
Review Comment:
I was thinking that `input` wasn't then passed back out of the method and so
there was no need to re-assign it, but actually it's used in the subsequent
`session.transfer`, so agree this should be re-instated
--
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.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
