Attach crossRegionAccessEnabled as CustomClientParams to a client Builder, also mention of Composers are renamed as decorators#4151
Conversation
…lder, also mention of Composers are renamed as decorators
| clientContextParams.get(JsonClientContextParams.CROSS_REGION_ACCESS_ENABLED) == null, | ||
| "Cross region access enabled has been configured on both ServiceConfiguration and the client level. Please limit this configuration to one location."); |
There was a problem hiding this comment.
Is there a reason we support both places? Is it hard to not support it in S3ClientConfiguration?
There was a problem hiding this comment.
The reason it is supported in both places is that the existing S3ClientContextParams are available in both locations. To access the values of these context parameters, they are accessed through getters on the S3Configuration.
if (serviceConfigBuilder.crossRegionAccessEnabled() != null) {
Validate.validState(Throw Exception in both places);
} else {
serviceConfigBuilder.crossRegionAccessEnabled(clientContextParams
.get(S3ClientContextParams.CROSS_REGION_ACCESS_ENABLED));
}What if the user wants to create a Config object solely to set the Client config in that S3 Configuration?"
"Is it difficult to not support it in S3ClientConfiguration?"
"It's not that difficult. We need to pass ContextParams to the decorator along with the service configuration to determine if it's a cross-region request."
"If we do not support this in S3Configuration, we will not be able to group it along with other client settings like useArnRegionEnabled, multiRegionEnabled, etc."
There was a problem hiding this comment.
Done ,removed it from S3ClientConfiguration
4e0e5a1 to
5c0193a
Compare
5c0193a to
8cb76a2
Compare
millems
left a comment
There was a problem hiding this comment.
Can you review the sonar bugs and mark them as a false positive or fix them?
|
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 <salande@amazon.com>
* 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 <salande@amazon.com>
* 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 <salande@amazon.com>
Attach crossRegionAccessEnabled as CustomClientParams to a client Builder, also mention of Composers are renamed as decorators
Motivation and Context
Modifications
Testing
License