Retry on timeout and 500+ errors #151
Conversation
This way clients can ask to wait for more than 0.5 second before retrying the request.
|
What kind of testing have you subjected this to? |
|
Not much indeed, still planning to but indeed forgot to mention it, sorry :-( |
|
Cool. the code looks fine from a read-through, but let's test the tar out of it before we merge it. If you want to try, there is a bodhi api client test tool in the bodhi repo you could mess with. |
|
I was thinking to test pkgdb-cli with these changes as well (and eventually to port pkgdb-cli to it) |
| @@ -182,7 +183,8 @@ def _authed_get(self, url, params=None, data=None, **kwargs): | |||
| response = self._session.get(url, params=params, data=data, **kwargs) | |||
| return response | |||
|
|
|||
| def send_request(self, method, auth=False, verb='POST', **kwargs): | |||
| def send_request(self, method, auth=False, verb='POST', retries=None, | |||
| retry_sleep=0.5, **kwargs): | |||
There was a problem hiding this comment.
Would it make sense to put retry_sleep as an attribute of OpenIdBaseClient, similarly to retries? One could then easily create Bodhi2Client and set Bodhi2Client.retry_sleep in order to modify the sleep interval.
There was a problem hiding this comment.
Yes. That makes a lot of sense to me.
There was a problem hiding this comment.
OK, so now you should kick it out of the send_request() parameters and use self.retry_sleep inside it instead.
There was a problem hiding this comment.
On Wed, Sep 30, 2015 at 08:40:49AM -0700, Kamil Páral wrote:
In fedora/client/openidbaseclient.py:
@@ -182,7 +183,8 @@ def _authed_get(self, url, params=None, data=None, *_kwargs):
response = self._session.get(url, params=params, data=data, *_kwargs)
return response
- def send_request(self, method, auth=False, verb='POST', **kwargs):
- def send_request(self, method, auth=False, verb='POST', retries=None,
retry_sleep=0.5, **kwargs):OK, so now you should kick it out of the send_request() parameters and use
self.retry_sleep inside it instead.
Retries is present in both place on the old client, why not keep it here as
well?
There was a problem hiding this comment.
Maybe I'm mistaken, but how can I currently configure retry_sleep? If I do bodhi = Bodhi2Client(); bodhi.retry_sleep = 2, it seems it won't have any effect in send_request() (because you don't utilize the class attribute).
Possible solutions that I see:
- Remove
retry_sleepfromsend_request()parameters. Useself.retry_sleepinside the method body. - Use
send_request(retry_sleep=None)as a default value, and then haveretry_sleep = retry_sleep if retry_sleep is not None or self.retry_sleep(or use multiple lines if you don't like ternary operator) inside the method body.
There was a problem hiding this comment.
Yeah, I was realizing the same after I replied, I'll adjust using 2)
| if timeout is None: | ||
| timeout = self.timeout | ||
|
|
||
| retry_sleep = retry_sleep or self.retry_sleep |
There was a problem hiding this comment.
There is one slight problem with this, send_request(retry_sleep=0) won't work correctly. You have to compare with None directly.
| # 'utf-8'. We can make that assumption because we're only | ||
| # interfacing with servers that we run (and we know that they | ||
| # all return responses encoded 'utf-8'). | ||
| response.encoding = 'utf-8' |
There was a problem hiding this comment.
response is undefined here.
|
Hey, this doesn't work at all. I went to test it but there are all kinds of undefined variables that get used in there. It was a copy/paste from somewhere else? Let's start over. I'm going to close it to get it out of the review queue. |
This pull-request allows the client to ask for a request to be retried if that request failed due to a
time-out or because it returned an error code >= 500.
In addition, it allows the clients to specify how long they want to wait in between two attempts of
sending the request (defaults to 0.5 second).
Fixes #144