-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add an option to retry when GlotPress download fails on "429 Too Many Requests" errors #402
Conversation
91a75de
to
840740b
Compare
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.
These changes look good to me! I read through it and it all makes sense 👍
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'd normally make a snarky remark to suggest adding tests for the retry logic. But I don't think it's needed in this instance because:
- Requires non-trivial test setup to cover a niche feature
- We already have a new implementation of the whole workflow in the works, so any extra time spent here has low ROI
The automation hit an HTTP error, likely the known HTTP 429 GlotPress error (e.g. see wordpress-mobile/release-toolkit#402), and those files were deleted. The tooling is right in deleting the files when it can't find a translation for them. But in this instance the translations were there, we just had an issue in downloading them. Notice that does not apply to the `release_notes.txt` file. There is no translation available at this point in time.
So I can get the latest non-breaking version of the release-toolkit for when I will be doing the final release later today Especially as that latest minor version includes wordpress-mobile/release-toolkit#402 which will help me mitigate any quota limit error I'll very likely get while downloading latest translations from GlotPress
These were deleted during the last `gp_downloadmetadata` likely because of a 429 error from GlotPress. Various GlotPress actions were updated to retry when receiving a 429 in wordpress-mobile/release-toolkit#402, but `gp_downloadmetadata` was not one of them.
Why?
We still keep having
429 - Too Many Requests
errors from time to time when running the actions which download the app translations from GlotPress.For example, on average I encounter those on at least 1-3 locales every time I'm doing a new beta or final release, forcing me to interrupt the lane, discard the unstaged changes in git, restart the lane from scratch again, and pray it will pass on those subsequent tries, otherwise I'll have to do all that again. And since the more you retry, the closer you are to the quota limit… things tend to actually get worse on subsequent retries.
This has become quite painful while doing betas and releases, especially for WordPress (which has more than 40 locales to download, so the chances of hitting the
429
error on at least one of them are higher)How?
This simply asks the user if they want to retry a the download of a locale if one fails.
UI.confirm
prompt before retrying, which allows to leave some time before the first and next retry of the request, giving us even more chances for the quota limit to cool down before the next try.Related work
In parallel to that, other work is being done to help getting rid of this
429
error and quota limit. Especially, we've asked for thegp-import-export
plugin to be installed on both of our GlotPress instances so we could do bulk-downloads of all the locales at once with a single request, and we plan to implement the code to in therelease-toolkit
to be able to use it hopefully soon (we already have a Draft here, but it's still very early stage so far).Supporting bulk-download in the
release-toolkit
will require a bit more involved work though, for which I don't have the bandwidth for just yet. So this workaround of still using one-at-a-time locales download but allowing retry would hopefully help mitigate the issue in the meantime.