Remove redundant future retry [RHELDST-9679] #32
Conversation
Originally, the fast purge client would retry failed futures. Now with the HTTP retries it doesn't make sense to have both. The interaction between the two causes failing purges to run much longer than the desired 5 minutes. This change removes the Future retry in favour of the urllib3 retry.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #32 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 150 150
=========================================
Hits 150 150 ☔ View full report in Codecov by Sentry. |
|
@release-engineering/distribution ptal |
|
Surprised this repo doesn't have codeowners |
There was a problem hiding this comment.
I'm not totally confident that this isn't somehow narrowing the set of errors which will be retried, because this was designed to retry all types of non-exiting exceptions while the retry policy on the HTTPAdapter is much more fine-grained.
Basically, this is changing things from "denylist" (retry every type of exception, unless explicitly not retried) to "allowlist" (retry only some specific types of exceptions). So please watch out for any new errors which might make it through to pushes due to this.
|
I did realise making that this change would narrow the scope of retries. However, given the retries were limited to a function that mainly just making a HTTP request and storing the result, I concluded the most likely errors would be HTTP related. |
Originally, the fast purge client would retry failed futures. Now with the HTTP retries it doesn't make sense to have both. The interaction between the two causes failing purges to run much longer than the desired 5 minutes.
This change removes the Future retry in favour of the urllib3 retry.