-
Notifications
You must be signed in to change notification settings - Fork 853
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
Enable setting multipart to true/false by defining it as a custom client context parameter #4899
Conversation
…ent context param
@@ -68,7 +64,7 @@ private static Supplier<S3AsyncClient> defaultS3AsyncClient() { | |||
if (crtInClasspath()) { | |||
return S3AsyncClient::crtCreate; | |||
} | |||
return S3AsyncClient::create; | |||
return S3AsyncClient::createWithMultipartEnabled; |
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.
return S3AsyncClient::createWithMultipartEnabled; | |
return () -> S3AsyncClient.builder().multipartEnabled(true).build(); |
Could we avoid creating the new static method createWithMultipartEnabled
on the client and just use this instead?
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 i contemplated using this, just thought it might be neater to create a new one. Sure, i can change it to this, it makes sense to have one lesser static method to have to test
}, | ||
{ | ||
"methodName": "createWithMultipartEnabled", | ||
"clientType": "async", | ||
"instanceType": "software.amazon.awssdk.services.s3.S3AsyncClient", | ||
"returnType": "software.amazon.awssdk.services.s3.S3AsyncClient", | ||
"statement": "builder().multipartEnabled(true).build()", | ||
"javaDoc": "Create a {@link S3AsyncClient} with multi-part operations enabled" |
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.
see other comment about static method
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.
Got it. I can change this
public static final AttributeMap.Key<Boolean> MULTIPART_ENABLED_KEY = | ||
new AttributeMap.Key<Boolean>(Boolean.class){}; |
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.
If we dont need it anymore, we should also remove it from customization.config: "contextParamEnabledKey": "S3AsyncClientDecorator.MULTIPART_ENABLED_KEY"
ln245
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.
Ah okay. I missed that one.
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.
Removed it
…ent context parameter
Quality Gate failedFailed conditions |
Multipart upload can be set as a client configuration parameter and is accessible at the time of instantiation. This allows classes that instantiate the S3AsyncClient to validate this parameter.
Modifications
1)Incorporated multiPartEnabled as a custom client configuration parameter by defining it in customization.config. The codegen module generates getter and setter methods for this parameter.
2) By default, set multipartEnabled = true in the S3AsyncClient
3) Removed code that sets multiPart Enabled on the client directly
Testing
Unit Tests to validate that
Screenshots (if appropriate)
Types of changes
Checklist
mvn install
succeedsscripts/new-change
script and following the instructions. Commit the new file created by the script in.changes/next-release
with your changes.License