Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

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

Merged
merged 1 commit into from
Oct 30, 2017

Conversation

csilvers
Copy link
Member

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.

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 csilvers requested a review from guptaragini October 30, 2017 22:32
Copy link

@guptaragini guptaragini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me! Thanks.
If this gets in, we'll have make changes to use this in our sailthru_to_bigquery script.

@csilvers csilvers merged commit 234889d into upstream-master Oct 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants