[supply] parallel uploads for meta per language

[ccaruceru]

Checklist

• I've run bundle exec rspec from the root directory to see all new and existing tests pass
• I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
• I see several green ci/circleci builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)
• I've read the Contribution Guidelines
• I've updated the documentation if necessary.

Motivation and Context

Make the Google Play images, screenshots, changelogs and metadata upload faster with the supply command 🚀

Background: at our company we have 24 languages for which we want to upload meta items (app icon, ~20 screenshots, a changelog and app metadata) and the default fastlane's single threaded upload process takes a very long time to complete (28m 10s).

By parallelising the meta upload process per language, the overall upload time can be reduced drastically (down to 2m 13s for the exact same 24 languages and data).

Description

• changes in supply/lib/supply/uploader.rb:
  • use a FastlaneCore::QueueWorker while doing the perform_upload_meta
  • add a new Struct called UploadJob to pass the parameters to the above queue worker
  • add more logging to see exactly what is being processed
• changes in fastlane_core/lib/fastlane_core/queue_worker.rb: the old way of setting the NUMBER_OF_THREADS of a QueueWorker only allowed up to a maximum of 10 workers. Since we need more than 10 threads to achieve optimal concurrency in the scenario described above, the new behaviour is that setting any of DELIVER_NUMBER_OF_THREADS or FL_NUMBER_OF_THREADS env vars beyond 10 (like 24) will return that value instead of 10. Other than that, the default behaviour where not setting any of the env vars will still return 10 threads like before.
  • update: added FL_MAX_NUMBER_OF_THREADS environment variable to "unlock" the max number of concurrent threads to any value (as discussed below; defaults to 10)

Note: Google still has no api support to reorder screenshots (like Apple does). Otherwise we could have made the screenshot upload process in parallel (which is the most expensive operation) and reorder the uploaded files later.

Testing Steps

FL_MAX_NUMBER_OF_THREADS=24 DELIVER_NUMBER_OF_THREADS=24 bundle exec fastlane supply --package_name <com.package.name> --version_name "Test version" --version_code <existing version code> --track production --release_status draft --metadata_path .fastlane/GooglePlay/metadata --json_key /path/to/account.json --skip_upload_apk true --skip_upload_aab true --skip_upload_metadata false --skip_upload_changelogs false --skip_upload_images false --skip_upload_screenshots false --changes_not_sent_for_review true

[ccaruceru]

.reject behaves the same as the previously used next if

[rogerluan]

Hi @ccaruceru 👋 could you merge master into your branch (or rebase it)? There was a fix to our CI that is in master that should make your PR pass all checks 🙏

Thank you!

[ccaruceru]

@rogerluan thanks for pointing that out.

I updated to latest master and all checks passed 🟢

[AliSoftware]

Great improvement, thanks for the contribution! 🎉

That would go well, in terms of supply speedups, with my other (WIP) optimization for not uploading images and screenshots already present remotely 🙂

Left a couple of questions/suggestions, curious to ear your thoughts about them!

[AliSoftware]

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 to 10 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 be 10 if all tested ENV vars are nil. So that 1 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:

• If either DELIVER_NUMBER_OF_THREADS or FL_NUMBER_OF_THREADS is defined, use that value, but up to a maximum value of 10 (i.e. if those env vars contain a number ≤10, use it, but if the value is >10, cap it to 10 instead)
• If none of the env vars are defined, use a default value of [10.to_i, 10].min = 10

At least that's what the initial implementation was doing. While after your changes here this leads to:

• If either DELIVER_NUMBER_OF_THREADS or FL_NUMBER_OF_THREADS is defined, use that value as long as it's greater than 1 ([value.to_i, 1].max
• If none of the env vars are defined, use a default value of [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 return 0, so ['aaa'.to_i, 10].min would also return 0. 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 of 10 to avoid thread-bombing.

---

Maybe this below — i.e. finding the first positive (>0) value after converting all candidates to to_i, then clamping between 1 and 10 — would make more sense to cover all cases and to better match the behavior we want ultimately? wdyt?

Suggested change

	NUMBER_OF_THREADS = FastlaneCore::Helper.test? ? 1 : [(ENV["DELIVER_NUMBER_OF_THREADS"] || ENV.fetch("FL_NUMBER_OF_THREADS", 10)).to_i, 1].max
	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, 10)

[ccaruceru]

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 of 10? Improving on your suggestion, it would look like so:

Suggested change

	NUMBER_OF_THREADS = FastlaneCore::Helper.test? ? 1 : [(ENV["DELIVER_NUMBER_OF_THREADS"] || ENV.fetch("FL_NUMBER_OF_THREADS", 10)).to_i, 1].max
	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)

[AliSoftware]

Yeah sounds like a great balance.

[AliSoftware]

I wonder if we should exit (e.g. UI. abort_with_message!) in case of error on any language, instead of just printing a red message but still continuing with the rest.

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 upload_screenshots raised mid-upload for some reason) and to instead have the whole lane stop to give me a change to fix and retry.

Otherwise the overall call to upload_to_play_store action will print red messages to the logs, but will still ultimately succeed (because UI.error doesn't raise, unlike UI. abort_with_message!) while having only partial data in PSC. This could be even more problematic if you run that lane on CI (like we do in our setup at my company), because that means your CI step will show up green making you think that everything went well, while ultimately your end users will have some screenshots or images or release notes missing.

[ccaruceru]

From my experience I would rather let supply upload all available assets and fix the missing ones later (w or w/o another fastlane call). The reason is because of the team I'm working with (Marketing) is prone to many human errors that invalidates the data we send to the Google Play Console (like title too long, invalid screenshot resolution, etc) and they prefer manual fixes in the UI rather in an automated system 🤷🏻‍♂️

Also if I'm not mistaken, I think this is the default behaviour of deliver as well (continue all uploads and stop with error at the end) 🤔

---

that means your CI step will show up green making you think that everything went well, while ultimately your end users will have some screenshots or images or release notes missing.

Tested locally and the exit code is 1 when there is an upload error, so we should be covered in CI when an error is thrown. Do you mean that when setting changes_not_sent_for_review to false and the track is production the process will continue to submit the app to the store even if some uploads failed?

[AliSoftware]

Ah TIL that deliver will fail later if an error was encountered in one of the cases. As long as the final exit code is 1 if any error occurred during the upload then I'm ok with it.

[ccaruceru]

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.

[21:24:37]: fr-FR - Google Api Error: Unauthorized - Request is missing required authentication credential. Expected OAuth 2 access token, login cookie or other valid authentication credential. See https://developers.google.com/identity/sign-in/web/devconsole-project.

The errors I saw in the past tests which made me believe the process exited win non zero code were actually thrown later by client.commit_current_edit.

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 UI.abort_with_message to inform the user to check the (above) logs for any errors. How does that sound @AliSoftware?

[AliSoftware]

For instance, the following error was thrown

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 2.214.0 while it worked a couple of days ago.

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 UI.abort_with_message to inform the user to check the (above) logs for any errors. How does that sound @AliSoftware?

Just to be clear, you'd UI.abord_with_message! only after having called client.commit_current_edit though, which means that the action would exit with non-zero code… but the changes would still have been submitted to Google Play, right?

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:

• Make this current PR keep the same behavior on failure as what we currently have without parallel uploads. That is, if one upload fails, stop everything without trying to upload the rest
• Separately, make a PR to add ConfigItem.new(key: allow_submit_partial_changes) (happy if you have a better name idea for that one 😄 ) that would allow the behavior you're after (i.e. allowing the screenshots that passed upload to be submitted even if other screenshots failed). But that would be an opt-in option, so that everyone could choose the behavior they prefer in case of failures (you're probably turn it on while I'd personally prefer it off), and also so that we don't break backwards compatibility in the action's behavior.
• Separately again, I was thinking making a PR to add some ConfigItem.new(key: :max_retries) to the upload_to_play_store action too, to allow retrying each API calls a max number of times if one fails. Which should already reduce the risk of a whole batch of screenshot uploads failing mid-process, and would also help mitigating Google Api Error: Unauthorized - Request is missing required authentication credential when trying to do a Google Play release #21507

[ccaruceru]

Thanks for the heads-up about the google api error, I thought it was just me 😄

Just to be clear, you'd UI.abord_with_message! only after having called client.commit_current_edit though, which means that the action would exit with non-zero code… but the changes would still have been submitted to Google Play, right?

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.

[AliSoftware]

I think begin…end is unnecessary when they are encapsulating the whole body of a block? i.e. I think you can remove begin and end while still keeping your rescue here?

[ccaruceru]

good catch! will change

[AliSoftware]

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 inout parameter in some other languages.

Basically you initialize local variable release_notes = [] on line 110, then pass the value of that var when building your UploadJob instance that you enqueue. Then for each job you append a new value to job.release_notes… but is that the same reference as the initial release_notes variable you created on line 110, as opposed to be a passed-by-value copy?? 🤔

(Also, even if it was passed by reference, what happens if multiple UploadJobs are finishing potentially out of order and try to append to that array concurrently? 🤔 💭 )

---

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:

• Removing :release_notes field from your UploadJob struct
• Declaring release_notes = [] before you declare your upload_worker and its block (so that the variable is declared and can be captured by the block declaration)
• Inside your block, use release_notes << upload_changelog(…) to directly append to the outside, captured variable

This approach would be very similar to the example that is given in the official doc of Thread::new 🙃

[ccaruceru]

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 release_notes above upload_worker and let the scope see it, as you suggested.

---

what happens if multiple UploadJobs are finishing potentially out of order and try to append to that array concurrently?

I don't think the order of the results matters that much since in upload_changelogs there's nothing that would depend on it 👀

[ccaruceru]

@AliSoftware can you have a second look? 🔎

[AliSoftware]

New code looks great… except for the omission to reset release_notes = [] on each iteration when version_codes contains more than one version 🙃

I'd also love for you to add some new unit in supply_spec.rb especially for the case of multiple version_codes and how it handles release_notes. I'd suggest you to approach this using red-green-refactor approach, i.e.

• First write the new unit tests for the case of multiple version_codes ([123, 456]) and multiple changelog/123.txt and changelog/456.txt files in metadata
• Then run the test and confirm that it fails, especially that the changelogs used for the second version code might incorrectly be using the ones from the first version code instead because you didn't reset release_notes = []
• Then fix the implementation (re-add release_notes = [] in each iteration)
• Then confirm that your test finally goes green after the fix

---

Finally, wdyt about documenting this new ability in the self.details documentation of upload_to_play_store action, by adding something along the lines of "metadata can be uploaded in parallel by setting the environment variable FL_NUMBER_OF_THREADS to a value between 1 and 10"?

Especially since the default value for NUMBER_OF_THREADS if that env var is not set will be 1 and thus the parallelisation won't be enabled by default, making this nice new improvement invisible and hard to discover for most users otherwise.

[AliSoftware]

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.

[ccaruceru]

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

[AliSoftware]

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.

[ccaruceru]

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 🧪

[ccaruceru]

Finally, wdyt about documenting this new ability in the self.details documentation of upload_to_play_store action, by adding something along the lines of "metadata can be uploaded in parallel by setting the environment variable FL_NUMBER_OF_THREADS to a value between 1 and 10"?

Especially since the default value for NUMBER_OF_THREADS if that env var is not set will be 1 and thus the parallelisation won't be enabled by default, making this nice new improvement invisible and hard to discover for most users otherwise.

I can add a few words to the details section too 👍🏻

But the new code (as it is now) actually starts 10 threads in parallel if none of the environment variables are present, because we default to 10 at the ned of our "search list". Should we revert that to the original behaviour too?

[AliSoftware]

Especially since the default value for NUMBER_OF_THREADS if that env var is not set will be 1 and thus the parallelisation won't be enabled by default, making this nice new improvement invisible and hard to discover for most users otherwise.

But the new code (as it is now) actually starts 10 threads in parallel if none of the environment variables are present, because we default to 10 at the ned of our "search list". Should we revert that to the original behaviour too?

Oh so sorry my bad, I misremembered 😅 You're right that the number of threads defaults to 10 (both with the old code and the new code), not to 1 as I wrote above. We should be good then 👍

[ccaruceru]

@AliSoftware added tests and updated the docs. Let me know what you think of them 🔍

ps: while adding the test I surfaced an even bigger error where we can't reuse same QueueWorker once it was started (what were the odds?) 😄

[ccaruceru]

@AliSoftware "kind reminder" 🙂

[AliSoftware]

Hey @ccaruceru , sorry I was on vacation for 2 weeks, just got back this evening, and I thus have a lot to catch up, but I didn't forget you 😄 I'll try to review the changes by end of this week 🤞

[AliSoftware]

Sorry for the delay in the review! Left a couple of feedback, nothing major though.

[AliSoftware]

I'm not a fan or reusing the same variable name for two different things. Maybe rename the original variable release_notes_queue = Queue.new and then use release_notes = Array.new(release_notes_queue.size) { release_notes_queue.pop }? (And for consistency, maybe name the attribute in UploadJob as :release_notes_queue instead of :release_notes too?)

[ccaruceru]

makes sense. will change to release_notes_queue for clarity 👌🏻

[AliSoftware]

Shouldn't that be reduce(:&&) to join all tests by a logical AND operator, as opposed to use a bitwise-and & operator?
Though, in practice, if that's really the intent, you'd be better served by using all_satisfy? method instead.

Also, I wonder if using the built-in match_array RSpec matcher (which compares content of arrays without regard for the potential different order of the elements being compared) wouldn't be enough to compare those two arrays, instead of writing your custom matcher? (Though I'm not 100% if AndroidPublisher::LocalizedText has a proper implementation of == that compares each property for equality instead of using ObjectIDs…)

[ccaruceru]

I used reduce(:&) because & is the operator we have for TrueClass and FalseClass (there's no &&). I would prefer using the expression in that form rather than expand it to reduce { |a, b| a && b }. What do you think?

---

As for the matcher, I actually had to write a new one because we lack the support for == on AndroidPublisher::LocalizedText 😕 , so even match_array wouldn't help in that case. (it does a plain comparison of object ids as you implied last).

[AliSoftware]

I used reduce(:&) because & is the operator we have for TrueClass and FalseClass (there's no &&). I would prefer using the expression in that form rather than expand it to reduce { |a, b| a && b }. What do you think?

Then in that case I'd rather have you use all_satisfy?, which is more idiomatic Ruby for that kind of thing

As for the matcher, I actually had to write a new one because we lack the support for == on AndroidPublisher::LocalizedText 😕 , so even match_array wouldn't help in that case. (it does a plain comparison of object ids as you implied last).

What about declaring that == ourselves then? Since Ruby allows to re-open classes to add definitions 🙃

class AndroidPublisher::LocalizedText
  def ==(other)
    self.language == other.language && self.text == other.text
  end
end

[AliSoftware]

Besides, I think your implementation of that matcher has a hidden bug, because in a.zip(b), if a has less elements than b then the result of the zip will only have as many elements as a, ignoring the trailing elements of b:

$ pry
[1] pry(main)> a = [1,2]
=> [1, 2]
[2] pry(main)> b = [3,4,5]
=> [3, 4, 5]
[3] pry(main)> a.zip(b)
=> [[1, 3], [2, 4]]

Which means that if actual_arr has less elements than expected_arr, your matcher will pass instead of detecting a mismatch. Which is a bug that I don't thing match_array is subject to 🙃

[ccaruceru]

The .zip "bug" is actually a good point. The code should be changed.

---

I forgot about that we can open classes anywhere we want, that was a good call 😄 . I added the == override as you suggested and that simplified the test code a lot. Now we can get away with just a match_all call. That should solve the zip problem too.

[AliSoftware]

Not tested, but I think you could use an object double like double('tracks', releases: [release]) instead of using OpenStruct.new? Which would me more idiomatic of rspec tests practices I think?

[AliSoftware]

Probably worth using a double('release', version_codes: version_codes) instead here? (If I remember the syntax for creating rspec doubles correctly)

[AliSoftware]

Shouldn't this one be an expect instead of an allow? 🤔

[ccaruceru]

good call. will change.

[AliSoftware]

Suggested change

	expected_notes = []
	languages.each do |lang|
	expected_notes << AndroidPublisher::LocalizedText.new(
	language: lang,
	text: "#{lang} changelog #{with_explicit_changelogs ? version_code : -1}"
	)
	end
	expected_notes = languages.map do |lang|
	AndroidPublisher::LocalizedText.new(
	language: lang,
	text: "#{lang} changelog #{with_explicit_changelogs ? version_code : -1}"
	)
	end

[ccaruceru]

haha, my procedural instinct blinded me from idiomatic ruby 😄

[AliSoftware]

Not sure if it's just me being used to that pattern or an actual ruby/rspec codestyle recommendation, but I'm more used to see this kind of parametric tests be implemented as parameters passed to shared_examples, rather than shared example called with a do…end block redefining let constants for each case.

See https://railsware.com/blog/using-configurable-shared-examples-in-rspec/ for examples of such pattern with parametrized shared_examples.

[ccaruceru]

thanks for pointing that out. I'll update the test.

[ccaruceru]

Sorry for the delay in the review! Left a couple of feedback, nothing major though.

@AliSoftware No worries!

The code is updated. Please have another look when you got some time 🙂

[AliSoftware]

Apart from the note on test code—which doesn't affect the current test given how the shared example is called, so up to you if you want to fix it or not, as it's not really blocking—this looks good now!

Unfortunately we can't really have the unit tests check if the parallelization itself works (i.e. if there isn't any risk of race conditions or incorrect ordering or whatnot) because by design the tests will always run with a single thread (NUMBER_OF_THREADS = FastlaneCore::Helper.test? ? 1 : …), but the code looks ok to me in that regard, thanks to the Queue.new and the upload_worker.start being blocking 👍

[ccaruceru]

Thanks for all the reviews! Everything should be good for merging now 👍🏻