You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/11/23 17:18:44 UTC
[GitHub] [nifi] MikeThomsen opened a new pull request, #6712: NIFI-10870 Wrapped debug logger statements with if statements in the …
MikeThomsen opened a new pull request, #6712:
URL: https://github.com/apache/nifi/pull/6712
…elasticsearch bundle and added a debug logger for dumping raw elastic queries.
<!-- Licensed to the Apache Software Foundation (ASF) under one or more -->
<!-- contributor license agreements. See the NOTICE file distributed with -->
<!-- this work for additional information regarding copyright ownership. -->
<!-- The ASF licenses this file to You under the Apache License, Version 2.0 -->
<!-- (the "License"); you may not use this file except in compliance with -->
<!-- the License. You may obtain a copy of the License at -->
<!-- http://www.apache.org/licenses/LICENSE-2.0 -->
<!-- Unless required by applicable law or agreed to in writing, software -->
<!-- distributed under the License is distributed on an "AS IS" BASIS, -->
<!-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -->
<!-- See the License for the specific language governing permissions and -->
<!-- limitations under the License. -->
# Summary
[NIFI-00000](https://issues.apache.org/jira/browse/NIFI-00000)
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [ ] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [ ] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [ ] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [ ] Pull Request based on current revision of the `main` branch
- [ ] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [ ] Build completed using `mvn clean install -P contrib-check`
- [ ] JDK 8
- [ ] JDK 11
- [ ] JDK 17
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6712: NIFI-10870 Wrapped debug logger statements with if statements in the …
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6712:
URL: https://github.com/apache/nifi/pull/6712#discussion_r1030836952
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-restapi-processors/src/main/java/org/apache/nifi/processors/elasticsearch/AbstractPutElasticsearch.java:
##########
@@ -166,7 +166,9 @@ void logElasticsearchDocumentErrors(final IndexOperationResponse response) throw
if (logErrors) {
getLogger().error(output);
} else {
- getLogger().debug(output);
+ if (getLogger().isDebugEnabled()) {
Review Comment:
```suggestion
} else if (getLogger().isDebugEnabled()) {
getLogger().debug(output);
}
```
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6712: NIFI-10870 Wrapped debug logger statements with if statements in the …
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6712:
URL: https://github.com/apache/nifi/pull/6712#discussion_r1030847443
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java:
##########
@@ -790,6 +791,30 @@ private Response performRequest(final String method, final String endpoint, fina
if (entity != null) {
request.setEntity(entity);
}
+
+ if (getLogger().isDebugEnabled()) {
+ ByteArrayOutputStream out = new ByteArrayOutputStream();
+ entity.writeTo(out);
+ out.close();
+
+ StringBuilder builder = new StringBuilder();
Review Comment:
```suggestion
StringBuilder builder = new StringBuilder(1000);
```
Initialising the `StringBuilder` with a large(r) buffer is sensible when we expect to be putting a lot of characters in the String - it's more performant (marginal gains, apprectiating this is only being called if DEBUG logging is enabled for the class), N.B. it's an initial buffer size *not* a limit on how large the String can become within the builder
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6712: NIFI-10870 Wrapped debug logger statements with if statements in the …
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6712:
URL: https://github.com/apache/nifi/pull/6712#discussion_r1030838777
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java:
##########
@@ -790,6 +791,30 @@ private Response performRequest(final String method, final String endpoint, fina
if (entity != null) {
request.setEntity(entity);
}
+
+ if (getLogger().isDebugEnabled()) {
+ ByteArrayOutputStream out = new ByteArrayOutputStream();
+ entity.writeTo(out);
+ out.close();
+
+ StringBuilder builder = new StringBuilder();
+ builder.append("Dumping Elasticsearch REST request...\n")
+ .append("HTTP Method: ")
+ .append(method)
+ .append("\n")
+ .append("Endpoint: ")
+ .append(endpoint)
+ .append("\n")
+ .append("Parameters: ")
+ .append(mapper.writerWithDefaultPrettyPrinter().writeValueAsString(parameters))
Review Comment:
Suggest creating the `Writer` a single time, e.g. in `onEnabled` after the `mapper` has been initialised
Also, `builder` and `out` can be made `final`
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6712: NIFI-10870 Wrapped debug logger statements with if statements in the …
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6712:
URL: https://github.com/apache/nifi/pull/6712#discussion_r1030847443
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java:
##########
@@ -790,6 +791,30 @@ private Response performRequest(final String method, final String endpoint, fina
if (entity != null) {
request.setEntity(entity);
}
+
+ if (getLogger().isDebugEnabled()) {
+ ByteArrayOutputStream out = new ByteArrayOutputStream();
+ entity.writeTo(out);
+ out.close();
+
+ StringBuilder builder = new StringBuilder();
Review Comment:
```suggestion
StringBuilder builder = new StringBuilder(1000);
```
Initialising the `StringBuilder` with a large(r) buffer is sensible when we expect to be putting a lot of characters in the String - it's more performant (marginal gains, apprectiating this is only being called if DEBUG logging is enabled for the class)
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6712: NIFI-10870 Wrapped debug logger statements with if statements in the …
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6712:
URL: https://github.com/apache/nifi/pull/6712#discussion_r1030836017
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-processors/src/main/java/org/apache/nifi/processors/elasticsearch/QueryElasticsearchHttp.java:
##########
@@ -462,7 +463,9 @@ private int getPage(final Response getResponse, final URL url, final ProcessCont
page.add(documentFlowFile);
}
- logger.debug("Elasticsearch retrieved " + responseJson.size() + " documents, routing to success");
+ if (logger.isDebugEnabled()) {
+ logger.debug("Elasticsearch retrieved " + responseJson.size() + " documents, routing to success");
Review Comment:
```suggestion
logger.debug("Elasticsearch retrieved {} documents, routing to success", responseJson.size());
```
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6712: NIFI-10870 Wrapped debug logger statements with if statements in the …
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6712:
URL: https://github.com/apache/nifi/pull/6712#discussion_r1068405018
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-restapi-processors/src/main/java/org/apache/nifi/processors/elasticsearch/AbstractPutElasticsearch.java:
##########
@@ -165,7 +165,7 @@ void logElasticsearchDocumentErrors(final IndexOperationResponse response) throw
if (logErrors) {
getLogger().error(output);
- } else {
+ } else if (getLogger().isDebugEnabled()) {
Review Comment:
Aside from the conditional `isDebugEnabled()` in the service implementation that logs the HTTP entity, this is the only other conditional that seems necessary.
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6712: NIFI-10870 Wrapped debug logger statements with if statements in the …
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6712:
URL: https://github.com/apache/nifi/pull/6712#discussion_r1068486611
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-restapi-processors/src/main/java/org/apache/nifi/processors/elasticsearch/AbstractPutElasticsearch.java:
##########
@@ -165,7 +165,7 @@ void logElasticsearchDocumentErrors(final IndexOperationResponse response) throw
if (logErrors) {
getLogger().error(output);
- } else {
+ } else if (getLogger().isDebugEnabled()) {
Review Comment:
Ah, yeah, forgot to follow up on that form my original comment (https://github.com/apache/nifi/pull/6712#pullrequestreview-1192230945), good catch
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] MikeThomsen commented on pull request #6712: NIFI-10870 Wrapped debug logger statements with if statements in the …
Posted by GitBox <gi...@apache.org>.
MikeThomsen commented on PR #6712:
URL: https://github.com/apache/nifi/pull/6712#issuecomment-1380875595
Going to close this and start over.
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6712: NIFI-10870 Wrapped debug logger statements with if statements in the …
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6712:
URL: https://github.com/apache/nifi/pull/6712#discussion_r1030835765
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-processors/src/main/java/org/apache/nifi/processors/elasticsearch/ScrollElasticsearchHttp.java:
##########
@@ -344,7 +346,9 @@ private void getPage(final Response getResponse, final URL url, final ProcessCon
}
}
builder.append("] }");
- logger.debug("Elasticsearch retrieved " + responseJson.size() + " documents, routing to success");
+ if (logger.isDebugEnabled()) {
+ logger.debug("Elasticsearch retrieved " + responseJson.size() + " documents, routing to success");
Review Comment:
```suggestion
logger.debug("Elasticsearch retrieved {} documents, routing to success", responseJson.size());
```
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] MikeThomsen commented on a diff in pull request #6712: NIFI-10870 Wrapped debug logger statements with if statements in the …
Posted by GitBox <gi...@apache.org>.
MikeThomsen commented on code in PR #6712:
URL: https://github.com/apache/nifi/pull/6712#discussion_r1067322336
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java:
##########
@@ -790,6 +791,30 @@ private Response performRequest(final String method, final String endpoint, fina
if (entity != null) {
request.setEntity(entity);
}
+
+ if (getLogger().isDebugEnabled()) {
+ ByteArrayOutputStream out = new ByteArrayOutputStream();
+ entity.writeTo(out);
+ out.close();
+
+ StringBuilder builder = new StringBuilder();
+ builder.append("Dumping Elasticsearch REST request...\n")
+ .append("HTTP Method: ")
+ .append(method)
+ .append("\n")
+ .append("Endpoint: ")
+ .append(endpoint)
+ .append("\n")
+ .append("Parameters: ")
+ .append(mapper.writerWithDefaultPrettyPrinter().writeValueAsString(parameters))
Review Comment:
Making the change, but noting here that we both seem to agree that according to the docs it should be thread-safe:
> Builder object that can be used for per-serialization configuration of serialization parameters, such as JSON View and root type to use. (and thus fully thread-safe with no external synchronization); new instances are constructed for different configurations. Instances are initially constructed by [ObjectMapper](https://fasterxml.github.io/jackson-databind/javadoc/2.7/com/fasterxml/jackson/databind/ObjectMapper.html) and can be reused in completely thread-safe manner with no explicit synchronization
[Source](https://fasterxml.github.io/jackson-databind/javadoc/2.7/com/fasterxml/jackson/databind/ObjectWriter.html)
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] MikeThomsen commented on pull request #6712: NIFI-10870 Wrapped debug logger statements with if statements in the …
Posted by GitBox <gi...@apache.org>.
MikeThomsen commented on PR #6712:
URL: https://github.com/apache/nifi/pull/6712#issuecomment-1379479990
@ChrisSamo632 Updated.
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] MikeThomsen closed pull request #6712: NIFI-10870 Wrapped debug logger statements with if statements in the …
Posted by GitBox <gi...@apache.org>.
MikeThomsen closed pull request #6712: NIFI-10870 Wrapped debug logger statements with if statements in the …
URL: https://github.com/apache/nifi/pull/6712
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6712: NIFI-10870 Wrapped debug logger statements with if statements in the …
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6712:
URL: https://github.com/apache/nifi/pull/6712#discussion_r1030835309
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-processors/src/main/java/org/apache/nifi/processors/elasticsearch/QueryElasticsearchHttp.java:
##########
@@ -462,7 +463,9 @@ private int getPage(final Response getResponse, final URL url, final ProcessCont
page.add(documentFlowFile);
}
- logger.debug("Elasticsearch retrieved " + responseJson.size() + " documents, routing to success");
+ if (logger.isDebugEnabled()) {
+ logger.debug("Elasticsearch retrieved " + responseJson.size() + " documents, routing to success");
Review Comment:
Should use a log formatting String instead, like other logger calls (rther than String concatenation)
--
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-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org