egalpin commented on code in PR #26424:
URL: https://github.com/apache/beam/pull/26424#discussion_r1290428111
##########
sdks/java/io/elasticsearch/src/main/java/org/apache/beam/sdk/io/elasticsearch/ElasticsearchIO.java:
##########
@@ -1224,26 +1241,9 @@ static class DefaultRetryPredicate implements
RetryPredicate {
this(429);
}
- /** Returns true if the response has the error code for any mutation. */
- private static boolean errorCodePresent(HttpEntity responseEntity, int
errorCode) {
Review Comment:
> So, you believe there's a scenario where the HTTP status code is 200, but
the status in the body is 429.
It's more so that I feel that we cannot remove the `errorCodePresent` method
and rely on http status code alone. I have seen first hand (examples above)
scenarios where 1 out of `n` bulk entities succeeds and therefore the http
status code is 200, even if `n-1` bulk entities failed and had their associated
`status` field in `items` set to something like `400`.
I believe the failure mode for not properly handling http 429 is that the
response body does not contain the key `items` which `errorCodePresent`
currently depends on to look further for `status`. Instead, in the case of
429, `status` is a top-level key in the response body.
So I feel that we need to keep all existing `errorCodePresent` logic, _and_
add additional handling to check the http status code.
--
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]
