Add clearer retry logic with logging [RHELDST-9679] #31
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #31 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 123 150 +27
=========================================
+ Hits 123 150 +27 ☔ View full report in Codecov by Sentry. |
amcmahon-rh
force-pushed
the
clearRetry
branch
3 times, most recently
from
f282166 to
78357ff
Compare
|
@rbikar @rohanpm @MichalHaluza could this PR be reviewed please. |
tests/test_purge.py
Outdated
|
|
||
| url = 'https://fastpurge.example.com/ccu/v3/delete/cpcode/production' | ||
| # Decrease backoff, otherwise the test will run for 5 minutes | ||
| os.environ["FAST_PURGE_RETRY_BACKOFF"] = "0.001" |
There was a problem hiding this comment.
This change will not be reverted at the end of the test. This means for other tests run during the same process, it'll be effectively random whether FAST_PURGE_RETRY_BACKOFF=0.001 or not, based on whether each test function ran before or after this one.
Could you please use monkeypatch.setenv instead so it's automatically reverted?
fastpurge/_client.py
Outdated
| self._logger.error("An invalid status code %s was received " | ||
| "when trying to %s to %s: %s" | ||
| % (response.status, method, url, response.reason)) |
There was a problem hiding this comment.
Minor: the preferred logging style is to let the logger itself do the interpolation, e.g.
| self._logger.error("An invalid status code %s was received " | |
| "when trying to %s to %s: %s" | |
| % (response.status, method, url, response.reason)) | |
| self._logger.error("An invalid status code %s was received " | |
| "when trying to %s to %s: %s", | |
| response.status, method, url, response.reason) |
One reason is that any exceptions while converting the arguments to strings will themselves then be gracefully logged instead of just crashing.
amcmahon-rh
force-pushed
the
clearRetry
branch
2 times, most recently
from
c0fd128 to
917ea60
Compare
Poor retry logic in this library has made some pub tasks fail. The logs are also unclear as to whether retries are occurring or not. This change extends the urllib3 retry logic to include logging to resolve both issues. It also introduces new envvars to make the retry periods configurable. The default backoff settings retry the request 10 times over a 5 minute period.
amcmahon-rh
force-pushed
the
clearRetry
branch
from
917ea60 to
d2b47e8
Compare
Poor retry logic in this library has made some pub tasks fail. The logs are also unclear as to whether retries are occurring or not. This change extends the urllib3 retry logic to include logging to resolve both issues. It also introduces new envvars to make the retry periods configurable. The default backoff settings retry the request 10 times over a 5 minute period.
I have also included the
responseslibrary for testing the retry logic. The mock_requests library appeared to not be able to support it, while responses had very clear support.