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
Changes from 4 commits
5cca70f
1530c8f
4c6dbd9
8ef972f
72e2fb6
5d4c89a
2e0ca94
548b293
3307aa3
f5b6c1b
eec2964
f71a7c1
e5a4030
ed3b77d
31e9866
fd7568c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,10 @@ | ||
require 'fastlane_core' | ||
|
||
module Supply | ||
# rubocop:disable Metrics/ClassLength | ||
class Uploader | ||
UploadJob = Struct.new(:language, :version_code, :release_notes) | ||
|
||
def perform_upload | ||
FastlaneCore::PrintTable.print_values(config: Supply.config, hide_keys: [:issuer], mask_keys: [:json_key_data], title: "Summary for supply #{Fastlane::VERSION}") | ||
|
||
|
@@ -81,6 +85,21 @@ def perform_upload_meta(version_codes, track_name) | |
version_codes.to_s == "" | ||
end | ||
|
||
upload_worker = FastlaneCore::QueueWorker.new do |job| | ||
begin | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch! will change |
||
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] | ||
job.release_notes << upload_changelog(job.language, job.version_code) unless Supply.config[:skip_upload_changelogs] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not 100% sure if this really works well in a multi-threaded environment? Especially since Ruby mostly works with values, not references like for the concept of Basically you initialize local variable (Also, even if it was passed by reference, what happens if multiple Maybe this works in practice though, because I think that in some way ruby Arrays "act as references"? So adding a value to an array you passed as parameter will add to the original (not to a copied of the passed-by-value array)… 🤔 But even if it does, I'm not sure it feels natural to rely on this. Instead, wdyt about:
This approach would be very similar to the example that is given in the official doc of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that you pointed that out, I think it sounds reasonable. Iirc, ruby passes everything by reference except immutable values (like integers), but just to be on the safe side we can move the declaration of
I don't think the order of the results matters that much since in |
||
UI.message("Uploaded all items for language '#{job.language}'... (#{Time.now - start_time} secs)") | ||
rescue => error | ||
UI.error("#{job.language} - #{error}") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should exit (e.g. Personally if something went wrong with one of the uploads (while some others passed) I'd prefer not to have partially uploaded data in the Play Store (e.g. only half of my screenshots for a single language because Otherwise the overall call to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From my experience I would rather let Also if I'm not mistaken, I think this is the default behaviour of
Tested locally and the exit code is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah TIL that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did a couple more tests and realized that any network or API errors will actually be logged and ignored during the upload process. For instance, the following error was thrown but the process exited with code 0.
The errors I saw in the past tests which made me believe the process exited win non zero code were actually thrown later by In this case, I propose to initialize a "has_errors" bool value before starting the uploads, set it to true when we catch an error, then check its value after the uploads complete and use a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I know it's not the topic of your comment, but FYI the "Google Api Error" you are seeing in your tests are likely unrelated to the changes in your PR btw, but more likely related to a current issue apparently randomly happening for everyone with the Google Api and fastlane — I myself have been having this issue all day even with fastlane
Just to be clear, you'd I can see why you'd want that, but what I don't like with this approach is that it differs from how things were behaving before. In current version of fastlane if one upload fails, then the action stops without trying further to upload other images nor committing the edit and submitting that passed and skipping the uploads that failed. So your suggestion would change that behavior. Given that, but also given that for the current unrelated issue we're having with Google API I was also considering implementing some auto-retry mechanism, I'd then suggest to:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the heads-up about the google api error, I thought it was just me 😄
Actually not, what I was thinking is adding an extra line immediately after starting the worker processes (line 115) to check for the error (something that looks like this): upload_worker.start
UI.abort_with_message!("Some of the upload processes ended with failure! Check the logs above for more infromation.") if has_errors update: No, you were right. The error should be actually thrown after the Edit is committed, otherwise we uploaded the data for nothing. But the last couple of points you made make sense to me now, so in this PR I'll focus to keep the current behaviour then do a follow up in another PR with the "allow submit partial changes" functionality. |
||
end | ||
end | ||
|
||
version_codes.each do |version_code| | ||
UI.user_error!("Could not find folder #{metadata_path}") unless File.directory?(metadata_path) | ||
|
||
|
@@ -89,17 +108,11 @@ def perform_upload_meta(version_codes, track_name) | |
UI.user_error!("Could not find release for version code '#{version_code}' to update changelog") unless release | ||
|
||
release_notes = [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One difference in behavior before/after this change is that before, That might break the logic (and potentially mix up the release notes published when there are different version codes with different You might thus want to keep this reset of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, release_notes) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
) | ||
upload_worker.start | ||
|
||
upload_changelogs(release_notes, release, track, track_name) unless release_notes.empty? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When reviewing this I initially wondered if 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
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.
I'm not sure about this change 🤔
I think the intention of the initial code was to use
FL_NUMBER_OF_THREADS
if defined, but capping that value to10
to not risk using more than 10 threads. While your change allows to use an unlimited number of threads. But having a maximum parallelism is recommended, because adding more threads is not always faster (given the OS and thread scheduler has to constantly switch between threads contexts so there's a sweet spot / critical number where adding more threads is actually detrimental rather than beneficial)Also, given the behavior of
ENV.fetch(key, default)
, the expression used as the first item of the array will be10
if all tested ENV vars arenil
. So that1
value as second item of the array isn't really useful here, because it will never be the value picked.I think the behavior we want is:
DELIVER_NUMBER_OF_THREADS
orFL_NUMBER_OF_THREADS
is defined, use that value, but up to a maximum value of10
(i.e. if those env vars contain a number ≤10, use it, but if the value is >10, cap it to10
instead)[10.to_i, 10].min
=10
At least that's what the initial implementation was doing. While after your changes here this leads to:
DELIVER_NUMBER_OF_THREADS
orFL_NUMBER_OF_THREADS
is defined, use that value as long as it's greater than1
([value.to_i, 1].max
[10.to_i, 1].max
=10
The only case that might not be covered in the original implementation is if one of the env var is defined… but with a value that is
0
or that is not a number (e.g.aaa
). In that case.to_i
would return0
, so['aaa'.to_i, 10].min
would also return0
. This is indeed worth fixing (to use the default instead as if the env var was not defined). But we still need an upper bound of10
to avoid thread-bombing.Maybe this below — i.e. finding the first positive (>0) value after converting all candidates to
to_i
, then clamping between1
and10
— would make more sense to cover all cases and to better match the behavior we want ultimately? wdyt?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.
I was confused as well about what was the intention of this line and how we set the number of threads 😅 , so I initially thought it would be best to do a change that's close to the original behaviour.
Capping the value of the max number of threads spawned by fastlane sounds like a sensible choice and I agree we should do it (we don't know what users might do later 😄). But I would also need a way to "unlock" that maximum cap for the reasons I stated above (which mostly applies to users who understand the consequence), so how about adding another env var called
FL_MAX_NUMBER_OF_THREADS
with a default value of10
? Improving on your suggestion, it would look like so: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.
Yeah sounds like a great balance.