-
Notifications
You must be signed in to change notification settings - Fork 962
Add "CRT_MEMORY_BUFFER_DISABLED" advanced client option #6579
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
base: master
Are you sure you want to change the base?
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 |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "type": "feature", | ||
| "category": "s3", | ||
| "contributor": "", | ||
| "description": "Add CRT shouldStream config as CRT_MEMORY_BUFFER_DISABLED SDK advanced client option" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,6 +55,15 @@ public final class SdkAdvancedAsyncClientOption<T> extends ClientOption<T> { | |
| public static final SdkAdvancedAsyncClientOption<Executor> FUTURE_COMPLETION_EXECUTOR = | ||
| new SdkAdvancedAsyncClientOption<>(Executor.class); | ||
|
|
||
| /** | ||
| * Advanced configuration for the native S3CrtAsyncClient which only applies for multipart uploads. When set to true, | ||
| * the client will skip buffering the part in native memory before sending the request. Default to false on small objects, | ||
| * and true when the object size exceed a certain threshold. When set to true, the client will also skip | ||
| * buffering for small objects. | ||
| */ | ||
| public static final SdkAdvancedAsyncClientOption<Boolean> CRT_MEMORY_BUFFER_DISABLED = | ||
|
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. Is there as reason this should be on SdkAdvanced options? That seems to imply that it might apply to any async client rather than specifically to S3 CRT only? Is there a reason to not put it on S3CrtAsyncClientBuilder only? I assume its because we want to "hide" it in "advanced" options rather than increasing the discoverability by having it on the S3 CRT client's builder direclty?
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. exactly, we don't want this to be as discoverable as other options. This is what we decided when we discussed about it this week. |
||
| new SdkAdvancedAsyncClientOption<>(Boolean.class); | ||
|
|
||
| private SdkAdvancedAsyncClientOption(Class<T> valueClass) { | ||
| super(valueClass); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,7 @@ | |
| import software.amazon.awssdk.crt.http.HttpProxyEnvironmentVariableSetting; | ||
| import software.amazon.awssdk.crt.http.HttpRequest; | ||
| import software.amazon.awssdk.crt.s3.ChecksumConfig; | ||
| import software.amazon.awssdk.crt.s3.FileIoOptions; | ||
| import software.amazon.awssdk.crt.s3.ResumeToken; | ||
| import software.amazon.awssdk.crt.s3.S3Client; | ||
| import software.amazon.awssdk.crt.s3.S3ClientOptions; | ||
|
|
@@ -127,6 +128,8 @@ private S3ClientOptions createS3ClientOption() { | |
| .ifPresent(options::withConnectTimeoutMs); | ||
| Optional.ofNullable(s3NativeClientConfiguration.httpMonitoringOptions()) | ||
| .ifPresent(options::withHttpMonitoringOptions); | ||
| Optional.ofNullable(s3NativeClientConfiguration.memoryBufferDisabled()) | ||
| .ifPresent(memoryBufferDisabled -> options.withFileIoOptions(new FileIoOptions(memoryBufferDisabled, 0.0, false))); | ||
|
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. Should we be setting the other FileIOptions here? I think the diskThroughputGbps (the 0.0?) applies only shouldStream/memoryBufferDisabled is true - so what if the user has set memoryBufferDisabled=true and we now set the diskThroughput to 0? Ditto on setting directIO - I assume
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. when disk throughput is set to 0, it uses the default value, which is the same as the The default for |
||
| return options; | ||
| } | ||
|
|
||
|
|
||
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.
I find this a little hard to parse and I'm not quite sure I understand the actual behavior... what about: