-
Notifications
You must be signed in to change notification settings - Fork 916
Offload completion of execution future to a separate thread #2557
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
Offload completion of execution future to a separate thread #2557
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall. Left some mostly nitpicky feedback.
...transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/S3ClientConfiguration.java
Outdated
Show resolved
Hide resolved
...nager/src/main/java/software/amazon/awssdk/transfer/s3/internal/DefaultS3CrtAsyncClient.java
Outdated
Show resolved
Hide resolved
...nager/src/main/java/software/amazon/awssdk/transfer/s3/internal/DefaultS3CrtAsyncClient.java
Outdated
Show resolved
Hide resolved
...transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/S3ClientConfiguration.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/software/amazon/awssdk/transfer/s3/internal/S3NativeClientConfiguration.java
Show resolved
Hide resolved
...r/src/main/java/software/amazon/awssdk/transfer/s3/internal/S3NativeClientConfiguration.java
Outdated
Show resolved
Hide resolved
int maxPoolSize = Math.max(64, processors * 2); | ||
ThreadPoolExecutor executor = new ThreadPoolExecutor(corePoolSize, maxPoolSize, | ||
10, TimeUnit.SECONDS, | ||
new LinkedBlockingQueue<>(1_000), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the pros and cons of LinkedBlockingQueue
vs. SynchronousQueue
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...c/test/java/software/amazon/awssdk/transfer/s3/internal/S3NativeClientConfigurationTest.java
Show resolved
Hide resolved
...c/test/java/software/amazon/awssdk/transfer/s3/internal/S3NativeClientConfigurationTest.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/software/amazon/awssdk/transfer/s3/internal/S3NativeClientConfiguration.java
Outdated
Show resolved
Hide resolved
utils/src/main/java/software/amazon/awssdk/utils/CompletableFutureUtils.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/software/amazon/awssdk/transfer/s3/internal/S3NativeClientConfiguration.java
Outdated
Show resolved
Hide resolved
...c/test/java/software/amazon/awssdk/transfer/s3/internal/S3NativeClientConfigurationTest.java
Show resolved
Hide resolved
45a9e4f
to
2ce01a5
Compare
2ce01a5
to
005370e
Compare
Kudos, SonarCloud Quality Gate passed! |
…9a62c5f9c Pull request: release <- staging/35a236a8-3c54-40fd-b3d8-e069a62c5f9c
Description
Offload completion of execution future to a separate thread to prevent the CRT threads being blocked by complex operations in the completion chain of a completable future
Motivation and Context
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn install
succeedsLicense