Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add support for user-specified timeout and retry options in the client. #67

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

csilvers
Copy link

Sailthru customer support suggests retrying on transient failures when
talking to sailthru. This adds support to the client library to make
it easy to do so. We retry on connection timeouts and all 5xx
errors. We do not retry on read timeouts since it's not safe for
POSTs, which are not idempotent (it would be ok for GETs, but that's a
TODO for another day).

Test Plan:
I couldn't figure out how to write an automated test for this since
requests hides all the retry logic from us. (I didn't want to mock
the internals of the requests module since I felt that was brittle.)

But I did create a local server with a route that always returns a
500, and supports only GET and not POST (so it gives a 500 for GET and
a 403 for POST). I then did

>>> import sailthru
>>> c = sailthru.SailthruClient('key', 'secret', api_url='http://localhost:8080', retries=3, timeout=5)
>>> print c.api_get('crash', '').is_ok()
None

and looked in the server-logs and saw the /crash url was hit 4 times.

I then did

>>> print c.api_post('crash', '').is_ok()
None

and looked in the server-logs and saw the endpoint was only hit 1 time
this time.

I then added a route to my server that waits before it responds, and
did

>>> print c.api_post('sleep', '').is_ok()
None

and timed that it took 5 seconds for this to run.

csilvers and others added 3 commits October 30, 2017 15:27
Sailthru customer support suggests retrying on transient failures when
talking to sailthru.  This adds support to the client library to make
it easy to do so.  We retry on connection timeouts and all 5xx
errors.  We do not retry on read timeouts since it's not safe for
POSTs, which are not idempotent (it would be ok for GETs, but that's a
TODO for another day).

Test Plan:
I couldn't figure out how to write an automated test for this since
requests hides all the retry logic from us.  (I didn't want to mock
the internals of the requests module since I felt that was brittle.)

But I did create a local server with a route that always returns a
500, and supports only GET and not POST (so it gives a 500 for GET and
a 403 for POST).  I then did
```
>>> import sailthru
>>> c = sailthru.SailthruClient('key', 'secret', api_url='http://localhost:8080', retries=3, timeout=5)
>>> print c.api_get('crash', '').is_ok()
None
```
and looked in the server-logs and saw the `/crash` url was hit 4 times.

I then did
```
>>> print c.api_post('crash', '').is_ok()
None
```
and looked in the server-logs and saw the endpoint was only hit 1 time
this time.

I then added a route to my server that waits before it responds, and
did
```
>>> print c.api_post('sleep', '').is_ok()
None
```
and timed that it took 5 seconds for this to run.
Add support for user-specified timeout and retry options in the client.
The changelog at https://pypi.python.org/pypi/requests even seems to
indicate this is kosher.  The old code worked on newer versions of
requests; this new code should work on older versions as well.

Auditors: ragini

Test plan:
Ran `python -c 'import requests; requests.packages.urllib3'`
on a requests module version 2.13.0.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant