-
Notifications
You must be signed in to change notification settings - Fork 198
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
Replace retry with backoff, add backoff to all Freesound requests #4663
Conversation
Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR: @krysal Excluding weekend1 days, this PR was ready for review 2 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)2. @AetherUnbound, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
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.
LGTM, except a nit about the position of a comment.
# Freesound often tends to be flaky when making requests during ingestion. | ||
# For the flaky HTTP errors above, we wait some time before trying the request | ||
# again in the hopes that the upstream issues resolve in that time: | ||
# * 400 - Although framed as bad requests, these appear to succeed later | ||
# * 503 - Service unavailable, so we should just try to wait |
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.
This comment would've been more helpful before L39.
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.
Nice to prune and keep updated dependencies! LGTM.
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.
Great idea! It would be wonderful if this eases some of the flakiness we've experienced🤞
Fixes
Fixes #4594 by @stacimc
Description
Warning
This should not be merged until a deployment can be made immediately after, since it introduces new dependencies.
This PR makes two major changes:
retry
withbackoff
The latter change is relatively straightforward: we encounter tons of exceptions during the Freesound ingestion process that seem to be transient and ultimately get resolved when checked later. However this causes significant disruptions in the execution of the DAG itself, since it often requires that maintainers restart the DAG with
SKIP_INGESTION_ERRORS
enabled for it. This change allows requests to be retried with an exponential backoff up to a max time (2 minutes for now) if they match the HTTP codes 400 or 503. On all other codes, the exception is raised and retries (outside of the normal ones for theDelayedRequester
) are not attempted.The former change is motivated by the above.
retry
is a Python library that hasn't been updated since 2016, and only supports fairly basic retry conditions.backoff
on the other hand has been updated as recently as 2022 and supports much more complexity in retry conditions. With the above case, we only wanted to retry onHTTPErrors
with a specific return code; that was not possible withretry
but is now possible withbackoff
. This should give us some additional flexibility moving forward too!Testing Instructions
I added new tests specifically for the conditions mentioned above. You can also run the
freesound_workflow
DAG locally to ensure everything behaves normally (and maybe encounter one of the above cases if you're lucky!).Checklist
Update index.md
).main
) or a parent feature branch.ov just catalog/generate-docs
for catalogPRs) or the media properties generator (
ov just catalog/generate-docs media-props
for the catalog or
ov just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin