Handle errors from OpenSearch by checking status field as well as error #4335
Conversation
…he error body for each bulk response item. Signed-off-by: David Venable <dlv@amazon.com>
dlvenable
requested review from
chenqi0805,
engechas,
graytaylor0,
dinujoh,
kkondaka,
asifsmohammed and
oeyh
as code owners
| @@ -234,7 +235,7 @@ private BulkOperationRequestResponse handleRetriesAndFailures(final Accumulating | |||
| final boolean doRetry = (Objects.isNull(exceptionFromRequest)) ? canRetry(bulkResponse) : canRetry(exceptionFromRequest); | |||
| if (!Objects.isNull(bulkResponse) && retryCount == 1) { // first attempt | |||
| for (final BulkResponseItem bulkItemResponse : bulkResponse.items()) { | |||
| if (bulkItemResponse.error() == null) { | |||
| if (!isItemInError(bulkItemResponse)) { | |||
| sentDocumentsOnFirstAttemptCounter.increment(); | |||
| } | |||
| } | |||
There was a problem hiding this comment.
There's another call to BulkResponseItem::error on line 247 for logging. Not as critical but would be good to have the same check there
| // Skip logging the error for version conflicts | ||
| final ErrorCause error = bulkItemResponse.error(); | ||
| if (error != null && VERSION_CONFLICT_EXCEPTION_TYPE.equals(error.type())) { | ||
| break; |
There was a problem hiding this comment.
Should this be a continue in case there are non version conflict exceptions later in the response items?
There was a problem hiding this comment.
Yes, definitely. Thanks for catching this!
| if (error != null && VERSION_CONFLICT_EXCEPTION_TYPE.equals(error.type())) { | ||
| break; | ||
| } | ||
| LOG.warn("operation = {}, status = {}, error = {}", bulkItemResponse.operationType(), bulkItemResponse.status(), error != null ? error.reason() : ""); | ||
| } | ||
| } | ||
| handleFailures(bulkRequest, bulkResponse.items()); |
There was a problem hiding this comment.
Could we do a similar rest status check on line 290 as well for the BulkResponse?
There was a problem hiding this comment.
Are you referring to this line?
if (bulkResponse.errors()) {
I think you are suggesting that we also check the status of the HTTP response. I don't see any methods supporting this on this class. I believe that the client will throw an exception if the whole HTTP status is an error status. That's what this catch is for:
} catch (Exception e) {
incrementErrorCounters(e);
return handleRetriesAndFailures(bulkRequestForRetry, retryCount, null, e);
}
Signed-off-by: David Venable <dlv@amazon.com>
Description
It appears that for some situations, Amazon OpenSearch Serverless is not including the
errorfield in bulk response errors. This PR includes a check on thestatusfield as well.Issues Resolved
N/A
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.