Skip to content
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

[supply] parallel uploads for meta per language #21474

Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion fastlane_core/lib/fastlane_core/queue_worker.rb
Expand Up @@ -6,7 +6,7 @@ module FastlaneCore
# Use this when you have all the items that you'll process in advance.
# Simply enqueue them to this and call `QueueWorker#start`.
class QueueWorker
NUMBER_OF_THREADS = FastlaneCore::Helper.test? ? 1 : [(ENV["DELIVER_NUMBER_OF_THREADS"] || ENV.fetch("FL_NUMBER_OF_THREADS", 10)).to_i, 10].min
NUMBER_OF_THREADS = FastlaneCore::Helper.test? ? 1 : [ENV["DELIVER_NUMBER_OF_THREADS"], ENV["FL_NUMBER_OF_THREADS"], 10].map(&:to_i).find(&:positive?).clamp(1, ENV.fetch("FL_MAX_NUMBER_OF_THREADS", 10).to_i)

# @param concurrency (Numeric) - A number of threads to be created
# @param block (Proc) - A task you want to execute with enqueued items
Expand Down
35 changes: 23 additions & 12 deletions supply/lib/supply/uploader.rb
@@ -1,6 +1,10 @@
require 'fastlane_core'

module Supply
# rubocop:disable Metrics/ClassLength
class Uploader
UploadJob = Struct.new(:language, :version_code)

def perform_upload
FastlaneCore::PrintTable.print_values(config: Supply.config, hide_keys: [:issuer], mask_keys: [:json_key_data], title: "Summary for supply #{Fastlane::VERSION}")

Expand Down Expand Up @@ -81,25 +85,32 @@ def perform_upload_meta(version_codes, track_name)
version_codes.to_s == ""
end

release_notes = []
upload_worker = FastlaneCore::QueueWorker.new do |job|
UI.message("Preparing uploads for language '#{job.language}'...")
start_time = Time.now
listing = client.listing_for_language(job.language)
upload_metadata(job.language, listing) unless Supply.config[:skip_upload_metadata]
upload_images(job.language) unless Supply.config[:skip_upload_images]
upload_screenshots(job.language) unless Supply.config[:skip_upload_screenshots]
release_notes << upload_changelog(job.language, job.version_code) unless Supply.config[:skip_upload_changelogs]
UI.message("Uploaded all items for language '#{job.language}'... (#{Time.now - start_time} secs)")
rescue => error
UI.abort_with_message!("#{job.language} - #{error}")
end

version_codes.each do |version_code|
UI.user_error!("Could not find folder #{metadata_path}") unless File.directory?(metadata_path)

track, release = fetch_track_and_release!(track_name, version_code)
UI.user_error!("Unable to find the requested track - '#{Supply.config[:track]}'") unless track
UI.user_error!("Could not find release for version code '#{version_code}' to update changelog") unless release

release_notes = []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One difference in behavior before/after this change is that before, release_notes was reset to [] on each iteration of the version_codes loop, while now it's not, and thus is common to all version_codes.

That might break the logic (and potentially mix up the release notes published when there are different version codes with different changelogs/*.txt files.

You might thus want to keep this reset of release_notes = [] inside the loop to cover for this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely observed 😄 I haven't been testing the multi version codes scenario. Will fix.

all_languages.each do |language|
next if language.start_with?('.') # e.g. . or .. or hidden folders
UI.message("Preparing to upload for language '#{language}'...")

listing = client.listing_for_language(language)

upload_metadata(language, listing) unless Supply.config[:skip_upload_metadata]
upload_images(language) unless Supply.config[:skip_upload_images]
upload_screenshots(language) unless Supply.config[:skip_upload_screenshots]
release_notes << upload_changelog(language, version_code) unless Supply.config[:skip_upload_changelogs]
end
upload_worker.batch_enqueue(
# skip . or .. or hidden folders
all_languages.reject { |lang| lang.start_with?('.') }.map { |lang| UploadJob.new(lang, version_code) }
)
upload_worker.start

upload_changelogs(release_notes, release, track, track_name) unless release_notes.empty?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When reviewing this I initially wondered if upload_worder.start was guaranteed to block, i.e. to wait for all jobs in the queue to finish before continuing to the next line. From what I see in the implementation of FastlaneCore::QueueWorker#start, there's a threads.each(:join) at the end of that method so we should be good after all (while if it wasn't the case, upload_changelogs(release_notes, …) would be run too soon…)

I'm telling you this because I feel like this PR should definitively benefit some additional unit tests (aka specs) to help us guarantee the behavior. Especially given how tricky multi-threaded code can sometimes be, with hidden edge cases and all, better get some additional assurance 🙂

Of course, the fact that NUMBER_OF_THREADS = FastlaneCore::Helper.test? ? 1 : … make the code basically work in serial (mono-thread) and not in parallel in the context of tests, will not really allow us to write tests that ensure that parallel execution won't have any major side effects that we wouldn't have thought about in case some actions are done out of order. But it could still help catch up some other side effects (like the release_notes case) so this would still be valuable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing tests is always a good idea. I'll have a look at your suggestions above and take some time this week make that happen 🧪

end
Expand Down