You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by GitBox <gi...@apache.org> on 2022/11/03 14:05:05 UTC
[GitHub] [jackrabbit-oak] fabriziofortino opened a new pull request, #743: OAK-9982: avoid direct use of Json.createObject
fabriziofortino opened a new pull request, #743:
URL: https://github.com/apache/jackrabbit-oak/pull/743
This could lead to dependency issues at runtime. Safer to use Elasticseaech java client utilities.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] fabriziofortino merged pull request #743: OAK-9982: avoid direct use of Json.createObject
Posted by GitBox <gi...@apache.org>.
fabriziofortino merged PR #743:
URL: https://github.com/apache/jackrabbit-oak/pull/743
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] fabriziofortino commented on a diff in pull request #743: OAK-9982: avoid direct use of Json.createObject
Posted by GitBox <gi...@apache.org>.
fabriziofortino commented on code in PR #743:
URL: https://github.com/apache/jackrabbit-oak/pull/743#discussion_r1014097607
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/async/ElasticResultRowAsyncIterator.java:
##########
@@ -253,9 +252,8 @@ class ElasticQueryScanner {
}
);
- if (LOG.isTraceEnabled()) {
- LOG.trace("Kicking initial search for query {}", ElasticIndexUtils.toString(searchReq));
- }
+
+ LOG.trace("Kicking initial search for query {}", searchReq);
Review Comment:
that's not needed anymore. Before this change, we had to create the `String` representation because the Elasticsearch client did not return it. The behavior has been fixed with the latest client (updated before this PR). I have removed the if-block because we pass the SearchRequest object reference now. Log4j does not call the toString except when the log level is >= TRACE
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] nit0906 commented on a diff in pull request #743: OAK-9982: avoid direct use of Json.createObject
Posted by GitBox <gi...@apache.org>.
nit0906 commented on code in PR #743:
URL: https://github.com/apache/jackrabbit-oak/pull/743#discussion_r1013188125
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/async/ElasticResultRowAsyncIterator.java:
##########
@@ -343,7 +341,7 @@ public void onFailure(Throwable t) {
}
LOG.error("Error retrieving data for jcr query [{}] :: Corresponding ES query {} : closing scanner, notifying listeners",
- indexPlan.getFilter(), ElasticIndexUtils.toString(query), t);
+ indexPlan.getFilter(), query, t);
Review Comment:
ah never mind - log4j should inherently call that
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] nit0906 commented on a diff in pull request #743: OAK-9982: avoid direct use of Json.createObject
Posted by GitBox <gi...@apache.org>.
nit0906 commented on code in PR #743:
URL: https://github.com/apache/jackrabbit-oak/pull/743#discussion_r1013183233
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/async/ElasticResultRowAsyncIterator.java:
##########
@@ -343,7 +341,7 @@ public void onFailure(Throwable t) {
}
LOG.error("Error retrieving data for jcr query [{}] :: Corresponding ES query {} : closing scanner, notifying listeners",
- indexPlan.getFilter(), ElasticIndexUtils.toString(query), t);
+ indexPlan.getFilter(), query, t);
Review Comment:
@fabriziofortino - should this be query.toString() ?
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] tihom88 commented on a diff in pull request #743: OAK-9982: avoid direct use of Json.createObject
Posted by GitBox <gi...@apache.org>.
tihom88 commented on code in PR #743:
URL: https://github.com/apache/jackrabbit-oak/pull/743#discussion_r1013770841
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/async/ElasticResultRowAsyncIterator.java:
##########
@@ -253,9 +252,8 @@ class ElasticQueryScanner {
}
);
- if (LOG.isTraceEnabled()) {
- LOG.trace("Kicking initial search for query {}", ElasticIndexUtils.toString(searchReq));
- }
+
+ LOG.trace("Kicking initial search for query {}", searchReq);
Review Comment:
I think we should keep these if conditions, as we will rarely enable these trace logs and parameter evaluation will happen for almost everything though it is not being logged.
https://logging.apache.org/log4j/1.2/manual.html#performance
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/async/ElasticResultRowAsyncIterator.java:
##########
@@ -363,9 +361,7 @@ private void scan() {
.highlight(highlight)
.size(getFetchSize(requests++))
);
- if (LOG.isTraceEnabled()) {
- LOG.trace("Kicking new search after query {}", ElasticIndexUtils.toString(searchReq));
- }
+ LOG.trace("Kicking new search after query {}", searchReq);
Review Comment:
same as above.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org