-
Notifications
You must be signed in to change notification settings - Fork 916
Wraps s3 client in cross regional client when enabled #4080
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
Wraps s3 client in cross regional client when enabled #4080
Conversation
3805867
to
c3d4502
Compare
c3d4502
to
45f255c
Compare
utils/src/main/java/software/amazon/awssdk/utils/ConditionalDecorator.java
Show resolved
Hide resolved
...es/s3/src/main/java/software/amazon/awssdk/services/s3/internal/client/S3ClientComposer.java
Outdated
Show resolved
Hide resolved
"hasAccelerateModeEnabledProperty":true | ||
}, | ||
"customRetryPolicy": "software.amazon.TestRetryPolicy", | ||
"clientComposerFactory": "software.amazon.awssdk.services.builder.ClientComposerFactory", |
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.
Are other parameters required in the test ?
Is it possible to keep just clientComposerFactory
in this test customization.config ?
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 some unused parameters
return clientComposerFactory; | ||
} | ||
|
||
public void setClientComposerFactory(String clientComposerFactory) { |
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.
Can we add some validator here using regex to match the expected fqcn pattern and error out if it fails to match the supported package patters ?
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.
It doesn't seem like we have any checks for similar parameters (fqcn). What do you suggest would be allowed namespace patterns?
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.
We can do it in future something like this
String pattern = "^(?:[a-zA-Z_$][a-zA-Z\\d_$]*\\.)+[a-zA-Z_$][a-zA-Z\\d_$]*$";
Validate.true(Pattern.matches(pattern, fqcnSuppliedUser) , "Class name is not fqcn");
.collect(Collectors.toList()); | ||
assertThat(ConditionalDecorator.decorate(0, decorators)).isEqualTo(20); | ||
} | ||
} |
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 appreciate the elegant utilization of the ConditionalDecorator, effectively incorporating the decorator pattern and lambda functions in this implementation.
I used String representation just to test for my understanding , you can add this test cases if you want.
Commenting just to save this test case in string form
import static org.assertj.core.api.Assertions.assertThat;
import java.util.*;
import org.junit.jupiter.api.Test;
public class ConditionalDecoratorTest {
@Test
void testStringDecoration() {
StringComposer stringComposer = new StringComposer();
// assertThat(stringComposer.composeString("test", star?, dollar?).getBaseString()).isEqualTo(Result);
assertThat(stringComposer.composeString("test", false, false).getBaseString()).isEqualTo("test");
assertThat(stringComposer.composeString("test", false, true).getBaseString()).isEqualTo("$test$");
assertThat(stringComposer.composeString("test", true, false).getBaseString()).isEqualTo("*test*");
assertThat(stringComposer.composeString("test", true, true).getBaseString()).isEqualTo("$*test*$");
}
class BaseString {
String baseString;
public BaseString(String baseString) {
this.baseString = baseString;
}
protected String getBaseString() {
return baseString;
}
}
class DefaultString extends BaseString {
private DefaultString(String baseString) {
super(baseString);
}
}
class StarDecorated extends BaseString {
private StarDecorated(BaseString baseString) {
super(baseString.getBaseString());
}
@Override
public String getBaseString() {
return "*" + baseString + "*";
}
}
class DollarDecorated extends BaseString {
private DollarDecorated(BaseString baseString) {
super(baseString.getBaseString());
}
@Override
public String getBaseString() {
return "$" + baseString + "$";
}
}
class StringComposer {
public BaseString composeString(String base, boolean starDecorated, boolean dollarDecorated) {
List<ConditionalDecorator<BaseString>> decorators = new ArrayList<>();
decorators.add(ConditionalDecorator.create(testOne -> starDecorated, StarDecorated::new));
decorators.add(ConditionalDecorator.create(testTwo -> dollarDecorated, DollarDecorated::new));
return ConditionalDecorator.decorate(new DefaultString(base), decorators);
}
}
}
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.
Thanks!
Kudos, SonarCloud Quality Gate passed! |
* Adding a generic method invokeOperation for abstract delegating clien… (#3996) Adding a generic method invokeOperation for abstract delegating client to allow overriding behavior changes for all operations * Add endpointProvider in ClientConfig and RequestOverrideConfig. Also add crossRegionBucketAccess in S3Configuration (#3978) * Add endpointProvider in ClientConfig and RequestOverrideConfig. Also add crossRegionBucketAccess in S3Configuration * updated review comments 1 * updated review comments 2 * Adds S3CrossRegion clients and codegen for retrieving bucket name (#4008) * Adds S3CrossRegion clients and codegen for retrieving bucket name * Removes codegen of bucket parameter since getValueForField() is better * Add back overloads * Adds cross region client logic for decorating endpoint provider (#4026) * Adds cross region client logic for decorating endpoint provider * Paginated methods returning publisher or iterable implement logic in … (#4046) * Paginated methods returning publisher or iterable implement logic in interface instead of throwing unsupported exception * Added internal annotation to class * Wraps s3 client in cross regional client when enabled (#4080) * Wraps s3 client in cross regional client when enabled * Adds composer interface * Add User Agent Api name for Cross region API calls (#4105) * Add User Agent Api name for Cross region API calls * Added test case for default client not to have user agent related to cross region * S3CrossRegion Sync and Async Clients Redirect implementation (#4089) * S3CrossRegionSyncClient Redirect implementation * Added implementation for Async client Decorator * Updated older Cross region test cases * Added paramterized test * Async Exception checged to completableException * Updated test cases and changes the Exception handling when HeadBucket Call fails * Handled Anna-karin's comments * Removed async execution of HeadBucket and attached it to the completableFuture of main request * Handled Zoe's comments * Added test case when Redirected after the Region is cached * Changed region constant to Region Type in Tests * Cross region support for CRT Client (#4129) * Cross region support for CRT Client * removing common class * handled review comments * Integration test cases for Cross region Async and Sync Clients (#4128) * Integration test cases for Cross region Sync and Sync Clients * Renamed files as Integration test * Handled CR comments to take care of retries thus captureInterceptor is removed * rebased and added CRT in Integ test * removed sout from integ * Attach crossRegionAccessEnabled as CustomClientParams to a client Builder, also mention of Composers are renamed as decorators (#4151) * Attach crossRegionAccessEnabled as CustomClientParams to a client Builder, also mention of Composers are renamed as decorators * Removed from S3Configurations * Move the optional parameter in getter --------- Co-authored-by: Anna-Karin Salander <[email protected]>
* Adding a generic method invokeOperation for abstract delegating clien… (#3996) Adding a generic method invokeOperation for abstract delegating client to allow overriding behavior changes for all operations * Add endpointProvider in ClientConfig and RequestOverrideConfig. Also add crossRegionBucketAccess in S3Configuration (#3978) * Add endpointProvider in ClientConfig and RequestOverrideConfig. Also add crossRegionBucketAccess in S3Configuration * updated review comments 1 * updated review comments 2 * Adds S3CrossRegion clients and codegen for retrieving bucket name (#4008) * Adds S3CrossRegion clients and codegen for retrieving bucket name * Removes codegen of bucket parameter since getValueForField() is better * Add back overloads * Adds cross region client logic for decorating endpoint provider (#4026) * Adds cross region client logic for decorating endpoint provider * Paginated methods returning publisher or iterable implement logic in … (#4046) * Paginated methods returning publisher or iterable implement logic in interface instead of throwing unsupported exception * Added internal annotation to class * Wraps s3 client in cross regional client when enabled (#4080) * Wraps s3 client in cross regional client when enabled * Adds composer interface * Add User Agent Api name for Cross region API calls (#4105) * Add User Agent Api name for Cross region API calls * Added test case for default client not to have user agent related to cross region * S3CrossRegion Sync and Async Clients Redirect implementation (#4089) * S3CrossRegionSyncClient Redirect implementation * Added implementation for Async client Decorator * Updated older Cross region test cases * Added paramterized test * Async Exception checged to completableException * Updated test cases and changes the Exception handling when HeadBucket Call fails * Handled Anna-karin's comments * Removed async execution of HeadBucket and attached it to the completableFuture of main request * Handled Zoe's comments * Added test case when Redirected after the Region is cached * Changed region constant to Region Type in Tests * Cross region support for CRT Client (#4129) * Cross region support for CRT Client * removing common class * handled review comments * Integration test cases for Cross region Async and Sync Clients (#4128) * Integration test cases for Cross region Sync and Sync Clients * Renamed files as Integration test * Handled CR comments to take care of retries thus captureInterceptor is removed * rebased and added CRT in Integ test * removed sout from integ * Attach crossRegionAccessEnabled as CustomClientParams to a client Builder, also mention of Composers are renamed as decorators (#4151) * Attach crossRegionAccessEnabled as CustomClientParams to a client Builder, also mention of Composers are renamed as decorators * Removed from S3Configurations * Move the optional parameter in getter --------- Co-authored-by: Anna-Karin Salander <[email protected]>
* Adding a generic method invokeOperation for abstract delegating clien… (#3996) Adding a generic method invokeOperation for abstract delegating client to allow overriding behavior changes for all operations * Add endpointProvider in ClientConfig and RequestOverrideConfig. Also add crossRegionBucketAccess in S3Configuration (#3978) * Add endpointProvider in ClientConfig and RequestOverrideConfig. Also add crossRegionBucketAccess in S3Configuration * updated review comments 1 * updated review comments 2 * Adds S3CrossRegion clients and codegen for retrieving bucket name (#4008) * Adds S3CrossRegion clients and codegen for retrieving bucket name * Removes codegen of bucket parameter since getValueForField() is better * Add back overloads * Adds cross region client logic for decorating endpoint provider (#4026) * Adds cross region client logic for decorating endpoint provider * Paginated methods returning publisher or iterable implement logic in … (#4046) * Paginated methods returning publisher or iterable implement logic in interface instead of throwing unsupported exception * Added internal annotation to class * Wraps s3 client in cross regional client when enabled (#4080) * Wraps s3 client in cross regional client when enabled * Adds composer interface * Add User Agent Api name for Cross region API calls (#4105) * Add User Agent Api name for Cross region API calls * Added test case for default client not to have user agent related to cross region * S3CrossRegion Sync and Async Clients Redirect implementation (#4089) * S3CrossRegionSyncClient Redirect implementation * Added implementation for Async client Decorator * Updated older Cross region test cases * Added paramterized test * Async Exception checged to completableException * Updated test cases and changes the Exception handling when HeadBucket Call fails * Handled Anna-karin's comments * Removed async execution of HeadBucket and attached it to the completableFuture of main request * Handled Zoe's comments * Added test case when Redirected after the Region is cached * Changed region constant to Region Type in Tests * Cross region support for CRT Client (#4129) * Cross region support for CRT Client * removing common class * handled review comments * Integration test cases for Cross region Async and Sync Clients (#4128) * Integration test cases for Cross region Sync and Sync Clients * Renamed files as Integration test * Handled CR comments to take care of retries thus captureInterceptor is removed * rebased and added CRT in Integ test * removed sout from integ * Attach crossRegionAccessEnabled as CustomClientParams to a client Builder, also mention of Composers are renamed as decorators (#4151) * Attach crossRegionAccessEnabled as CustomClientParams to a client Builder, also mention of Composers are renamed as decorators * Removed from S3Configurations * Move the optional parameter in getter --------- Co-authored-by: Anna-Karin Salander <[email protected]>
Motivation and Context
Users want cross region ability, but do not want or need to know implementation details, they just want to enable the feature. This change modifies the S3 client builder code to check if cross region is enabled and then wraps the regular client in the cross region client and returns it (as an S3Client/S3AsyncClient).
Modifications
Testing
Unit and functional testing.