-
Notifications
You must be signed in to change notification settings - Fork 244
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
[services] reliably retry all requests #13029
Conversation
`request_retry_transient_errors` is a charlatan. It does not retry errors that occur in reading the response body. I eliminated it in favor of the tried and true `retry_transient_errors` and some new helper methods that initiate the request *and* read the response.
session, | ||
'GET', | ||
resp_json = await retry_transient_errors( | ||
session.get_return_json, |
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 was also thinking about calling this get_read_json
. Thoughts? I'm trying to convey a distinction between this method and the one which returns a body reading handle.
An example of an error that wasn't retried properly:
|
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 don't really take issue with these names. What I care most about here is that those methods properly use the response as a context manager, which I feel is easy to miss and a footgun, so I'm in favor of more convenience functions like these that return the end data type (like a Dict) instead of a httpx.ClientResponse
@@ -101,10 +101,11 @@ async def async_copy_paste_login(copy_paste_token: str, namespace: Optional[str] | |||
raise_for_status=True, | |||
timeout=aiohttp.ClientTimeout(total=5), | |||
headers=headers) as session: | |||
async with await request_retry_transient_errors( |
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 should be an httpx.ClientSession
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 catch. I removed another case of aiohttp.ClientSession from job.py as well. It now only appears in httpx.py.
request_retry_transient_errors
is a charlatan. It does not retry errors that occur in reading the response body. I eliminated it in favor of the tried and trueretry_transient_errors
and some new helper methods that initiate the request and read the response.