Adds cross region client logic for decorating endpoint provider #4026

cenedhryn · 2023-05-22T16:04:41Z

Motivation and Context

Adds the structure and signatures for the new cross region client to be able to successfully decorate the existing S3 client with an override config containing a decorated endpoint provider.

Modifications

Changes the signature of invokeOperation (back) to CompletableFuture, which is necessary in order to use nonblocking I/O
Modifies paginated methods that return Publisher or Iterable to NOT use invokeOperation in the abstract delegating clients, i.e. back to originating behavior. The correct place to intercept these calls is when each page is called which will be handled by a later PR.
Adds the logic of modifying the request and correctly creating/overriding the RequestOverrideConfig to the cross region client
Adds a BucketEndpointProvider that will contain the cross-region logic (separate PR).

NOTE: There is an unchecked cast back to T after the request has been modified, due to two contributing factors:

The toBuilder method returns an S3 request and the type is lost.
The override config method is present on the subtype (T) and on the Aws-level request, but not on the S3 request.

Adding the override config then returns an Aws request builder. It's not possible to give the compiler enough information to deduce that the request is a T, and we cannot modify the request hierarchy due to backwards compatibility reasons. However, because we control the generation of the subtypes like GetObjectRequest etc, we know that the cast is safe.

Testing

Codegen functional testing to verify the delegating client functionality
S3 functional testing to verify override functionality.

dagnir · 2023-05-22T19:22:53Z

...n/java/software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionAsyncClient.java


        if (!bucket.isPresent()) {
            return operation.apply(request);
        }

-        //TODO: add modifyRequest logic
-        return operation.apply(request);
+        return operation.apply(requestWithDecoratedEndpointProvider(request, bucket.get()))


What does handleOperationFailure do? Should we just return the future from apply instead?

CompletableFuture<T> result = operation.apply(...); result.whenComplete((r, t) -> handleOperationFailure(...)); return result;

The reason for this is that if we return a new future (from whenComplete), then we have to handle the case where the caller cancels the future; however "real" S3 client should already have set that up the future returned from apply

Another thought, if the result gets cancelled by the caller, then we should handle that case in whenComplete as well since we probably don't want to remove that cache entry in that case.

Makes sense. Added this comment to the caching task description.

dagnir · 2023-05-22T19:24:33Z

...in/java/software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionSyncClient.java

+                          .build();
+    }
+
+    //TODO: optimize shared sync/async code


Do we have a backlog task in case we forget?

I didn't think we would since it's quite annoying and I'm pretty sure we wouldn't forget, but just in case I added a task.

sonarqubecloud · 2023-05-22T20:20:34Z

SonarCloud Quality Gate failed.

1 Bug
0 Vulnerabilities
0 Security Hotspots
58 Code Smells

88.6% Coverage
2.6% Duplication

* 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]>

Adds cross region client logic for decorating endpoint provider

790c492

cenedhryn requested a review from a team as a code owner May 22, 2023 16:04

Fixing checkstyle issues

3e3f0c0

cenedhryn requested a review from dagnir May 22, 2023 18:43

dagnir reviewed May 22, 2023

View reviewed changes

dagnir approved these changes May 22, 2023

View reviewed changes

cenedhryn merged commit 2a15452 into cross_region_bucket_access/project_branch May 22, 2023

cenedhryn deleted the salande/cross-region-endpoint-structure branch May 22, 2023 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Adds cross region client logic for decorating endpoint provider #4026

Adds cross region client logic for decorating endpoint provider #4026

Uh oh!

cenedhryn commented May 22, 2023 •

edited

Loading

Uh oh!

dagnir May 22, 2023

Uh oh!

dagnir May 22, 2023

Uh oh!

cenedhryn May 22, 2023

Uh oh!

dagnir May 22, 2023

Uh oh!

cenedhryn May 22, 2023

Uh oh!

sonarqubecloud bot commented May 22, 2023

Uh oh!

Uh oh!

Adds cross region client logic for decorating endpoint provider #4026

Adds cross region client logic for decorating endpoint provider #4026

Uh oh!

Conversation

cenedhryn commented May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Modifications

Testing

Uh oh!

dagnir May 22, 2023

Choose a reason for hiding this comment

Uh oh!

dagnir May 22, 2023

Choose a reason for hiding this comment

Uh oh!

cenedhryn May 22, 2023

Choose a reason for hiding this comment

Uh oh!

dagnir May 22, 2023

Choose a reason for hiding this comment

Uh oh!

cenedhryn May 22, 2023

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented May 22, 2023

Uh oh!

Uh oh!

cenedhryn commented May 22, 2023 •

edited

Loading