[TM DownloadDirectory Part1] Create POJO classes for download directory #2993
Conversation
zoewangg
changed the title
| * Download all objects under a specific prefix and bucket to the provided directory. By default, all objects in the entire | ||
| * bucket will be downloaded. |
There was a problem hiding this comment.
More documentation around behaviors such as whether we require the directory to pre-exist or not will be added as we start to work on implementation.
...fer-manager/src/main/java/software/amazon/awssdk/transfer/s3/CompletedDirectoryDownload.java
Outdated
Show resolved
Hide resolved
| public Collection<FailedFileDownload> failedTransfers() { | ||
| return failedTransfers; | ||
| } |
There was a problem hiding this comment.
Discussion, non-blocking: IMO, it's more polite to return the most concrete abstract type, e.g. List instead of Collection here so that people can have constant-time random access to its members. I know that's controversial "because order doesn't matter" here, but I think it's more likely that a user would want to use failedTransfers().get(n) to get the n'th failure than a user would get confused about the order being "inconsistent" just because we returned a List instead of a Collection.
There was a problem hiding this comment.
Haha, @Bennett-Lynch and I had the same debate before! We can definitely open the discussion. The main reason I think Collection is better because it gives us flexibility to change the implementation (although it's very unlikely we will change it).
I think customers can still do failedFileUploads.stream().collect(Collectors.toList()) if they want to iterate the list by index.
There was a problem hiding this comment.
If you've already convinced Bennett that this is the correct decision, then I don't want to make you have to debate this again.
There was a problem hiding this comment.
I was pro-List as well, and still somewhat am, but was mostly persuaded to use Collection since it allows us to use our ConcurrentLinkedQueue directly. I agree that ordering doesn't strictly matter here, but it still seems like a nice-to-have.
There was a problem hiding this comment.
@millems Everyone's opinion matters! Let's vote on it. :)
@Bennett-Lynch We are still copying it to an ArrayList technically.
...fer-manager/src/main/java/software/amazon/awssdk/transfer/s3/CompletedDirectoryDownload.java
Outdated
Show resolved
Hide resolved
...fer-manager/src/main/java/software/amazon/awssdk/transfer/s3/CompletedDirectoryDownload.java
Outdated
Show resolved
Hide resolved
.../s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/DirectoryDownload.java
Show resolved
Hide resolved
...nsfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/DownloadDirectoryRequest.java
Outdated
Show resolved
Hide resolved
...src/main/java/software/amazon/awssdk/transfer/s3/DownloadDirectoryOverrideConfiguration.java
Show resolved
Hide resolved
| public Optional<DownloadDirectoryOverrideConfiguration> overrideConfiguration() { | ||
| return Optional.ofNullable(overrideConfiguration); | ||
| } |
There was a problem hiding this comment.
Maybe the default could be DownloadDirectoryOverrideConfiguration.builder().build() so that callers don't have to deal with the possibility of empty?
There was a problem hiding this comment.
We currently return Optional<AwsRequestOverrideConfiguration> for the request override configuration getter though
|
SonarCloud Quality Gate failed. |
* [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)
…7b8ad058a Pull request: release <- staging/cb91ab5c-fb9f-4124-80e7-ed07b8ad058a
Motivation and Context
Modifications
[TM DownloadDirectory Part1] Create POJO classes for download directory. Implementations will be in a separate PR.
Changes will be merged to a feature branch for now once approved.
Testing
Added unit tests
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License