thomasmueller commented on code in PR #639:
URL: https://github.com/apache/jackrabbit-oak/pull/639#discussion_r929967722
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/async/ElasticResultRowAsyncIterator.java:
##########
@@ -111,6 +111,17 @@ public boolean hasNext() {
throw new IllegalStateException("Error reading next result
from Elastic", e);
Review Comment:
I wonder if we should log this...?
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/async/ElasticResultRowAsyncIterator.java:
##########
@@ -111,6 +111,17 @@ public boolean hasNext() {
throw new IllegalStateException("Error reading next result
from Elastic", e);
}
}
+
+ // Check if there are any Throwable filled from onFailure Callback in
the errorReference
+ // Any exception (such as ParseException) during the prefetch (init
scanner) via the async call to ES would be available here
+ // when the cursor is actually being traversed.
+ // This is being done so that we can log the caller stack trace in
case of any exception from ES and not just the trace of the async query thread.
+
+ Throwable error = errorRef.getAndSet(null);
+ if (error != null) {
+ error.fillInStackTrace();
+ LOG.error("Error while fetching Results from Elastic for [{}]",
indexPlan.getFilter().toString(), error);
Review Comment:
Nit: Results => results
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/async/ElasticResultRowAsyncIterator.java:
##########
@@ -323,7 +334,14 @@ public void onSuccess(SearchResponse<ObjectNode>
searchResponse) {
public void onFailure(Throwable t) {
metricHandler.measureFailedQuery(indexNode.getDefinition().getIndexPath(),
System.currentTimeMillis() - searchStartTime);
- LOG.error("Error retrieving data from Elastic: closing scanner,
notifying listeners", t);
+ // Check in case errorRef is already set - this seems unlikely
since we close the scanner once we hit failure.
+ // But still, in case this do happen, we will log a warn.
+ Throwable error = errorRef.getAndSet(t);
+ if (error != null) {
+ LOG.warn("Error reference for async iterator was previously
set to {}. It has now been reset to new error {}", error.getMessage(),
t.getMessage());
+ }
+
+ LOG.error("Error retrieving data from Elastic for query [{}] :
closing scanner, notifying listeners", indexPlan.getFilter().toString(), t);
Review Comment:
I think the "toString()" is not necessary. Removing it also protects against
NullPointerException (which is likely not possible... but still)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
