Add option to automatically retry requests.

Add option to automatically retry requests.

[jterrace, 2013-06-03 16:14:31]

LGTM

https://codereview.appspot.com/9920043/diff/2001/apiclient/http.py
File apiclient/http.py (right):

https://codereview.appspot.com/9920043/diff/2001/apiclient/http.py#newcode692
apiclient/http.py:692: self._sleep_fun(0.5 + self._rand_fun() * 2**retry_num)
How about a logging.warning or logging.info here?

[yovadia, 2013-06-03 21:36:32]

Thanks. I added a warning log message and fixed the retry logic not to sleep
after the final retry.

https://codereview.appspot.com/9920043/diff/2001/apiclient/http.py
File apiclient/http.py (right):

https://codereview.appspot.com/9920043/diff/2001/apiclient/http.py#newcode692
apiclient/http.py:692: self._sleep_fun(0.5 + self._rand_fun() * 2**retry_num)
On 2013/06/03 16:14:31, jterrace wrote:
> How about a logging.warning or logging.info here?

Done.

[yovadia, 2013-06-05 14:22:22]

Ping?

Also, I've added a new test and made some minor naming changes.

[jcgregorio_google, 2013-06-07 13:57:11]

https://codereview.appspot.com/9920043/diff/14001/apiclient/http.py
File apiclient/http.py (right):

https://codereview.appspot.com/9920043/diff/14001/apiclient/http.py#newcode601
apiclient/http.py:601: rand_stub=random.random,
See below, these should be dropped.

https://codereview.appspot.com/9920043/diff/14001/apiclient/http.py#newcode643
apiclient/http.py:643: self._rand_stub = rand_stub
Comment for each member.

Actually, if these are only used for testing, then just do this in the
constructor:

 self._rand = random.random 
 self._sleep = time.sleep

and then swap out the properties when testing.

https://codereview.appspot.com/9920043/diff/14001/apiclient/http.py#newcode655
apiclient/http.py:655: represents the last request.
If I set this to 0 then retries are disabled? If so, then please add that to the
description.

https://codereview.appspot.com/9920043/diff/14001/apiclient/http.py#newcode670
apiclient/http.py:670: _, body = self.next_chunk(http=http)
Retries should also be added to next_chunk.

[yovadia, 2013-06-10 22:34:21]

Thanks; PTAL.

https://codereview.appspot.com/9920043/diff/14001/apiclient/http.py
File apiclient/http.py (right):

https://codereview.appspot.com/9920043/diff/14001/apiclient/http.py#newcode601
apiclient/http.py:601: rand_stub=random.random,
On 2013/06/07 13:57:12, jcgregorio_google wrote:
> See below, these should be dropped.

Done.

https://codereview.appspot.com/9920043/diff/14001/apiclient/http.py#newcode643
apiclient/http.py:643: self._rand_stub = rand_stub
On 2013/06/07 13:57:12, jcgregorio_google wrote:
> Comment for each member.
> 
> Actually, if these are only used for testing, then just do this in the
> constructor:
> 
>  self._rand = random.random 
>  self._sleep = time.sleep
> 
> and then swap out the properties when testing.

Thanks; that does look better.

https://codereview.appspot.com/9920043/diff/14001/apiclient/http.py#newcode655
apiclient/http.py:655: represents the last request.
On 2013/06/07 13:57:12, jcgregorio_google wrote:
> If I set this to 0 then retries are disabled? If so, then please add that to
the
> description.

Done.

https://codereview.appspot.com/9920043/diff/14001/apiclient/http.py#newcode670
apiclient/http.py:670: _, body = self.next_chunk(http=http)
On 2013/06/07 13:57:12, jcgregorio_google wrote:
> Retries should also be added to next_chunk.

Done for resumable requests and downloads.

[jcgregorio_google, 2013-06-13 19:26:25]

https://codereview.appspot.com/9920043/diff/25001/apiclient/http.py
File apiclient/http.py (right):

https://codereview.appspot.com/9920043/diff/25001/apiclient/http.py#newcode672
apiclient/http.py:672: binary exponential backoff. If all retries fail, the
raised
Do we need to specify 'binary', or is just 'exponential' detailed enough?

https://codereview.appspot.com/9920043/diff/25001/tests/test_http.py
File tests/test_http.py (right):

https://codereview.appspot.com/9920043/diff/25001/tests/test_http.py#newcode328
tests/test_http.py:328: for retry_num in xrange(num_retries):
oh, that's tricky, can you add a comment on what you're doing here. Or switch it
to an explicit list of expected_sleeptimes =[1, 2, ...]

https://codereview.appspot.com/9920043/diff/25001/tests/test_http.py#newcode659
tests/test_http.py:659: request.execute()
add:

   self.fail("Should have raised exception.")

[yovadia, 2013-06-13 20:15:22]

Thanks; PTAL.

https://codereview.appspot.com/9920043/diff/25001/apiclient/http.py
File apiclient/http.py (right):

https://codereview.appspot.com/9920043/diff/25001/apiclient/http.py#newcode672
apiclient/http.py:672: binary exponential backoff. If all retries fail, the
raised
On 2013/06/13 19:26:25, jcgregorio_google wrote:
> Do we need to specify 'binary', or is just 'exponential' detailed enough?

Done.

https://codereview.appspot.com/9920043/diff/25001/tests/test_http.py
File tests/test_http.py (right):

https://codereview.appspot.com/9920043/diff/25001/tests/test_http.py#newcode328
tests/test_http.py:328: for retry_num in xrange(num_retries):
On 2013/06/13 19:26:25, jcgregorio_google wrote:
> oh, that's tricky, can you add a comment on what you're doing here. Or switch
it
> to an explicit list of expected_sleeptimes =[1, 2, ...]

Done.

https://codereview.appspot.com/9920043/diff/25001/tests/test_http.py#newcode659
tests/test_http.py:659: request.execute()
On 2013/06/13 19:26:25, jcgregorio_google wrote:
> add:
> 
>    self.fail("Should have raised exception.")

Done.

[jcgregorio_google, 2013-06-14 20:27:17]

On 2013/06/13 20:15:22, yovadia wrote:
> Thanks; PTAL.
> 
> https://codereview.appspot.com/9920043/diff/25001/apiclient/http.py
> File apiclient/http.py (right):
> 
> https://codereview.appspot.com/9920043/diff/25001/apiclient/http.py#newcode672
> apiclient/http.py:672: binary exponential backoff. If all retries fail, the
> raised
> On 2013/06/13 19:26:25, jcgregorio_google wrote:
> > Do we need to specify 'binary', or is just 'exponential' detailed enough?
> 
> Done.
> 
> https://codereview.appspot.com/9920043/diff/25001/tests/test_http.py
> File tests/test_http.py (right):
> 
>
https://codereview.appspot.com/9920043/diff/25001/tests/test_http.py#newcode328
> tests/test_http.py:328: for retry_num in xrange(num_retries):
> On 2013/06/13 19:26:25, jcgregorio_google wrote:
> > oh, that's tricky, can you add a comment on what you're doing here. Or
switch
> it
> > to an explicit list of expected_sleeptimes =[1, 2, ...]
> 
> Done.
> 
>
https://codereview.appspot.com/9920043/diff/25001/tests/test_http.py#newcode659
> tests/test_http.py:659: request.execute()
> On 2013/06/13 19:26:25, jcgregorio_google wrote:
> > add:
> > 
> >    self.fail("Should have raised exception.")
> 
> Done.

LGTM, thanks!

[jcgregorio_google, 2013-06-14 20:33:15]

On 2013/06/14 20:27:17, jcgregorio_google wrote:
> On 2013/06/13 20:15:22, yovadia wrote:
> > Thanks; PTAL.
> > 
> > https://codereview.appspot.com/9920043/diff/25001/apiclient/http.py
> > File apiclient/http.py (right):
> > 
> >
https://codereview.appspot.com/9920043/diff/25001/apiclient/http.py#newcode672
> > apiclient/http.py:672: binary exponential backoff. If all retries fail, the
> > raised
> > On 2013/06/13 19:26:25, jcgregorio_google wrote:
> > > Do we need to specify 'binary', or is just 'exponential' detailed enough?
> > 
> > Done.
> > 
> > https://codereview.appspot.com/9920043/diff/25001/tests/test_http.py
> > File tests/test_http.py (right):
> > 
> >
>
https://codereview.appspot.com/9920043/diff/25001/tests/test_http.py#newcode328
> > tests/test_http.py:328: for retry_num in xrange(num_retries):
> > On 2013/06/13 19:26:25, jcgregorio_google wrote:
> > > oh, that's tricky, can you add a comment on what you're doing here. Or
> switch
> > it
> > > to an explicit list of expected_sleeptimes =[1, 2, ...]
> > 
> > Done.
> > 
> >
>
https://codereview.appspot.com/9920043/diff/25001/tests/test_http.py#newcode659
> > tests/test_http.py:659: request.execute()
> > On 2013/06/13 19:26:25, jcgregorio_google wrote:
> > > add:
> > > 
> > >    self.fail("Should have raised exception.")
> > 
> > Done.
> 
> LGTM, thanks!

Committed at
https://code.google.com/p/google-api-python-client/source/detail?r=b88d4a00ee...