More strategic retries in fetch.

More strategic retries in fetch.

Define a generic conditional exponential backoff retry decorator.
Reimplement the content in fetch to use this decorator instead of
"_retry". Move decorator usage to the specific site of retryable failure
and only retry known-retryable errors so we don't waste time on failures.

This also adds a retry to Gitiles JSON fetching and a unit test to
assert that this retry logic works successfully.

Defines GitilesFetchError, which includes the status code and error text
ain the output message.

BUG=chromium:649529, chromium:649466
TEST=unit

Committed: https://github.com/luci/recipes-py/commit/044dba0634a396fd78f97f0f633136420780fc37

[dnj, 2016-09-23 00:42:45]

dnj@chromium.org changed reviewers:
+ iannucci@chromium.org, martiniss@chromium.org

[dnj, 2016-09-23 00:42:46]

PTAL

https://codereview.chromium.org/2362993002/diff/1/recipe_engine/fetch.py
File recipe_engine/fetch.py (right):

https://codereview.chromium.org/2362993002/diff/1/recipe_engine/fetch.py#newc...
recipe_engine/fetch.py:57: @staticmethod
This was a pylint failure b/c it's static in child classes and not here.

https://codereview.chromium.org/2362993002/diff/1/recipe_engine/fetch.py#newc...
recipe_engine/fetch.py:87: class UncleanFilesystemError(FetchError):
(Moved closer to GitBackend)

[dnj, 2016-09-23 00:43:27]

Description was changed from

==========
More strategic retries in fetch.

Define a generic conditional exponential backoff retry decorator.
Reimplement the content in fetch to use this decorator instead of
"_retry". Move decorator usage to the specific site of retryable failure
and only retry known-retryable errors so we don't waste time on failures.

This also adds a retry to Gitiles JSON fetching and a unit test to
assert that this retry logic works successfully.

Defines GitilesFetchError, which includes the status code and error text
ain the output message.

BUG=
==========

to

==========
More strategic retries in fetch.

Define a generic conditional exponential backoff retry decorator.
Reimplement the content in fetch to use this decorator instead of
"_retry". Move decorator usage to the specific site of retryable failure
and only retry known-retryable errors so we don't waste time on failures.

This also adds a retry to Gitiles JSON fetching and a unit test to
assert that this retry logic works successfully.

Defines GitilesFetchError, which includes the status code and error text
ain the output message.

BUG=chromium:649529
TEST=unit
==========

[martiniss, 2016-09-23 00:49:50]

looks great, thanks for doing this :)

https://codereview.chromium.org/2362993002/diff/1/recipe_engine/fetch.py
File recipe_engine/fetch.py (right):

https://codereview.chromium.org/2362993002/diff/1/recipe_engine/fetch.py#newc...
recipe_engine/fetch.py:171: """Returns (bool): True "e" is a GitilesFetchError
with transient HTTP code.
nit: docs formatting

https://codereview.chromium.org/2362993002/diff/1/recipe_engine/fetch.py#newc...
recipe_engine/fetch.py:206: recipes_cfg_text = base64.b64decode(
Can you add something like what I did:

 try:
      recipes_cfg_text = base64.b64decode(recipes_cfg_request.text)
    except UnicodeEncodeError as e:
      # UnicodeEncodeError can happen sometimes, if gitiles gives us some weird
      # response. Just retry the whole thing.
      logging.exception(
          "got strange response from gitiles: %s", recipes_cfg_request.text)
      raise RetryFetch(e)

[martiniss, 2016-09-23 00:59:12]

Description was changed from

==========
More strategic retries in fetch.

Define a generic conditional exponential backoff retry decorator.
Reimplement the content in fetch to use this decorator instead of
"_retry". Move decorator usage to the specific site of retryable failure
and only retry known-retryable errors so we don't waste time on failures.

This also adds a retry to Gitiles JSON fetching and a unit test to
assert that this retry logic works successfully.

Defines GitilesFetchError, which includes the status code and error text
ain the output message.

BUG=chromium:649529
TEST=unit
==========

to

==========
More strategic retries in fetch.

Define a generic conditional exponential backoff retry decorator.
Reimplement the content in fetch to use this decorator instead of
"_retry". Move decorator usage to the specific site of retryable failure
and only retry known-retryable errors so we don't waste time on failures.

This also adds a retry to Gitiles JSON fetching and a unit test to
assert that this retry logic works successfully.

Defines GitilesFetchError, which includes the status code and error text
ain the output message.

BUG=chromium:649529,chromium:649466
TEST=unit
==========

[dnj, 2016-09-23 01:40:55]

https://codereview.chromium.org/2362993002/diff/1/recipe_engine/fetch.py
File recipe_engine/fetch.py (right):

https://codereview.chromium.org/2362993002/diff/1/recipe_engine/fetch.py#newc...
recipe_engine/fetch.py:171: """Returns (bool): True "e" is a GitilesFetchError
with transient HTTP code.
On 2016/09/23 00:49:50, martiniss wrote:
> nit: docs formatting

Done.

https://codereview.chromium.org/2362993002/diff/1/recipe_engine/fetch.py#newc...
recipe_engine/fetch.py:206: recipes_cfg_text = base64.b64decode(
On 2016/09/23 00:49:50, martiniss wrote:
> Can you add something like what I did:
> 
>  try:
>       recipes_cfg_text = base64.b64decode(recipes_cfg_request.text)
>     except UnicodeEncodeError as e:
>       # UnicodeEncodeError can happen sometimes, if gitiles gives us some
weird
>       # response. Just retry the whole thing.
>       logging.exception(
>           "got strange response from gitiles: %s", recipes_cfg_request.text)
>       raise RetryFetch(e)

Does Gitiles actually return 200 w/ a UnicodeError? b/c before, this code wasn't
looking at status codes. Now it does, so a non-200 response will not get as far
as parsing.

If Gitiles can return a 200 and still give junk data, that's a GItiles bug that
needs to be fixed and I'll add this handling code.

[martiniss, 2016-09-23 01:45:39]

lgtm

[dnj, 2016-09-23 01:50:30]

The CQ bit was checked by dnj@chromium.org

[commit-bot: I haz the power, 2016-09-23 01:50:34]

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-09-23 01:53:10]

Description was changed from

==========
More strategic retries in fetch.

Define a generic conditional exponential backoff retry decorator.
Reimplement the content in fetch to use this decorator instead of
"_retry". Move decorator usage to the specific site of retryable failure
and only retry known-retryable errors so we don't waste time on failures.

This also adds a retry to Gitiles JSON fetching and a unit test to
assert that this retry logic works successfully.

Defines GitilesFetchError, which includes the status code and error text
ain the output message.

BUG=chromium:649529,chromium:649466
TEST=unit
==========

to

==========
More strategic retries in fetch.

Define a generic conditional exponential backoff retry decorator.
Reimplement the content in fetch to use this decorator instead of
"_retry". Move decorator usage to the specific site of retryable failure
and only retry known-retryable errors so we don't waste time on failures.

This also adds a retry to Gitiles JSON fetching and a unit test to
assert that this retry logic works successfully.

Defines GitilesFetchError, which includes the status code and error text
ain the output message.

BUG=chromium:649529,chromium:649466
TEST=unit

Committed:
https://github.com/luci/recipes-py/commit/044dba0634a396fd78f97f0f63313642078...
==========

[commit-bot: I haz the power, 2016-09-23 01:53:12]

Committed patchset #2 (id:20001) as
https://github.com/luci/recipes-py/commit/044dba0634a396fd78f97f0f63313642078...