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

Allow to override FUTURE_COMPLETION_EXECUTOR for S3Crt client #4841

Closed
2 tasks
scrocquesel opened this issue Jan 21, 2024 · 6 comments · Fixed by #4880
Closed
2 tasks

Allow to override FUTURE_COMPLETION_EXECUTOR for S3Crt client #4841

scrocquesel opened this issue Jan 21, 2024 · 6 comments · Fixed by #4880
Labels
feature-request A feature should be added or improved.

Comments

@scrocquesel
Copy link
Contributor

Describe the feature

S3AsyncClientBuilder allows to configure the executor used to complete future. This configuration is not exposed on the S3CrtAsyncClientBuilder

Use Case

In a quarkus application, futures are executed on a thread with a different TCCL and I encounter class not found exception when CDI proxy tries to create the real object from within the default sdk-async-response executor.

Proposed Solution

Expose advancedOption(SdkAdvancedAsyncClientOption.FUTURE_COMPLETION_EXECUTOR, executor) somewhere in S3CrtAsyncClientBuilder.

Other Information

I think that SCHEDULED_EXECUTOR_SERVICE should also be configured.

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

AWS Java SDK version used

2.23.6

JDK version used

17

Operating System and version

linux

@scrocquesel scrocquesel added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jan 21, 2024
@geniusit
Copy link

geniusit commented Jan 26, 2024

I notice a huge performance benefit when using the injected quarkus ManagedExecutor for the DynamoDbAsyncClient (Netty):

.asyncConfiguration(builder -> builder.advancedOption(SdkAdvancedAsyncClientOption.FUTURE_COMPLETION_EXECUTOR, managedExecutor))

@scrocquesel
Copy link
Contributor Author

I notice a huge performance benefit when using the injected quarkus ManagedExecutor for the DynamoDbAsyncClient (Netty):

.asyncConfiguration(builder -> builder.advancedOption(SdkAdvancedAsyncClientOption.FUTURE_COMPLETION_EXECUTOR, managedExecutor))

Thank you for the feedback. I guess, we could expect the same boost with S3Crt.
BTW, In Quarkus application, if you'd use the Quarkus Amazon extension, this is the default behavior when injecting clients produced by the extension for netty and crt http based async clients. Netty clients will also reuse the Netty EventLoop used by Vert.x if available to avoid having multiple EventLoop competiting the same cpu resources.

@zoewangg zoewangg removed the needs-triage This issue or PR still needs to be triaged. label Feb 1, 2024
Copy link

github-actions bot commented Feb 2, 2024

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.

@scrocquesel
Copy link
Contributor Author

@zoewangg Too bad you didn't add SCHEDULED_EXECUTOR_SERVICE as suggested in other information. Should I open another issue ?

@zoewangg
Copy link
Contributor

zoewangg commented Feb 2, 2024

@scrocquesel ah, sorry, I missed that. SCHEDULED_EXECUTOR_SERVICE is not directly used in S3 CRT client though. It's used in retry layer and waiters, and retry in S3 CRT client is handled in the native CRT library.

@scrocquesel
Copy link
Contributor Author

@scrocquesel ah, sorry, I missed that. SCHEDULED_EXECUTOR_SERVICE is not directly used in S3 CRT client though. It's used in retry layer and waiters, and retry in S3 CRT client is handled in the native CRT library.

Ok, I will close the issue I just opened then. Thank you for the feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants