OpenSearch Sink: Add log messages when there is no exception #3532
Conversation
Signed-off-by: Kondaka <krishkdk@amazon.com>
kkondaka
requested review from
chenqi0805,
engechas,
graytaylor0,
dinujoh,
asifsmohammed,
dlvenable and
oeyh
as code owners
| @@ -238,6 +238,13 @@ private BulkOperationRequestResponse handleRetriesAndFailures(final Accumulating | |||
| if (doRetry) { | |||
| if (retryCount % 5 == 0) { | |||
| LOG.warn("Bulk Operation Failed. Number of retries {}. Retrying... ", retryCount, e); | |||
| if (Objects.isNull(e)) { | |||
| for (final BulkResponseItem bulkItemResponse : bulkResponse.items()) { | |||
| if (Objects.nonNull(bulkItemResponse.error())) { | |||
There was a problem hiding this comment.
Are we sure that this will be non-null for successful records? Did you test and see no log here?
There was a problem hiding this comment.
Do you mean to say "it will be NULL for successful records"?
Don't know how to test this path easily. But based on the following existing code, I think it will be set to null for successful records.
if (bulkItemResponse.error() == null) {
sentDocumentsOnFirstAttemptCounter.increment();
}
| if (Objects.nonNull(bulkItemResponse.error())) { | ||
| LOG.warn("operation = {}, error = {}", bulkItemResponse.operationType(), bulkItemResponse.error()); | ||
| } | ||
| } | ||
| handleFailures(bulkRequest, bulkResponse.items()); | ||
| } else { | ||
| handleFailures(bulkRequest, failure); |
There was a problem hiding this comment.
The problem is that right now we only hit this block I think (or at least that is the case when max_retries is reached). But as long as the failure is actually useful this makes sense
There was a problem hiding this comment.
The new change, prints "failure" in both cases. If it is non-null, it will be printed. If it is non-null, I think it would be useful.
| @@ -238,6 +238,13 @@ private BulkOperationRequestResponse handleRetriesAndFailures(final Accumulating | |||
| if (doRetry) { | |||
| if (retryCount % 5 == 0) { | |||
| LOG.warn("Bulk Operation Failed. Number of retries {}. Retrying... ", retryCount, e); | |||
| if (Objects.isNull(e)) { | |||
There was a problem hiding this comment.
The reason isNull exists is to help with streams. I think outside of a stream, we should use x == null.
| if (Objects.isNull(failure)) { | ||
| for (final BulkResponseItem bulkItemResponse : bulkResponse.items()) { | ||
| if (Objects.nonNull(bulkItemResponse.error())) { |
There was a problem hiding this comment.
Again, let's use bulkItemResponse.error() == null.
| @@ -248,7 +255,13 @@ private BulkOperationRequestResponse handleRetriesAndFailures(final Accumulating | |||
| } | |||
|
|
|||
| private void handleFailures(final AccumulatingBulkRequest<BulkOperationWrapper, BulkRequest> bulkRequest, final BulkResponse bulkResponse, final Throwable failure) { | |||
| LOG.warn("Bulk Operation Failed.", failure); | |||
There was a problem hiding this comment.
Do we want the full stack trace here? We know where the exception is thrown from right?
Or should this be the following?
LOG.warn("Bulk Operation Failed. {}", failure.getMessage());
There was a problem hiding this comment.
This function may be called from multiple paths, right?
Signed-off-by: Kondaka <krishkdk@amazon.com>
Description
OpenSearch Sink: Add log messages when there is no exception.
OpenSearch Sink BulkOperation failure logs messages only if there is an exception. It is possible that it fails without exception, it is good to log the error reasons for each operation in those cases as well.
Resolves #3508
Issues Resolved
Resolves #3508
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.