apply_issue: catch all Rietveld flakiness failures.

apply_issue: catch all Rietveld flakiness failures.

R=iannucci@chromium.org
BUG=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=296990

[Sergey Berezin, 2015-10-02 17:18:48]

sergeyberezin@chromium.org changed reviewers:
+ sergeyberezin@chromium.org

[Sergey Berezin, 2015-10-02 17:18:48]

LGTM since we're on fire, but this code is due for  cleanup once the fire's
down. Thanks!

[tandrii(chromium), 2015-10-02 17:19:35]

The CQ bit was checked by tandrii@chromium.org

[commit-bot: I haz the power, 2015-10-02 17:19:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1375343005/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1375343005/1

[commit-bot: I haz the power, 2015-10-02 17:21:54]

Committed patchset #1 (id:1) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=296990

[jam, 2015-10-03 00:56:53]

nit: looks like
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...
got diagnosed as infra failure, when the developer had just deleted the
patchset. we probably want to not  mark those as infra failures.

[tandrii(chromium), 2015-10-05 13:49:53]

On 2015/10/03 00:56:53, jam wrote:
> nit: looks like
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...
> got diagnosed as infra failure, when the developer had just deleted the
> patchset. we probably want to not  mark those as infra failures.

Good point. So i refactored this panic-mode code into smarter one. Now if
specific PS doesn't exist, it returns 1 as before.
See https://codereview.chromium.org/1373363006

[Sergiy Byelozyorov, 2015-10-05 20:02:27]

sergiyb@chromium.org changed reviewers:
+ sergiyb@chromium.org

[Sergiy Byelozyorov, 2015-10-05 20:02:27]

Similar patch is needed in git_cl.py:1070 and git_cl.py:1082. Probably also with
retries... we fail to catch URLError there and fail all the way though.

[Sergey Berezin, 2015-10-05 21:57:13]

On 2015/10/05 20:02:27, Sergiy Byelozyorov wrote:
> Similar patch is needed in git_cl.py:1070 and git_cl.py:1082. Probably also
with
> retries... we fail to catch URLError there and fail all the way though.

Filed http://crbug.com/539584 as part of the postmortem action items.

[tandrii(chromium), 2015-10-05 21:58:12]

On 2015/10/05 20:02:27, Sergiy Byelozyorov wrote:
> Similar patch is needed in git_cl.py:1070 and git_cl.py:1082. Probably also
with
> retries... we fail to catch URLError there and fail all the way though.

I disagree. This patch doesn't add any retry logic, it merely changes return
code. `git cl *` isn't used on normal bots, afaik, only on autorollers and other
specialized tooling. Without changing that tooling, changing return code for git
cl isn't very useful.
 
Finally, the retries are inside rietveld.py, and since I landed this
https://codereview.chromium.org/1388463002 this weird URlError won't be raised
any more, unless it fails 40 times.

[Sergey Berezin, 2015-10-05 22:02:48]

On 2015/10/05 21:58:12, tandrii(chromium) wrote:
> On 2015/10/05 20:02:27, Sergiy Byelozyorov wrote:
> > Similar patch is needed in git_cl.py:1070 and git_cl.py:1082. Probably also
> with
> > retries... we fail to catch URLError there and fail all the way though.
> 
> I disagree. This patch doesn't add any retry logic, it merely changes return
> code. `git cl *` isn't used on normal bots, afaik, only on autorollers and
other
> specialized tooling. 

Autorollers are bots, so it's used :-) I don't see how at least some limited
retry can hurt there. Dev's can also be frustrated when uploading a CL fails due
to a brief connection fluke. 

Anyways, comments are welcome on the bug - http://crbug.com/539584

> Without changing that tooling, changing return code for git
> cl isn't very useful.
>  
> Finally, the retries are inside rietveld.py, and since I landed this
> https://codereview.chromium.org/1388463002 this weird URlError won't be raised
> any more, unless it fails 40 times.

[tandrii(chromium), 2015-10-05 22:25:05]

On 2015/10/05 22:02:48, Sergey Berezin wrote:
> On 2015/10/05 21:58:12, tandrii(chromium) wrote:
> > On 2015/10/05 20:02:27, Sergiy Byelozyorov wrote:
> > > Similar patch is needed in git_cl.py:1070 and git_cl.py:1082. Probably
also
> > with
> > > retries... we fail to catch URLError there and fail all the way though.
> > 
> > I disagree. This patch doesn't add any retry logic, it merely changes return
> > code. `git cl *` isn't used on normal bots, afaik, only on autorollers and
> other
> > specialized tooling. 
> 
> Autorollers are bots, so it's used :-) I don't see how at least some limited
> retry can hurt there. 
Agree, it doesn't hurt, and after this new Win error as well most others (like
5xx) are retried 40 times with 15 secs timeout each (by default). That's what
that CL below was for. Since most tools import rietveld.py to talk to rietveld,
duplicating retry logic in them is not good. But retrying all but whitelisted
error in rietveld.py is a good thing (http://crbug.com/539580).

We still have buildbucket and stuff, so thanks for filing that second bug!

> > Finally, the retries are inside rietveld.py, and since I landed this
> > https://codereview.chromium.org/1388463002 this weird URlError won't be
raised
> > any more, unless it fails 40 times.