-
Notifications
You must be signed in to change notification settings - Fork 1k
Add expectContinueThresholdInBytes config to S3 #6864
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
Changes from 4 commits
b5444df
b15b984
d30c816
2663f24
5b9d2ae
d4f2f34
037d3fd
dfa4c34
6fca413
75fbb4e
ee89614
90976d6
18c1f65
77ae2de
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 |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "type": "feature", | ||
| "category": "Amazon S3", | ||
| "contributor": "", | ||
| "description": "Add configurable `expectContinueThresholdInBytes` to S3Configuration (default 1 MB). The Expect: 100-continue header is now only added to PutObject and UploadPart requests when the content-length meets or exceeds the threshold, reducing latency overhead for small uploads." | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,7 @@ public final class StreamingRequestInterceptor implements ExecutionInterceptor { | |
|
|
||
| private static final String DECODED_CONTENT_LENGTH_HEADER = "x-amz-decoded-content-length"; | ||
| private static final String CONTENT_LENGTH_HEADER = "Content-Length"; | ||
| private static final long DEFAULT_EXPECT_CONTINUE_THRESHOLD_IN_BYTES = 1_048_576L; | ||
|
Contributor
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. Trying to find a better way to define DEFAULT_EXPECT_CONTINUE_THRESHOLD_IN_BYTES in only one place in case of when changing values, we only changed one of them. But seems difficult.
Contributor
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. Do you think it is better to create a new S3configuration using builder.build() and get the default value out of it when needed at line 66 instead of defining another one here?
Contributor
Author
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. Yeah good call - I think the right place for the default is in S3Configuration as thats how we do other defaults. The previous logic was handling the case where S3config isn't available - I'm not sure when that would ever actually happen. I've updated this logic to just default to using 0 when there isn't a config, effectively disabling the threshold. |
||
|
|
||
| @Override | ||
| public SdkHttpRequest modifyHttpRequest(Context.ModifyHttpRequest context, | ||
|
|
@@ -55,22 +56,24 @@ private boolean shouldAddExpectContinueHeader(Context.ModifyHttpRequest context, | |
| return false; | ||
| } | ||
|
|
||
| if (isExpect100ContinueDisabled(executionAttributes)) { | ||
| S3Configuration s3Config = getS3Configuration(executionAttributes); | ||
|
|
||
| if (s3Config != null && !s3Config.expectContinueEnabled()) { | ||
| return false; | ||
| } | ||
|
|
||
| long threshold = s3Config != null ? s3Config.expectContinueThresholdInBytes() | ||
| : DEFAULT_EXPECT_CONTINUE_THRESHOLD_IN_BYTES; | ||
|
|
||
| return getContentLengthHeader(context.httpRequest()) | ||
| .map(Long::parseLong) | ||
| .map(length -> length != 0L) | ||
| .map(length -> length >= threshold && length != 0L) | ||
| .orElse(true); | ||
| } | ||
|
|
||
| private boolean isExpect100ContinueDisabled(ExecutionAttributes executionAttributes) { | ||
| private S3Configuration getS3Configuration(ExecutionAttributes executionAttributes) { | ||
| ServiceConfiguration serviceConfig = executionAttributes.getAttribute(SdkExecutionAttribute.SERVICE_CONFIG); | ||
| if (serviceConfig instanceof S3Configuration) { | ||
| return !((S3Configuration) serviceConfig).expectContinueEnabled(); | ||
| } | ||
| return false; | ||
| return serviceConfig instanceof S3Configuration ? (S3Configuration) serviceConfig : null; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
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.
Forgot to raise this earlier... should we throw error if expectContinueEnabled is false and expectContinueThresholdInBytes is configured? Don't remember how we handle it for mutlipartEnabled vs multipartConfiguration, let me check
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.
Yeah - good call out - I believe for multipartEnable + multipartConfiguration we do not throw - we just silently ignore the multipartConfiguration.