CQ: handle more error conditions in try_job_on_rietveld.py

CQ: handle more error conditions in try_job_on_rietveld.py

This catches errors I observed running local CQ instance.
All errors are temporary network/service hiccups
and so are caught and logged as warnings.

BUG=none

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

[Isaac (away), 2013-10-22 21:05:12]

Can you provide more context / a bug for this?

[Paweł Hajdan Jr., 2013-10-23 21:47:37]

+Robbie

I talked with Isaac offline about this, and I updated the commit message.

Just trying to get a review.

[Isaac (away), 2013-10-24 17:25:41]

https://chromiumcodereview.appspot.com/33443003/diff/1/commit-queue/verificat...
File commit-queue/verification/try_job_on_rietveld.py (right):

https://chromiumcodereview.appspot.com/33443003/diff/1/commit-queue/verificat...
commit-queue/verification/try_job_on_rietveld.py:791: else:
Would this make more sense for rietveld.py to handle and return None?

https://chromiumcodereview.appspot.com/33443003/diff/1/commit-queue/verificat...
commit-queue/verification/try_job_on_rietveld.py:811: if e.errno ==
errno.ECONNRESET:
We're now raising if this doesn't match?

[Paweł Hajdan Jr., 2013-10-24 18:28:12]

https://chromiumcodereview.appspot.com/33443003/diff/1/commit-queue/verificat...
File commit-queue/verification/try_job_on_rietveld.py (right):

https://chromiumcodereview.appspot.com/33443003/diff/1/commit-queue/verificat...
commit-queue/verification/try_job_on_rietveld.py:791: else:
On 2013/10/24 17:25:41, Isaac wrote:
> Would this make more sense for rietveld.py to handle and return None?

I don't think so. If anything, it might be better to raise a Rietveld-specific
exception wrapping urllib's one, but I don't think it's worth it here.

Returning None from Rietveld loses information about what exactly happened, and
this info can be useful to the client (as you see in this CL we do inspect the
exception and react a bit differently depending on error).

https://chromiumcodereview.appspot.com/33443003/diff/1/commit-queue/verificat...
commit-queue/verification/try_job_on_rietveld.py:811: if e.errno ==
errno.ECONNRESET:
On 2013/10/24 17:25:41, Isaac wrote:
> We're now raising if this doesn't match?

If we're catching it here it means something in the try block throws it, which
means previously it would just keep unwinding the stack.

Now we catch and handle just the connection reset, while other errors still
raise, just as before.

[Isaac (away), 2013-10-24 19:01:33]

https://chromiumcodereview.appspot.com/33443003/diff/1/commit-queue/verificat...
File commit-queue/verification/try_job_on_rietveld.py (right):

https://chromiumcodereview.appspot.com/33443003/diff/1/commit-queue/verificat...
commit-queue/verification/try_job_on_rietveld.py:791: else:
On 2013/10/24 18:28:12, Paweł Hajdan Jr. wrote:
> On 2013/10/24 17:25:41, Isaac wrote:
> > Would this make more sense for rietveld.py to handle and return None?
> 
> I don't think so. If anything, it might be better to raise a Rietveld-specific
> exception wrapping urllib's one, but I don't think it's worth it here.
> 
> Returning None from Rietveld loses information about what exactly happened,
and
> this info can be useful to the client (as you see in this CL we do inspect the
> exception and react a bit differently depending on error).

Python 3 supports native exception chaining.  In Python 2, you can do this:

except urllib2.HTTPError as e:
    raise RietveldError(str(e)), None, sys.exc_info()[2]

Might be nice to have this logic into rietveld.py so that assumptions about
server behavior are in a single place.

[Isaac (away), 2013-10-24 19:13:49]

lgtm

[commit-bot: I haz the power, 2013-10-24 19:20:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/phajdan.jr@chromium.org/33443003/1

[commit-bot: I haz the power, 2013-10-24 19:21:06]

Change committed as 230786