singhpk234 commented on code in PR #17710:
URL: https://github.com/apache/iceberg/pull/17710#discussion_r3808765178
##########
core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java:
##########
@@ -145,14 +155,42 @@ public boolean retryRequest(HttpResponse response, int
execCount, HttpContext co
// A retry is permitted if all the following conditions are met:
// 1. The maximum retry count has not been exceeded.
- // 2. The response code is considered retryable, for one of the following
reasons:
- // - It's in a predefined list of retriable codes.
+ // 2. The response is considered retryable, for one of the following
reasons:
+ // - The response code is in a predefined list of retriable codes.
// - The request is idempotent, and the response code indicates a retry
is safe.
// - The response code is '503 Service Unavailable' and includes a
'Retry-After' header.
+ // - The response reports an AWS throttling error, which can use a 400
response code.
return execCount <= maxRetries
&& (retriableCodes.contains(response.getCode())
|| shouldRetryIdempotent(request, response.getCode())
- || is503Retryable);
+ || is503Retryable
+ || isAwsThrottling(response));
+ }
+
+ private boolean isAwsThrottling(HttpResponse response) {
+ if (response.getCode() < HttpStatus.SC_BAD_REQUEST) {
+ return false;
+ }
+
+ Header header = response.getFirstHeader(AWS_ERROR_TYPE_HEADER);
+ if (header == null || header.getValue() == null) {
+ return false;
+ }
+
+ // the header value may be formatted as 'ErrorType', 'ErrorType:uri',
'namespace#ErrorType',
+ // or 'namespace#ErrorType:uri'
+ String errorType = header.getValue();
+ int colon = errorType.indexOf(':');
+ if (colon >= 0) {
+ errorType = errorType.substring(0, colon);
+ }
+
+ int hash = errorType.lastIndexOf('#');
+ if (hash >= 0) {
+ errorType = errorType.substring(hash + 1);
+ }
+
+ return AWS_THROTTLING_ERROR_TYPES.contains(errorType);
}
Review Comment:
I wonder if this is something glue should fix instead ? 400 means malformed
request or something wrong in the request
https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L5506
i understand spec doesn't clearly state that 429 is the error code to throw,
but this seems incorrect to me to rely on response header that too of 400
status
i would recommend keeping this class as generic to rest as possible and not
add implementation specific semantics here
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
