[TM DownloadDirectory Part3] Various updates on downloadDirectory #3020

zoewangg · 2022-02-09T22:11:33Z

Modifications

Limit the number of concurrent downloads by creating a dedicated S3ObjectSubscriber with backpressure support
Create nonexistent parent directories
Strip prefix from the path if it's not empty. Feature request from v1 Fix downloadDirectory method in TransferManager aws-sdk-java#1321
Add more Javadocs
Don't pass delimiter by default to avoid potentially high number of listObjectsV2 calls. Feature request from v1: Performance Bottleneck: TransferManager downloadDirectory aws-sdk-java#1215

Testing

Added unit tests

Screenshots (if appropriate)

Types of changes

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

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
I have added tests to cover my changes
All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

I confirm that this pull request can be released under the Apache 2 license

…t download file 2. create nonexistent parent directories 3. normalize key if prefix is not empty

…to zoewang/tm-downloadDirectoryPart3

…ObjectsV2 calls

...nager/src/main/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelper.java

joviegas · 2022-02-09T23:44:22Z

...nager/src/main/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelper.java

+         */
+        private static String normalizeKey(DownloadDirectoryRequest downloadDirectoryRequest, S3Object s3Object,
+                                           String delimiter) {
+            return downloadDirectoryRequest.prefix()


NonBlocker , just suggestion :
was curious if we can use regex for this , something like ^(.+)\/([^\/]+)$ where / 6th character is the delimiter

String regexExpression = String.format("^(.+)\\%s([^\\/]+)$", delimiter);

I'm not sure if it's possible to define a regex because we don't know the length of the prefix beforehand.

...nager/src/main/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelper.java

core/sdk-core/src/main/java/software/amazon/awssdk/core/async/SdkPublisher.java

Bennett-Lynch · 2022-02-11T19:26:14Z

utils/src/main/java/software/amazon/awssdk/utils/async/AsyncBufferingSubscriber.java

+@SdkProtectedApi
+public class AsyncBufferingSubscriber<T> implements Subscriber<T> {
+    private static final Logger log = Logger.loggerFor(AsyncBufferingSubscriber.class);
+    private final Event<T> completeEvent = new CompleteEvent<>();


Nit: Can be static?

Generic can't be used in a static field. :(

Bennett-Lynch · 2022-02-11T19:27:46Z

utils/src/main/java/software/amazon/awssdk/utils/async/AsyncBufferingSubscriber.java

+    private static final Logger log = Logger.loggerFor(AsyncBufferingSubscriber.class);
+    private final Event<T> completeEvent = new CompleteEvent<>();
+    private final Queue<Event<T>> buffer;
+    private final CompletableFuture<?> future;


Nit: Possible to use a more specific name, like endOfStreamFuture?

Yeah, this future actually finishes after all futures are finished(not end of stream). I'll rename it

Bennett-Lynch · 2022-02-11T19:33:12Z

utils/src/main/java/software/amazon/awssdk/utils/async/AsyncBufferingSubscriber.java

+        Validate.paramNotNull(subscription, "subscription");
+        if (this.subscription != null) {
+            subscription.cancel();
+            return;


Should we return here? If this is unsupported behavior, should we throw or log a warning?

Yeah, per reactive streams spec, we must call subscription.cancel. I can add a logging statement there

A Subscriber MUST call Subscription.cancel() on the given Subscription after an onSubscribe signal if it already has an active Subscription.

utils/src/main/java/software/amazon/awssdk/utils/async/AsyncBufferingSubscriber.java

…ddress feedback

...ager/src/main/java/software/amazon/awssdk/transfer/s3/internal/AsyncBufferingSubscriber.java

sonarqubecloud · 2022-02-14T21:47:47Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
22 Code Smells

82.4% Coverage
2.6% Duplication

* [TM DownloadDirectory Part1] Create POJO classes for download directory (#2993) * Create POJO classes for download directory * Address feedback * Address comments * A couple of minor refactoring on the S3TransferManager (#2997) * [TM DownloadDirectory Part2] Implement download directory in transfer manager (#3010) * Implement download directory in transfer manager * Add more tests and address comments * Remove create parent directories logic and add changelog entry * [TM DownloadDirectory Part3] Various updates on downloadDirectory (#3020) * Implement download directory in transfer manager * Add more tests and address comments * Remove create parent directories logic and add changelog entry * Various updates on downloadDirectory 1. limit the number of concurrent download file 2. create nonexistent parent directories 3. normalize key if prefix is not empty * By default, set delimiter to null to avoid potentially excessive listObjectsV2 calls * Move async buffering logic to SdkPublisher * Move AsyncBufferingSubscriber back to s3-tranfer-manager module and address feedback * remove unnecessary isDelivering.set(false)

zoewangg added 5 commits February 2, 2022 19:51

Implement download directory in transfer manager

994a820

Add more tests and address comments

a799667

Remove create parent directories logic and add changelog entry

f7788b9

Various updates on downloadDirectory 1. limit the number of concurren…

337ea13

…t download file 2. create nonexistent parent directories 3. normalize key if prefix is not empty

Merge remote-tracking branch 'origin/zoewang/tm-downloadDirectory' in…

1a0e81c

…to zoewang/tm-downloadDirectoryPart3

zoewangg requested a review from a team as a code owner February 9, 2022 22:11

By default, set delimiter to null to avoid potentially excessive list…

21aac2a

…ObjectsV2 calls

joviegas reviewed Feb 9, 2022

View reviewed changes

joviegas approved these changes Feb 10, 2022

View reviewed changes

Bennett-Lynch reviewed Feb 10, 2022

View reviewed changes

...nager/src/main/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelper.java Outdated Show resolved Hide resolved

dagnir reviewed Feb 10, 2022

View reviewed changes

...nager/src/main/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelper.java Outdated Show resolved Hide resolved

...nager/src/main/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelper.java Outdated Show resolved Hide resolved

Move async buffering logic to SdkPublisher

e0532a7

Bennett-Lynch reviewed Feb 11, 2022

View reviewed changes

Move AsyncBufferingSubscriber back to s3-tranfer-manager module and a…

7418b6c

…ddress feedback

Bennett-Lynch approved these changes Feb 12, 2022

View reviewed changes

...ager/src/main/java/software/amazon/awssdk/transfer/s3/internal/AsyncBufferingSubscriber.java Outdated Show resolved Hide resolved

remove unnecessary isDelivering.set(false)

29ccd70

zoewangg merged commit 33b1be1 into zoewang/tm-downloadDirectory Feb 14, 2022

zoewangg deleted the zoewang/tm-downloadDirectoryPart3 branch February 14, 2022 21:49

zoewangg mentioned this pull request Feb 14, 2022

Implement download directory in the S3TransferManager #3027

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[TM DownloadDirectory Part3] Various updates on downloadDirectory #3020

[TM DownloadDirectory Part3] Various updates on downloadDirectory #3020

Uh oh!

zoewangg commented Feb 9, 2022 •

edited

Loading

Uh oh!

Uh oh!

joviegas Feb 9, 2022

Uh oh!

zoewangg Feb 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Bennett-Lynch Feb 11, 2022

Uh oh!

zoewangg Feb 11, 2022

Uh oh!

Bennett-Lynch Feb 11, 2022

Uh oh!

zoewangg Feb 11, 2022

Uh oh!

Bennett-Lynch Feb 11, 2022

Uh oh!

zoewangg Feb 11, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Feb 14, 2022

Uh oh!

Uh oh!

[TM DownloadDirectory Part3] Various updates on downloadDirectory #3020

[TM DownloadDirectory Part3] Various updates on downloadDirectory #3020

Uh oh!

Conversation

zoewangg commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Feb 14, 2022

Uh oh!

Uh oh!

zoewangg commented Feb 9, 2022 •

edited

Loading