[DO NOT MERGE] TransferManager: DirectoryUploader & DirectoryDownloader #3288

jterapin · 2025-08-29T19:58:26Z

Experimental prototype

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

To make sure we include your contribution in the release notes, please make sure to add description entry for your changes in the "unreleased changes" section of the CHANGELOG.md file (at corresponding gem). For the description entry, please make sure it lives in one line and starts with Feature or Issue in the correct format.
For generated code changes, please checkout below instructions first:
https://github.com/aws/aws-sdk-ruby/blob/version-3/CONTRIBUTING.md

Thank you for your contribution!

github-actions · 2025-09-09T17:46:53Z

Detected 1 possible performance regressions:

aws-sdk-s3.gem_size_kb - z-score regression: 513.54 -> 517.0. Z-score: 26.42

mullermp

I think this is on the right path but we should break this down into chunks. I think you should first refactor file uploader/downloader to use the executor, and we should release that, then add directory upload/download after that.

mullermp · 2025-09-25T16:42:38Z

gems/aws-sdk-s3/lib/aws-sdk-s3/default_executor.rb

+        @queue = Queue.new
+        @max_threads = options[:max_threads] || 10
+        @pool = []
+        @running = true


Does running state default to true in the initializer for other implementations of this in ruby-concurrency?

mullermp · 2025-09-25T16:43:59Z

gems/aws-sdk-s3/lib/aws-sdk-s3/directory_downloader.rb

+module Aws
+  module S3
+    # Raised when DirectoryDownloader fails to download objects from S3 bucket
+    class DirectoryDownloadError < StandardError


By convention we were putting these in separate files right? If you want to promote the other two (multipart errors) to the files where they are used that's fine too, but let's stay consistent.

mullermp · 2025-09-25T16:45:52Z

gems/aws-sdk-s3/lib/aws-sdk-s3/multipart_file_uploader.rb

      # @option options [Integer] :thread_count (DEFAULT_THREAD_COUNT)
      def initialize(options = {})
        @client = options[:client] || Client.new
        @thread_count = options[:thread_count] || DEFAULT_THREAD_COUNT


The thread count wouldn't matter in this class anymore right?

mullermp · 2025-09-25T16:47:06Z

gems/aws-sdk-s3/lib/aws-sdk-s3/multipart_file_uploader.rb

+        parts = upload_parts(upload_id, source, file_size, options)
+        complete_upload(upload_id, parts, file_size, options)
+      ensure
+        @executor.shutdown if @executor.running? && @options[:executor].nil?


This seems odd. I would think the executor always exists because we provide a default - if nil we still provide a default. Also you would check if it were nil before you attempt to call a method on it?

mullermp · 2025-09-25T16:49:36Z

gems/aws-sdk-s3/lib/aws-sdk-s3/directory_downloader.rb

+        end
+
+        download_opts = options.dup
+        @bucket = bucket


It's odd to set instance state like bucket in a method. Wouldn't you pass bucket, errors, configuration, etc, down to relevant methods and return back errors?

Yeah, I'd agree. I'm not against this being a "one-shot" class - IE, for each directory download, you must create this object again - in which case bucket/destination would be set on initialization instead.

Either way though - setting it as a member variable here is a little weird.

mullermp · 2025-09-25T16:50:05Z

gems/aws-sdk-s3/lib/aws-sdk-s3/directory_downloader.rb

+        downloads = process_download_queue(producer, downloader, download_opts)
+        build_result(downloads)
+      ensure
+        @executor.shutdown unless @options[:executor]


We should always assume an executor I think.

Since this is an internal/private api - I think I'd agree that its reasonable to always require an executor to be provided, and then we don't ever shut it down

alextwoods · 2025-09-25T17:34:28Z

gems/aws-sdk-s3/lib/aws-sdk-s3/default_executor.rb

+      end
+
+      def shutdown
+        @running = false


Should we track state of running -> shutting down -> shutdown? There is a gap in between calling shutdown and being fully shutdown (Where tasks cannot be submitted but tasks are still completing).

Additionally - we should probably offer a "kill" method that will immediately kill all threads. And finally - shutdown methods usually offer the ability to specify a timeout after which remaining tasks are killed (I think you can use the limit value in join for that).

alextwoods · 2025-09-25T17:45:38Z

gems/aws-sdk-s3/lib/aws-sdk-s3/directory_downloader.rb

+        downloads = process_download_queue(producer, downloader, download_opts)
+        build_result(downloads)
+      ensure
+        @executor.shutdown unless @options[:executor]


Since this is an internal/private api - I think I'd agree that its reasonable to always require an executor to be provided, and then we don't ever shut it down

alextwoods · 2025-09-25T17:47:38Z

gems/aws-sdk-s3/lib/aws-sdk-s3/directory_downloader.rb

+        end
+
+        download_opts = options.dup
+        @bucket = bucket


Yeah, I'd agree. I'm not against this being a "one-shot" class - IE, for each directory download, you must create this object again - in which case bucket/destination would be set on initialization instead.

Either way though - setting it as a member variable here is a little weird.

alextwoods · 2025-09-25T17:55:21Z

gems/aws-sdk-s3/lib/aws-sdk-s3/directory_downloader.rb

+      def process_download_queue(producer, downloader, opts)
+        download_attempts = 0
+        completion_queue = Queue.new
+        queue_executor = DefaultExecutor.new


Do we need an executor here? I know this needs to be done async/in a thread, but would it work to just spawn a single thread here?

Edit: NVM, I understand why we're doing this to limit the max concurrent downloads at one time. Maybe add comments or have a parameter/constant for that?

alextwoods · 2025-09-25T17:57:18Z

gems/aws-sdk-s3/lib/aws-sdk-s3/directory_downloader.rb

+          @filter_callback = options[:filter_callback]
+          @errors = options[:errors]
+          @directory_downloader = options[:directory_downloader]
+          @object_queue = SizedQueue.new(100)


make this a constant?

alextwoods · 2025-09-25T18:36:55Z

gems/aws-sdk-s3/lib/aws-sdk-s3/directory_downloader.rb

+        download_attempts = 0
+        completion_queue = Queue.new
+        queue_executor = DefaultExecutor.new
+        while (object = producer.object_queue.shift) != :done


Suggestion on the Producer interface - I think the object_queue should be an internal detail here. I'd lean towards having this implement enumerable, so here you would just do:

producer.each do |object| break if @abord_download # rest of code end

calling each on the producer would start it (so no need to call run) and would handle yielding objects from the internal object_queue.

alextwoods · 2025-09-25T18:38:36Z

gems/aws-sdk-s3/lib/aws-sdk-s3/directory_uploader.rb

+        upload_attempts = 0
+        completion_queue = Queue.new
+        queue_executor = DefaultExecutor.new
+        while (file = producer.file_queue.shift) != :done


Ditto here - I think this producer could follow the same pattern.

alextwoods · 2025-09-25T18:40:42Z

gems/aws-sdk-s3/lib/aws-sdk-s3/directory_uploader.rb

+            uploader.upload(f[:path], opts.merge(key: f[:key]))
+          rescue StandardError => e
+            @errors << e
+            @abort_upload = true unless @ignore_failure


naming nit - what about upload_aborted instead to make it clear that this is state rather than configuration

alextwoods · 2025-09-25T18:45:01Z

gems/aws-sdk-s3/lib/aws-sdk-s3/directory_uploader.rb

+
+        upload_opts = options.dup
+        @ignore_failure = upload_opts.delete(:ignore_failure) || false
+        @errors = []


Same comment as on the DirectoryDownloader - I think we need to decide if these classes are "one shot" or re-usable. If they are re-usable, then we cannot set state on the class.

Because the File uploader/downloader are NOT one shot, I guess I'd lean towards consistency here and having a DirectoryUploader that is re-useable and then I think the easiest way to manage this is to create a private class DirectoryUpload which is one-shot (ie, represents the state of a single directory upload.

As part of this - I think we would move management of the queue executor to the top level object- that is, the same queue executor would be shared across any concurrent DirectoryUploads (so that effectively a configured max_concurrent_file_uploads setting would apply across all concurrent uploads started from the same DirectoryUploader).

alextwoods · 2025-09-25T18:48:54Z

gems/aws-sdk-s3/lib/aws-sdk-s3/file_downloader.rb

+        File.rename(opts[:temp_path], destination) if opts[:temp_path]
      ensure
-        File.delete(@temp_path) if @temp_path && File.exist?(@temp_path)
+        @executor.shutdown if @executor.running? && @options[:executor].nil?


I think there is an issue with this - the FileDowloader can be used to download multiple files right (ie, without creating a new one)? so here, if we shutdown the executor on the first download call, we'll never be able to call download again.

I think we'd probably want to call shutdown only when the downloader is GC'ed (and maybe at that point, we would just call kill on it instead?)

jterapin and others added 12 commits August 27, 2025 08:44

Initial setup

6cb37fe

Minor adjustments

d31ae4d

Directory downloader impl

74ed189

Directory uploader impl

4e8db17

Merge branch 'version-3' into tm-directory-features

098049d

Merge branch 'version-3' into tm-directory-features

8f387d2

Add default executor

7749ba5

Add running check to default executor

99f0de6

Refactor MultipartFileUploader with executor

441fa82

Fix typo in MultipartFileUploader

c792439

Update TM upload file with executor

adce496

Merge branch 'version-3' into tm-directory-features

012c2bc

jterapin added 17 commits September 10, 2025 12:50

Merge branch 'version-3' into tm-directory-features

ee9c9da

Merge from version-3

75df844

Merge branch 'version-3' into tm-directory-features

e5d3245

Merge branch 'version-3' into tm-directory-features

173f5e4

Merge branch 'version-3' into tm-directory-features

cf88ff2

Update to only spawn workers when needed

2758c4d

Update directory uploader

b92d3b3

Update directory uploader

6afb495

Update uploader

86b53e8

Merge branch 'version-3' into tm-directory-features

d587ae1

Add minor improvements to directory uploader

eae3814

Merge branch 'version-3' into tm-directory-features

14010ef

Fix specs

8ab4edc

Minor updates to multipart file uploader

face84d

Minor refactors

36a1e87

Fix options

7dd9f98

Refactor DirectoryUploader

77ab1ba

jterapin added 5 commits September 24, 2025 15:52

Merge version-3 into branch

e843137

Update multipartfileuploader

009127d

Refactor FileDownloader

39912fd

Implement Directory Downloader

f9fb117

Add TODO

d307555

mullermp reviewed Sep 25, 2025

View reviewed changes

alextwoods reviewed Sep 25, 2025

View reviewed changes

[DO NOT MERGE] TransferManager: DirectoryUploader & DirectoryDownloader #3288

Are you sure you want to change the base?

[DO NOT MERGE] TransferManager: DirectoryUploader & DirectoryDownloader #3288

Conversation

jterapin commented Aug 29, 2025

Uh oh!

github-actions bot commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mullermp left a comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Sep 9, 2025 •

edited

Loading