RetriableHttp to sleep between retries

RetriableHttp to sleep between retries

R=sergeyberezin@chromium.org
BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/97d61b58808647475f4da7573f5ffa5537a33d4d

[tnn, 2016-07-14 18:41:54]

We need some sort of sleep between each retry.

Another thing is that I feel retrying 5 times is too many. Maybe 2 is enough
because we send a request again in the next minute anyway.

If MonAPI accidentally returns 500, we will send 5 times more traffic to them.

Please let me know what you think.

[Sergey Berezin, 2016-07-14 21:10:17]

LGTM + nit (but feel free to ignore it, it's mostly FYI).

https://codereview.chromium.org/2153483002/diff/1/infra_libs/test/httplib2_ut...
File infra_libs/test/httplib2_utils_test.py (right):

https://codereview.chromium.org/2153483002/diff/1/infra_libs/test/httplib2_ut...
infra_libs/test/httplib2_utils_test.py:165: @mock.patch('time.sleep',
autospec=True)
nit: in this case it's likely not as relevant, but we often mock time functions
by passing them as an extra parameter. E.g.:

class RetriableHttp()
  def __init__(self, ..., sleep=time.sleep): ...

and in the test:

  self.http = infra_libs.RetriableHttp(..., sleep=mock.Mock(...))

This way, if some other random function actually depends on time, we don't
deadlock mysteriously.

[tnn, 2016-07-14 21:37:02]

On 2016/07/14 21:10:17, Sergey Berezin wrote:
> LGTM + nit (but feel free to ignore it, it's mostly FYI).
> 
>
https://codereview.chromium.org/2153483002/diff/1/infra_libs/test/httplib2_ut...
> File infra_libs/test/httplib2_utils_test.py (right):
> 
>
https://codereview.chromium.org/2153483002/diff/1/infra_libs/test/httplib2_ut...
> infra_libs/test/httplib2_utils_test.py:165: @mock.patch('time.sleep',
> autospec=True)
> nit: in this case it's likely not as relevant, but we often mock time
functions
> by passing them as an extra parameter. E.g.:
> 
> class RetriableHttp()
>   def __init__(self, ..., sleep=time.sleep): ...
> 
> and in the test:
> 
>   self.http = infra_libs.RetriableHttp(..., sleep=mock.Mock(...))
> 
> This way, if some other random function actually depends on time, we don't
> deadlock mysteriously.

As we talked offline, we'll keep it and later refactor it if we need to. Thanks!

[tnn, 2016-07-14 21:37:25]

On 2016/07/14 21:37:02, tanin wrote:
> On 2016/07/14 21:10:17, Sergey Berezin wrote:
> > LGTM + nit (but feel free to ignore it, it's mostly FYI).
> > 
> >
>
https://codereview.chromium.org/2153483002/diff/1/infra_libs/test/httplib2_ut...
> > File infra_libs/test/httplib2_utils_test.py (right):
> > 
> >
>
https://codereview.chromium.org/2153483002/diff/1/infra_libs/test/httplib2_ut...
> > infra_libs/test/httplib2_utils_test.py:165: @mock.patch('time.sleep',
> > autospec=True)
> > nit: in this case it's likely not as relevant, but we often mock time
> functions
> > by passing them as an extra parameter. E.g.:
> > 
> > class RetriableHttp()
> >   def __init__(self, ..., sleep=time.sleep): ...
> > 
> > and in the test:
> > 
> >   self.http = infra_libs.RetriableHttp(..., sleep=mock.Mock(...))
> > 
> > This way, if some other random function actually depends on time, we don't
> > deadlock mysteriously.
> 
> As we talked offline, we'll keep it and later refactor it if we need to.
Thanks!

I forgot to mention the reason. Because the current code is shorter.

[tnn, 2016-07-14 21:37:35]

The CQ bit was checked by tanin@chromium.org

[commit-bot: I haz the power, 2016-07-14 21:37:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 21:51:52]

Description was changed from

==========
RetriableHttp to sleep between retries

R=sergeyberezin@chromium.org
BUG=
==========

to

==========
RetriableHttp to sleep between retries

R=sergeyberezin@chromium.org
BUG=

Committed:
https://chromium.googlesource.com/infra/infra/+/97d61b58808647475f4da7573f5ff...
==========

[commit-bot: I haz the power, 2016-07-14 21:51:52]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/infra/infra/+/97d61b58808647475f4da7573f5ff...

[commit-bot: I haz the power, 2016-07-14 21:51:55]

CQ bit was unchecked.