Skip to content

Integration test cases for Cross region Async and Sync Clients #4128

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

Conversation

joviegas
Copy link
Contributor

@joviegas joviegas commented Jun 21, 2023

Motivation and Context

  • PUT/GET/HEAD/POST/DELETE test cases with cross region

Modifications

  • Added test cases for Sync and Async Client
  • Note : A separate PR will be created for Crt based client since it needs additional changes in sdk-crt wrapper code.

Tested for getAPI service call in S3CrossRegionIntegrationTestBase

Testing with Default and Cross region Client

Tested for getAPI service call in S3CrossRegionIntegrationTestBase

ASYNC_CLIENT

ApiCallDuration (milli) ServiceCallDuration (milli) MarshallingDuration (micro)
GET_301 1036 1015 203
GET_200 128 127 141
FIRST_CALL_CROSS_REGION 1164 1142 344
FIRST_CALL_DEFAULT(SAME_REGION) 406 395 198
SECOND_CALL_CROSS_REGION 27 27 130
SECOND_CALL_DEFAULT_REGION 23 22 112

###SYNC_CLIENT

ApiCallDuration (milli) ServiceCallDuration (milli) MarshallingDuration (micro)
GET_301 686 639 202270
GET_200 108 77 149099
**FIRST_CALL_CROSS_REGION ** 1164 1142 351369
FIRST_CALL_DEFAULT(SAME_REGION) 109 75 198206
SECOND_CALL_CROSS_REGION 23 21 137379
SECOND_CALL_DEFAULT_REGION 22 20 165265

@joviegas joviegas requested a review from a team as a code owner June 21, 2023 22:30
@joviegas joviegas force-pushed the joviegas/cross_region_integ_tests branch from 7f35b68 to cfc2eb6 Compare June 21, 2023 22:35
@joviegas joviegas changed the title Integration test cases for Cross region Sync and Sync Clients Integration test cases for Cross region Async and Sync Clients Jun 21, 2023
import software.amazon.awssdk.services.s3.paginators.ListObjectsV2Publisher;

public class S3AsyncCrossRegionIntegrationTest extends S3CrossRegionIntegrationTestBase {
private S3AsyncClient crossRegionS3Client ;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: space before ;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

DEFAULT_REGION.id()));
assertThat(captureInterceptor.httpMethods()).isEqualTo(Arrays.asList(SdkHttpMethod.DELETE, SdkHttpMethod.DELETE));
assertThat(response).isNotNull();
assertThat(captureInterceptor.getServiceCalls()).isEqualTo(2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it would be flaky if there are ever any retries. Is there some way to rewrite the checks so that it's more resilient to retries?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.
Removed the interceptors
Testing using interceptors is covered in Junits.
Now I just make sure the responses from Cross region calls are as expected

@dagnir dagnir mentioned this pull request Jun 22, 2023
1 task
@joviegas joviegas force-pushed the joviegas/cross_region_integ_tests branch from fc476a6 to 036c756 Compare June 23, 2023 20:02
@joviegas joviegas force-pushed the joviegas/cross_region_integ_tests branch from 036c756 to 914f62b Compare June 23, 2023 21:11
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 5 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 77 Code Smells

89.5% 89.5% Coverage
0.1% 0.1% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@joviegas joviegas merged commit be3fe4c into cross_region_bucket_access/project_branch Jun 23, 2023
joviegas added a commit that referenced this pull request Jul 5, 2023
* 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]>
L-Applin pushed a commit that referenced this pull request Jul 24, 2023
* 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]>
L-Applin pushed a commit that referenced this pull request Jul 24, 2023
* 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]>
@joviegas joviegas deleted the joviegas/cross_region_integ_tests branch January 15, 2025 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants