Skip to content
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

Make calls to endpoint discovery non-blocking for asynchornous clients #5205

Merged
merged 19 commits into from May 15, 2024

Conversation

anirudh9391
Copy link
Contributor

@anirudh9391 anirudh9391 commented May 9, 2024

Make endpoint discovery calls fully asynchronous.

Motivation and Context

Conforms to the expectation that asynchronous requests made by non-blocking clients do not block the thread.

Modifications

Created a new internal API to be invoked by asynchronous clients. This new API returns a completableFuture as opposed to waiting on request object completion

Testing

  • Existing Unit tests for Endpoint Discovery pass
  • Codegen tests pass

E2E test validation

    @Test
    public void test() {
        SdkEventLoopGroup group = SdkEventLoopGroup.builder()
                                                   .numberOfThreads(1)
                                                   .build();

        SqsAsyncClient sqs = SqsAsyncClient.builder()
                                           .asyncConfiguration(o -> o.advancedOption(SdkAdvancedAsyncClientOption.FUTURE_COMPLETION_EXECUTOR, Runnable::run))
                                           .region(Region.US_WEST_2)
                                           .httpClient(NettyNioAsyncHttpClient.builder()
                                                                              .eventLoopGroup(group)
                                                                              .build())
                                           .build();

        TimestreamWriteAsyncClient timestream = TimestreamWriteAsyncClient.builder().region(Region.US_WEST_2)
                                                                          .httpClient(NettyNioAsyncHttpClient.builder()
                                                                                                             .eventLoopGroup(group).build())
                                                                          .build();

        CompletableFuture<ListQueuesResponse> future = sqs.listQueues();
        future.whenComplete((r, e) -> {
                                timestream.listDatabases(req -> req.maxResults(1));
                                assertThat(1).isEqualTo(1);
                            }
        ).join();

        assertThat(1).isEqualTo(1);
    }

Screenshots (if appropriate)

Types of changes

  • [ x] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

@anirudh9391 anirudh9391 requested a review from a team as a code owner May 9, 2024 05:50
Copy link
Contributor

@dagnir dagnir left a comment

Choose a reason for hiding this comment

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

Can we include the test we discussed offline for testing a client using Runnable::run for FUTURE_COMPLETION_EXECUTOR?

@@ -193,12 +193,12 @@ public CompletableFuture<TestDiscoveryIdentifiersRequiredResponse> testDiscovery
.overrideConfiguration().flatMap(AwsRequestOverrideConfiguration::credentialsIdentityProvider)
.orElseGet(() -> clientConfiguration.option(AwsClientOption.CREDENTIALS_IDENTITY_PROVIDER))
.resolveIdentity();
endpointFuture = identityFuture.thenApply(credentials -> {
endpointFuture = identityFuture.thenCompose(credentials -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we propagate exception cancellation? What happens if endpoint discovery takes longer than the configured client execution timeout? Would the request fail properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm.. So the cancellation Exception is going to be thrown here

return discoverEndpoint(request).handle(
                (endpointDiscoveryEndpoint, throwable) -> {
                    if (throwable != null) {
                        throw new RuntimeException(throwable);

This will be propogated upwards, unless you mean something else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am wrapping it around an EndpointDiscoveryFailedException

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant if users have client execution timeout enabled and if endpoint discovery takes longer than the configured timeout, the returned future will be cancelled, but will endpoint discovery future also be cancelled?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we just need CompletableFutureUtils.forwardExceptionTo(executeFuture, endpointFuture ); on line 277.

Can we write a test to verify that?

Example: https://github.com/aws/aws-sdk-java-v2/blob/master/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crt/CopyObjectHelperTest.java#L314

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline. Doing this as a follow up item

zoewangg
zoewangg previously approved these changes May 14, 2024
<dependency>
<groupId>software.amazon.awssdk</groupId>
<artifactId>sqs</artifactId>
<version>2.25.53-SNAPSHOT</version>
Copy link
Contributor

@zoewangg zoewangg May 14, 2024

Choose a reason for hiding this comment

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

This needs to be ${awsjavasdk.version}

It seems odd that we are using real service clients in codegen-generated-classes-test, is there any reason we can't use EndpointDiscoveryTestAsyncClient

@zoewangg zoewangg dismissed their stale review May 14, 2024 23:00

Missed tests

Copy link

sonarcloud bot commented May 15, 2024

@anirudh9391 anirudh9391 merged commit e0d2e83 into master May 15, 2024
17 checks passed
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.

None yet

3 participants