-
Notifications
You must be signed in to change notification settings - Fork 2k
Retries with 1, 2, 4, 8, 16, 32s backoff #6322
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
base: master
Are you sure you want to change the base?
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
At least it allows me to more or less use MusicBrainz
snejus
left a comment
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.
See my comment
| def make_response(): | ||
| if isinstance(last_response, HTTPResponse): | ||
| body = last_response.data | ||
| return HTTPResponse( | ||
| body=io.BytesIO(body), | ||
| status=last_response.status, | ||
| preload_content=False, | ||
| headers=last_response.headers, | ||
| ) | ||
| return last_response | ||
|
|
||
| def responses(): | ||
| yield NewConnectionError(None, "Connection failed") | ||
| yield URLError("bad") | ||
| while True: | ||
| yield make_response() | ||
|
|
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 had to stare at this for a couple of minutes until I managed to work out how does this work 😅 I think this needs to be simplified...
However, there is a more significant issue - test durations:
$ poe test --durations=10 test/plugins/utils/test_request_handler.py
...
=============================================================================== slowest 10 durations ===============================================================================
31.04s call test/plugins/utils/test_request_handler.py::TestRequestHandlerRetry::test_retry_exhaustion[conn_error]
31.03s call test/plugins/utils/test_request_handler.py::TestRequestHandlerRetry::test_retry_exhaustion[server_error]
1.01s call test/plugins/utils/test_request_handler.py::TestRequestHandlerRetry::test_retry_on_connection_error[success]
0.13s setup test/plugins/utils/test_request_handler.py::TestRequestHandlerRetry::test_retry_on_connection_error[success]Tests that take 30s are now viable here, unfortunately. I think we want to re-think how do we test this configuration.
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.
Yes, it is a price tag. Frankly, I not sure that we should test it. And yes, figure out how to test it was... quite a challenge!
But I have no idea how to test timeout based things without time penalty.
|
@snejus what do you think about this tests? |
Description
At least it allows me to more or less use MusicBrainz, and makes issues from #6312 almost not noticible
To Do
docs/to describe it.)docs/changelog.rstto the bottom of one of the lists near the top of the document.)