raunaqmorarka commented on code in PR #17710:
URL: https://github.com/apache/iceberg/pull/17710#discussion_r3815544730
##########
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:
Would this shape work: a small interface in core, say
`HttpResponseRetryPredicate`, that `ExponentialHttpRequestRetryStrategy`
consults in addition to its built-in rules. `HTTPClient` would load the AWS
implementation from iceberg-aws reflectively whenever SigV4 auth is enabled,
the same way `AuthManagers` resolves the SigV4 `AuthManager` by hardcoded class
name. The `x-amzn-ErrorType` parsing moves to that implementation, so Glue and
S3 Tables endpoints get the retry automatically and this class stays generic.
The check only fires when the response carries the AWS error type header, so
other servers are unaffected.
If this direction sounds right I'll restructure the PR accordingly.
--
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]
