Add expectContinueThresholdInBytes config to S3#6864
Add expectContinueThresholdInBytes config to S3#6864alextwoods wants to merge 9 commits intomasterfrom
Conversation
alextwoods
added
the
api-surface-area-approved-by-team
Fred1155
left a comment
Fred1155
left a comment
There was a problem hiding this comment.
Should we modify the Expect100ContinueHeaderTest as well?
|
|
||
| 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| // ----------------------------------------------------------------------- | ||
|
|
||
| @Test | ||
| void modifyHttpRequest_putObject_contentLengthAboveThreshold_shouldAddExpectHeader() { |
There was a problem hiding this comment.
nit: can we use parameterized tests?
There was a problem hiding this comment.
Yeah, good idea! Refactored these to be parameterized.
| if (this.expectContinueThresholdInBytes.value() < 0) { | ||
| throw new IllegalArgumentException( | ||
| "expectContinueThresholdInBytes must not be negative, but was: " | ||
| + this.expectContinueThresholdInBytes.value()); | ||
| } |
There was a problem hiding this comment.
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.
Yeah - good call out - I believe for multipartEnable + multipartConfiguration we do not throw - we just silently ignore the multipartConfiguration.
Yeah, good question. I didn't add anything to these tests since the documentation on them says "Functional tests to verify RFC 9110 compliance". And that behavior hasn't changed. Those tests don't test the pre-existing S3 What do you think? |
|
3 participants
Motivation and Context
The
Expect: 100-continueheader allows a client to check whether the server will accept a request before sending the body. While this saves bandwidth on rejected requests, it adds a full round-trip of latency on every successful request. Benchmark data shows this overhead can increase latency by up to 30% for small uploads (under 1 MB) on public internet.Currently the SDK unconditionally adds the header to all
PutObjectandUploadPartrequests (whenexpectContinueEnabledis true). This change introduces a configurable content-length threshold so the header is skipped for small payloads where the cost outweighs the benefit.This is consistent with other AWS SDKs — JavaScript V3 and Kotlin both use a content-length threshold for this header.
Related to #6774 and #6828
Modifications
expectContinueThresholdInBytesconfiguration with default of 1MB.Testing
new + existing unit tests.
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License