-
Notifications
You must be signed in to change notification settings - Fork 940
Parallel split for multipart GetObject File Download #6425
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
base: feature/master/large-object-dl
Are you sure you want to change the base?
Parallel split for multipart GetObject File Download #6425
Conversation
…in the onResponse callback. Keep track of all inflight requests.
...src/main/java/software/amazon/awssdk/core/internal/async/ThreadSafeEmittingSubscription.java
Show resolved
Hide resolved
utils/src/main/java/software/amazon/awssdk/utils/ContentRangeParser.java
Show resolved
Hide resolved
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.
Still going through the PR.
core/sdk-core/src/main/java/software/amazon/awssdk/core/async/AsyncResponseTransformer.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformer.java
Outdated
Show resolved
Hide resolved
...src/main/java/software/amazon/awssdk/core/internal/async/ThreadSafeEmittingSubscription.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformer.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformer.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...n/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerPublisher.java
Show resolved
Hide resolved
...n/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerPublisher.java
Show resolved
Hide resolved
...n/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerPublisher.java
Outdated
Show resolved
Hide resolved
core/sdk-core/src/main/java/software/amazon/awssdk/core/async/AsyncResponseTransformer.java
Outdated
Show resolved
Hide resolved
...n/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerPublisher.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Show resolved
Hide resolved
…ents - try/catch in ThreadSafeEmittingSubscription - negative totalParts check - NonLinearMultipartDownloaderSubscriberTckTest tck test
core/sdk-core/src/main/java/software/amazon/awssdk/core/async/AsyncResponseTransformer.java
Outdated
Show resolved
Hide resolved
...a/software/amazon/awssdk/core/internal/async/DefaultAsyncResponseTransformerSplitResult.java
Outdated
Show resolved
Hide resolved
...n/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerPublisher.java
Show resolved
Hide resolved
...n/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerPublisher.java
Outdated
Show resolved
Hide resolved
...n/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerPublisher.java
Show resolved
Hide resolved
...src/main/java/software/amazon/awssdk/core/internal/async/ThreadSafeEmittingSubscription.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public void onResponse(T response) { | ||
Optional<String> contentRangeList = response.sdkHttpResponse().firstMatchingHeader("x-amz-content-range"); |
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.
Since we are depending on header sent by service will this cause issue for Third party tools like minio or gcp ? Since we are erring out if the header is not present it would be good to know its impact when used with Third party s3 like minio or gcp
...va/software/amazon/awssdk/services/s3/internal/multipart/MultipartConfigurationResolver.java
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
- renamed EmittingSubscription, mark it ThreadSafe - Added comments - some other renaming
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...ware/amazon/awssdk/services/s3/internal/multipart/ParallelMultipartDownloaderSubscriber.java
Show resolved
Hide resolved
...ware/amazon/awssdk/services/s3/internal/multipart/ParallelMultipartDownloaderSubscriber.java
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
core/sdk-core/src/main/java/software/amazon/awssdk/core/async/AsyncResponseTransformer.java
Outdated
Show resolved
Hide resolved
.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/EmittingSubscription.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public void onResponse(T response) { | ||
Optional<String> contentRangeList = response.sdkHttpResponse().firstMatchingHeader("x-amz-content-range"); |
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.
Is this logic specific to S3? Could we possibly apply it to a generic streaming service? In general my guess is no, because other streaming APIs don't necessarily support content-range (at least my cursory inspection most I looked at do not support requests or responses with content-range).
Given that - should we keep this class in S3 instead of core?
...ware/amazon/awssdk/services/s3/internal/multipart/ParallelMultipartDownloaderSubscriber.java
Show resolved
Hide resolved
...n/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerPublisher.java
Show resolved
Hide resolved
CompletableFuture<T> delegateFuture = delegate.prepare(); | ||
CompletableFutureUtils.forwardResultTo(delegateFuture, future); | ||
CompletableFutureUtils.forwardExceptionTo(future, delegateFuture); |
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.
Seems like this should be inverted so that IndividualTransformer
does the exception/result forwarding within prepare
. That would clean things up a little
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.
We can't call delegate.prepare()
in the actual prepare()
callback of this IndividualFileTransformer
because we need to have received the SdkResponse
to create the delegate, to know at which offset to write to (after reading the content range header). This is why delegate.prepare()
is called here in onResponse
...n/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerPublisher.java
Show resolved
Hide resolved
.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/EmittingSubscription.java
Outdated
Show resolved
Hide resolved
.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/EmittingSubscription.java
Outdated
Show resolved
Hide resolved
.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/EmittingSubscription.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformer.java
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...ware/amazon/awssdk/services/s3/internal/multipart/ParallelMultipartDownloaderSubscriber.java
Show resolved
Hide resolved
...are/amazon/awssdk/services/s3/internal/multipart/NonLinearMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
…oposal. Renamed to ParallelMultipartDownloaderSubscriber as per PR comment - Other PR comment: Removed unused builder parameter for EmittingSubscription
…n/large-object-merge # Conflicts: # services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartFileDownloadWiremockTest.java
...ware/amazon/awssdk/services/s3/internal/multipart/ParallelMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
...ware/amazon/awssdk/services/s3/internal/multipart/ParallelMultipartDownloaderSubscriber.java
Outdated
Show resolved
Hide resolved
|
Implement parallel download for multipart GetObject in s3 Async Client and Transfer Manager.
Modifications
NonLinearMultipartDownloaderSubscriber
andFileAsyncResponseTransformerPublisher
. Note for reviewer: This is the core of the PR new functionality and review should probably start with those two classes.FileAsyncResponseTransformerPublisher
needs to wrapped to publish progress to the progress updater. This is done inGenericS3TransferManager
andTransferProgressUpdater
supportNonSerial
onSplitResult
ParallelConfiguration
new config class inMultipartConfiguration
for themaxInFlightParts
configposition
,path
andFileTransformerConfiguration
Testing