anoopj commented on code in PR #17710:
URL: https://github.com/apache/iceberg/pull/17710#discussion_r3814419559
##########
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:
The fix looks correct to me, but at the wrong layer. This code is generic
HTTP handling and should not do AWS specific handling. One way to model this is
to configure an optional error handler class that can be injected, where you
can do AWS specific handling. (similar to `HTTPClient.REST_TLS_CONFIGURER`)
--
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]
