Skip to content

Adds the option to auto-retry for downloading metadata from GlotPress #474

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.

Sign up for GitHub

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

Merged
oguzkocer merged 4 commits into from
May 19, 2023
Merged

Adds the option to auto-retry for downloading metadata from GlotPress #474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8fa9163
Adds the option to auto-retry for downloading metadata from GlotPress
oguzkocer May 10, 2023
17ca0b7
Limit number of auto_retry attempts in metadata_download_helper
oguzkocer May 18, 2023
5c19fca
Adds CHANGELOG entry for new auto_retry option in gp_downloadmetadata…
oguzkocer May 18, 2023
dc56ef0
Change auto_retry gp_downloadmetadata_action parameter to true
oguzkocer May 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ _None_

### New Features

_None_
- Adds auto_retry option to `gp_downloadmetadata_action`. [#474]

### Bug Fixes

Expand Down
9 changes: 8 additions & 1 deletion lib/fastlane/plugin/wpmreleasetoolkit/actions/common/gp_downloadmetadata_action.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,13 @@ def self.run(params)
UI.message "Locales: #{params[:locales].inspect}"
UI.message "Source locale: #{params[:source_locale].nil? ? '-' : params[:source_locale]}"
UI.message "Path: #{params[:download_path]}"
UI.message "Auto-retry: #{params[:auto_retry]}"

# Check download path
FileUtils.mkdir_p(params[:download_path])

# Download
downloader = Fastlane::Helper::MetadataDownloader.new(params[:download_path], params[:target_files])
downloader = Fastlane::Helper::MetadataDownloader.new(params[:download_path], params[:target_files], params[:auto_retry])

params[:locales].each do |loc|
if loc.is_a?(Array)
Expand Down Expand Up @@ -68,6 +69,12 @@ def self.available_options
env_name: 'FL_DOWNLOAD_METADATA_DOWNLOAD_PATH',
description: 'The path of the target files',
type: String),
FastlaneCore::ConfigItem.new(key: :auto_retry,
env_name: 'FL_DOWNLOAD_METADATA_AUTO_RETRY',
description: 'Whether to auto retry downloads after Too Many Requests error',
type: FastlaneCore::Boolean,
optional: true,
default_value: true),
]
end

Expand Down
16 changes: 13 additions & 3 deletions lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,17 @@
module Fastlane
module Helper
class MetadataDownloader
AUTO_RETRY_SLEEP_TIME = 20
MAX_AUTO_RETRY_ATTEMPTS = 30
Copy link
Contributor

@AliSoftware AliSoftware May 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if it wouldn't make more sense to have the auto_retry ConfigItem be an Integer that would allow to set that max-retry-attempts value?

That way we could use a value like 30 (!) for CI because we really need to be sure to end up in success, while we could use a value like 3 for repos still doing manual release, because it'd be nice not to have to UI.confirm the retry manually every time this happens just on the first or second try, but if it fails consistently more than 3 times we might want to wait longer when confirming manually.

Copy link
Contributor Author

@oguzkocer oguzkocer May 18, 2023

Choose a reason for hiding this comment

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

That's a cool idea. I could make that change if there is practical use for it for others or if you feel strongly that it is a better approach.

My understanding is that, this issue is mostly specific to WPAndroid & WPiOS and as you mentioned it's a temporary one. That's why I did this in the most straightforward way.

Copy link
Contributor

@mokagio mokagio May 19, 2023

Choose a reason for hiding this comment

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

FWIW, I don't feel strongly about it.


attr_reader :target_folder, :target_files

def initialize(target_folder, target_files)
def initialize(target_folder, target_files, auto_retry)
@target_folder = target_folder
@target_files = target_files
@auto_retry = auto_retry
@alternates = {}
@auto_retry_attempt_counter = 0
Copy link
Contributor

@AliSoftware AliSoftware May 19, 2023

Choose a reason for hiding this comment

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

It just happened to me that the downloader instance is reused at call site for each locale to be downloaded.

This means that if you only set the @auto_retry_attempt_counter in initialize, the counter will be shared across all usages of the downloader, and thus across all locales for which you call download(…) on.

Was this intentional to consider that MAX_AUTO_RETRY_ATTEMPTS be seen as "max 30 retries in total" as opposed to "max 30 retries per locale"? If not, I think we need to make sure @auto_retry_attempt_counter is reset to 0 every time the caller calls download(…) on a new locale.

For example, maybe you could move the current 3 LoC implementation of download to a private try_download method, then change download(…) implementation to now do @auto_retry_attempt_counter = 0 + call try_download(…)?

Copy link
Contributor Author

@oguzkocer oguzkocer May 19, 2023

Choose a reason for hiding this comment

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

This was intentional. The only goal I had with this attempt counter is so that we wouldn't get into an infinite loop. I don't think we'll ever hit this limit unless there is an issue with the server or CI in which case, I thought it'd be best to stop and let the release manager handle it either manually or by retrying once the issue is resolved.

Copy link
Contributor

@AliSoftware AliSoftware May 19, 2023

Choose a reason for hiding this comment

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

SGTM 👍

end

# Downloads data from GlotPress, in JSON format
Expand Down Expand Up @@ -112,8 +117,13 @@ def handle_glotpress_download(response:, locale:, is_source:)
UI.message("Received 301 for `#{locale}`. Following redirect...")
download(locale, response.header['location'], is_source)
when '429'
# We got rate-limited, offer to try again
if UI.confirm("Retry downloading `#{locale}` after receiving 429 from the API?")
# We got rate-limited, auto_retry or offer to try again with a prompt
if @auto_retry && @auto_retry_attempt_counter <= MAX_AUTO_RETRY_ATTEMPTS
UI.message("Received 429 for `#{locale}`. Auto retrying in #{AUTO_RETRY_SLEEP_TIME} seconds...")
sleep(AUTO_RETRY_SLEEP_TIME)
Copy link
Contributor

@AliSoftware AliSoftware May 18, 2023

Choose a reason for hiding this comment

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

I wonder if we wouldn't benefit from a linear or exponential wait time between attempts instead of a fixed time?

e.g. sleep(10 + @auto_retry_attempt_counter*5) or something?

Copy link
Contributor Author

@oguzkocer oguzkocer May 18, 2023

Choose a reason for hiding this comment

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

I've considered this one, but in my experience, it's really not necessary. I think the server just checks the number of requests in a certain amount of time, but I could be wrong.

For what it's worth, I actually wanted to go with something shorter, but since it's in CI, it doesn't really matter if it takes 1 or 2 minutes longer. Funnily enough, I tried to create the 429 error today for quite some time and I couldn't get it even once.

Happy to apply the suggestion if you feel there is enough benefit.

@auto_retry_attempt_counter += 1
download(locale, response.uri, is_source)
elsif UI.confirm("Retry downloading `#{locale}` after receiving 429 from the API?")
download(locale, response.uri, is_source)
else
UI.error("Abandoning `#{locale}` download as requested.")
Expand Down