-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AWS: Set better defaults for S3 retry behaviour #11052
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,10 +20,12 @@ | |
|
||
import java.io.Serializable; | ||
import java.net.URI; | ||
import java.time.Duration; | ||
import java.util.Collections; | ||
import java.util.Map; | ||
import java.util.Set; | ||
import java.util.stream.Collectors; | ||
import javax.xml.stream.XMLStreamException; | ||
import org.apache.iceberg.EnvironmentContext; | ||
import org.apache.iceberg.aws.AwsClientProperties; | ||
import org.apache.iceberg.aws.glue.GlueCatalog; | ||
|
@@ -38,6 +40,14 @@ | |
import software.amazon.awssdk.auth.credentials.AnonymousCredentialsProvider; | ||
import software.amazon.awssdk.core.client.config.ClientOverrideConfiguration; | ||
import software.amazon.awssdk.core.client.config.SdkAdvancedClientOption; | ||
import software.amazon.awssdk.core.exception.SdkServiceException; | ||
import software.amazon.awssdk.core.retry.RetryMode; | ||
import software.amazon.awssdk.core.retry.RetryPolicy; | ||
import software.amazon.awssdk.core.retry.backoff.EqualJitterBackoffStrategy; | ||
import software.amazon.awssdk.core.retry.conditions.OrRetryCondition; | ||
import software.amazon.awssdk.core.retry.conditions.RetryCondition; | ||
import software.amazon.awssdk.core.retry.conditions.RetryOnExceptionsCondition; | ||
import software.amazon.awssdk.core.retry.conditions.TokenBucketRetryCondition; | ||
import software.amazon.awssdk.services.s3.S3ClientBuilder; | ||
import software.amazon.awssdk.services.s3.S3Configuration; | ||
import software.amazon.awssdk.services.s3.model.ObjectCannedACL; | ||
|
@@ -393,6 +403,21 @@ public class S3FileIOProperties implements Serializable { | |
*/ | ||
private static final String S3_FILE_IO_USER_AGENT = "s3fileio/" + EnvironmentContext.get(); | ||
|
||
/** Number of times to retry S3 operations. */ | ||
public static final String S3_RETRY_NUM_RETRIES = "s3.retry.num-retries"; | ||
|
||
public static final int S3_RETRY_NUM_RETRIES_DEFAULT = 5; | ||
|
||
/** Minimum wait time to retry a S3 operation */ | ||
public static final String S3_RETRY_MIN_WAIT_MS = "s3.retry.min-wait-ms"; | ||
|
||
public static final long S3_RETRY_MIN_WAIT_MS_DEFAULT = 2_000; // 2 seconds | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would 500 ms make more sense maybe? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was the original value that we did our load tests with but we did not see any benefit from lower value and it caused significantly higher throttle rates |
||
|
||
/** Maximum wait time to retry a S3 read operation */ | ||
public static final String S3_RETRY_MAX_WAIT_MS = "s3.retry.max-wait-ms"; | ||
|
||
public static final long S3_RETRY_MAX_WAIT_MS_DEFAULT = 20_000; // 20 seconds | ||
|
||
private String sseType; | ||
private String sseKey; | ||
private String sseMd5; | ||
|
@@ -423,6 +448,9 @@ public class S3FileIOProperties implements Serializable { | |
private final String endpoint; | ||
private final boolean isRemoteSigningEnabled; | ||
private String writeStorageClass; | ||
private int s3RetryNumRetries; | ||
private long s3RetryMinWaitMs; | ||
private long s3RetryMaxWaitMs; | ||
private final Map<String, String> allProperties; | ||
|
||
public S3FileIOProperties() { | ||
|
@@ -455,6 +483,9 @@ public S3FileIOProperties() { | |
this.isRemoteSigningEnabled = REMOTE_SIGNING_ENABLED_DEFAULT; | ||
this.isS3AccessGrantsEnabled = S3_ACCESS_GRANTS_ENABLED_DEFAULT; | ||
this.isS3AccessGrantsFallbackToIamEnabled = S3_ACCESS_GRANTS_FALLBACK_TO_IAM_ENABLED_DEFAULT; | ||
this.s3RetryNumRetries = S3_RETRY_NUM_RETRIES_DEFAULT; | ||
this.s3RetryMinWaitMs = S3_RETRY_MIN_WAIT_MS_DEFAULT; | ||
this.s3RetryMaxWaitMs = S3_RETRY_MAX_WAIT_MS_DEFAULT; | ||
this.allProperties = Maps.newHashMap(); | ||
|
||
ValidationException.check( | ||
|
@@ -553,6 +584,12 @@ public S3FileIOProperties(Map<String, String> properties) { | |
properties, | ||
S3_ACCESS_GRANTS_FALLBACK_TO_IAM_ENABLED, | ||
S3_ACCESS_GRANTS_FALLBACK_TO_IAM_ENABLED_DEFAULT); | ||
this.s3RetryNumRetries = | ||
PropertyUtil.propertyAsInt(properties, S3_RETRY_NUM_RETRIES, S3_RETRY_NUM_RETRIES_DEFAULT); | ||
this.s3RetryMinWaitMs = | ||
PropertyUtil.propertyAsLong(properties, S3_RETRY_MIN_WAIT_MS, S3_RETRY_MIN_WAIT_MS_DEFAULT); | ||
this.s3RetryMaxWaitMs = | ||
PropertyUtil.propertyAsLong(properties, S3_RETRY_MAX_WAIT_MS, S3_RETRY_MAX_WAIT_MS_DEFAULT); | ||
|
||
ValidationException.check( | ||
keyIdAccessKeyBothConfigured(), | ||
|
@@ -753,6 +790,34 @@ public void setS3AccessGrantsFallbackToIamEnabled(boolean s3AccessGrantsFallback | |
this.isS3AccessGrantsFallbackToIamEnabled = s3AccessGrantsFallbackToIamEnabled; | ||
} | ||
|
||
public int s3RetryNumRetries() { | ||
return s3RetryNumRetries; | ||
} | ||
|
||
public void setS3RetryNumRetries(int s3RetryNumRetries) { | ||
this.s3RetryNumRetries = s3RetryNumRetries; | ||
} | ||
|
||
public long s3RetryMinWaitMs() { | ||
return s3RetryMinWaitMs; | ||
} | ||
|
||
public void setS3RetryMinWaitMs(long s3RetryMinWaitMs) { | ||
this.s3RetryMinWaitMs = s3RetryMinWaitMs; | ||
} | ||
|
||
public long s3RetryMaxWaitMs() { | ||
return s3RetryMaxWaitMs; | ||
} | ||
|
||
public void setS3RetryMaxWaitMs(long s3RetryMaxWaitMs) { | ||
this.s3RetryMaxWaitMs = s3RetryMaxWaitMs; | ||
} | ||
|
||
public long s3RetryTotalWaitMs() { | ||
return (long) s3RetryNumRetries() * s3RetryMaxWaitMs(); | ||
} | ||
|
||
private boolean keyIdAccessKeyBothConfigured() { | ||
return (accessKeyId == null) == (secretAccessKey == null); | ||
} | ||
|
@@ -824,6 +889,65 @@ public <T extends S3ClientBuilder> void applyEndpointConfigurations(T builder) { | |
} | ||
} | ||
|
||
/** | ||
* Override the retry configurations for an S3 client. | ||
* | ||
* <p>Sample usage: | ||
* | ||
* <pre> | ||
* S3Client.builder().applyMutation(s3FileIOProperties::applyRetryConfigurations) | ||
* </pre> | ||
*/ | ||
public <T extends S3ClientBuilder> void applyRetryConfigurations(T builder) { | ||
builder.overrideConfiguration( | ||
config -> | ||
config.retryPolicy( | ||
// Use a retry strategy which will persistently retry throttled exceptions with | ||
// exponential backoff, to give S3 a chance to autoscale. | ||
// LEGACY mode works best here, as it will allow throttled exceptions to use all of | ||
// the configured retry attempts. | ||
RetryPolicy.builder(RetryMode.LEGACY) | ||
.numRetries(s3RetryNumRetries) | ||
.throttlingBackoffStrategy( | ||
EqualJitterBackoffStrategy.builder() | ||
.baseDelay(Duration.ofMillis(s3RetryMinWaitMs)) | ||
.maxBackoffTime(Duration.ofMillis(s3RetryMaxWaitMs)) | ||
.build()) | ||
|
||
// Workaround: add XMLStreamException as a retryable exception. | ||
// https://github.com/aws/aws-sdk-java-v2/issues/5442 | ||
// Without this workaround, we see SDK failures if there's a socket exception | ||
// while parsing an error XML response. | ||
.retryCondition( | ||
OrRetryCondition.create( | ||
RetryCondition.defaultRetryCondition(), | ||
RetryOnExceptionsCondition.create(XMLStreamException.class))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is comprehensive. We've seen issues with exceptions thrown as Example:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, it seems RetryableStage doesn't even catch all the default exceptions mentioned here particularly the UncheckedIO one. Seems like we need to solve this on the SDK side regardless but I am leaning towards leaving them here so that we don't need to come back and revisit this later on. What do you think? I can update aws/aws-sdk-java-v2#5442 with this info as well, not sure whether you know a separate issue open regarding this. |
||
|
||
// Workaround: exclude all 503s from consuming retry tokens. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on this it sounds like you're proposing unlimited retries on 503s and 32 retries on other failures? This feels far too aggressive, but I'm not sure if I'm reading this correctly. Why would we treat bucket HEAD requests differently anyway? (Also, I'm not sure that we actually make that request currently, so I don't think this is necessary) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The retry policy is configured with both a max retry count and a token bucket. Throttling exceptions (503) won't consume tokens from the token bucket, so the retry count is limited by the max retry count of 32. Retryable but non-throttling exceptions will consume tokens from the token bucket. We're reusing the SDK default values for the token bucket, which is 500 tokens, and a non-throttling exception will consume 5 tokens. So, if there's a spike in non-throttling exceptions, we'd expect the token bucket to kick in and effectively disable retries. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's a workaround to an SDK bug where 503s from HEAD requests aren't properly categorized as throttling exceptions. HEAD is used here: https://github.com/apache/iceberg/blob/main/aws/src/main/java/org/apache/iceberg/aws/s3/BaseS3File.java#L80 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I confused the HEAD request here with a Bucket HEAD request (not an object HEAD request). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I see, this is the part I missed. I tried digging into the AWS SDK to see if there's a simpler way to express all this but don't think there is. But this implementation does seem correct for achieving this. |
||
// https://github.com/aws/aws-sdk-java-v2/issues/5414 | ||
// Without this workaround, workloads which see 503s from S3 HEAD will fail | ||
// prematurely. | ||
.retryCapacityCondition( | ||
TokenBucketRetryCondition.builder() | ||
.tokenBucketSize(500) // 500 is the SDK default | ||
.exceptionCostFunction( | ||
e -> { | ||
if (e instanceof SdkServiceException) { | ||
SdkServiceException sdkServiceException = | ||
(SdkServiceException) e; | ||
if (sdkServiceException.isThrottlingException() | ||
|| sdkServiceException.statusCode() == 503) { | ||
return 0; | ||
} | ||
} | ||
|
||
// 5 is the SDK default for non-throttling exceptions | ||
return 5; | ||
}) | ||
.build()) | ||
.build())); | ||
} | ||
|
||
/** | ||
* Add the S3 Access Grants Plugin for an S3 client. | ||
* | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why not use
RetryStrategy
from theretries
API introduced in version2.26
? The current version used in iceberg supports that and technicallyRetryPolicy
has already been flagged as deprecated.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mainly for the retryCapacityCondition that we use in retryPolicy to workaround the HEAD 503 problem which I don't find a good replacement in RetryStrategy. We have an issue open with the SDK to fix that, after that we should be able to move.
Just double checking the support level, it seems like there is no immediate date to stop supporting RetryPolicy as well: https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/retry-strategy.html#migrate-from-retry-policies