Implement HttpStatusPush receiver to keep try job status.

Implement HttpStatusPush receiver to keep try job status.

Implement TryJobResult object.

[M-A, 2011-11-01 20:48:39]

Base on http://codereview.appspot.com/5303090/.

[M-A, 2011-11-04 20:33:11]

ping

[cmp, 2011-11-04 22:29:07]

http://codereview.appspot.com/5331058/diff/5001/codereview/views.py
File codereview/views.py (right):

http://codereview.appspot.com/5331058/diff/5001/codereview/views.py#newcode2370
codereview/views.py:2370: 'try_job_results': [t.as_dict() for t in
patchset.try_job_results],
I think you mean to_dict() here.

http://codereview.appspot.com/5331058/diff/5001/codereview/views.py#newcode2373
codereview/views.py:2373: # TODO(maruel): Remove.
please update to mention when this code can be removed.  probably "Remove after
all tryservers are upgraded to not use the build result interface." or such.

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py
File codereview/views_chromium.py (right):

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:59: """Parses a string into datetime including
microseconds."""
please modify this docstring to show an example string that this method would
format

would this be an expected value? "1970-01-01 00:00:00.0000"

it seems exceptionally permissive.  if unexpected values are passed, the app
will crash with ValueError: time data '<bad string>' does not match format
'%Y-%m-%d %H:%M:%S'.  maybe catch this result, log it, and return None in those
cases.

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:117: TryJobResult obhect.
s/obhect/object/

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:131: pass
from experience let's log these errors instead of just passing

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:133: logging.warn('Bad packet, no issue or
patchset: %r' % properties)
if we get an exception from line 130, i claim we also got a bad/unexpected
packet.  we should error out here for those, too.

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:138: result = result[0]
How many result variations are there?  Are they due to Buildbot 0.7 vs 0.8
differences, or TryJobStatus/HttpStatusPush differences?

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:172:
models.TryJobResult.result_priority(try_obj.result)):
Did you consider having some sort of comparator method to simplify the syntax?

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:173: # Update result only if relevant.
this comment covers line 171/172 (explains that we're first checking relevancy).
 please move it to be before 171 and have it be something like 'Check if the
result is relevant and update it if so.'

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:177: return
db.run_in_transaction(tx_try_job_result)
can this generate an exception if the record can't be written (eg. appengine
outage)?  if it raises an exception, what happens to the rest of the packets? 
since those never hit the datastore, i expect we lose them, which would be bad.

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:187: def process_status_push(packets_json):
Boy, this looks familiar.  Where did I see this flow before? :)

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:205: logging.warn('Invalid pakect %r' % packet)
s/pakect/packet/

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:208: has_update = has_update or this_has_update
I don't see code using has_update.  I can imagine why you would want to use it,
but if it's not being used, then we can just call the handler and skip setting
this_has_update and has_update.

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:390: # TODO(maruel): Auth.
auth sooner, can we set a value on a config object in the datastore and use that
as a key sufficient for now?

[M-A, 2011-11-11 22:07:39]

http://codereview.appspot.com/5331058/diff/5001/codereview/views.py
File codereview/views.py (right):

http://codereview.appspot.com/5331058/diff/5001/codereview/views.py#newcode2370
codereview/views.py:2370: 'try_job_results': [t.as_dict() for t in
patchset.try_job_results],
On 2011/11/04 22:29:07, cmp wrote:
> I think you mean to_dict() here.

Done.

http://codereview.appspot.com/5331058/diff/5001/codereview/views.py#newcode2373
codereview/views.py:2373: # TODO(maruel): Remove.
On 2011/11/04 22:29:07, cmp wrote:
> please update to mention when this code can be removed. 

Removed.

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py
File codereview/views_chromium.py (right):

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:59: """Parses a string into datetime including
microseconds."""
On 2011/11/04 22:29:07, cmp wrote:
> please modify this docstring to show an example string that this method would
> format
> 
> would this be an expected value? "1970-01-01 00:00:00.0000"

It's really just str(datetime.datetime()). I added that to the docstring.

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:117: TryJobResult obhect.
On 2011/11/04 22:29:07, cmp wrote:
> s/obhect/object/

done

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:131: pass
On 2011/11/04 22:29:07, cmp wrote:
> from experience let's log these errors instead of just passing

done

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:133: logging.warn('Bad packet, no issue or
patchset: %r' % properties)
On 2011/11/04 22:29:07, cmp wrote:
> if we get an exception from line 130, i claim we also got a bad/unexpected
> packet.  we should error out here for those, too.

done

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:138: result = result[0]
On 2011/11/04 22:29:07, cmp wrote:
> How many result variations are there?  Are they due to Buildbot 0.7 vs 0.8
> differences, or TryJobStatus/HttpStatusPush differences?

No, it's dependent on how buildbot feels, I don't recall the exact logic but
it's embedded the result inside a list as needed.

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:172:
models.TryJobResult.result_priority(try_obj.result)):
On 2011/11/04 22:29:07, cmp wrote:
> Did you consider having some sort of comparator method to simplify the syntax?

Ambivalent here.

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:173: # Update result only if relevant.
On 2011/11/04 22:29:07, cmp wrote:
> this comment covers line 171/172 (explains that we're first checking
relevancy).
>  please move it to be before 171 and have it be something like 'Check if the
> result is relevant and update it if so.'

done.

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:177: return
db.run_in_transaction(tx_try_job_result)
On 2011/11/04 22:29:07, cmp wrote:
> can this generate an exception if the record can't be written (eg. appengine
> outage)?  if it raises an exception, what happens to the rest of the packets? 
> since those never hit the datastore, i expect we lose them, which would be
bad.

In the meantime, I've switched to synchronous processing, which fixes this
problem by having HttpStatusPush keeps the backlog on server-side exceptions.

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:205: logging.warn('Invalid pakect %r' % packet)
On 2011/11/04 22:29:07, cmp wrote:
> s/pakect/packet/

done.

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:208: has_update = has_update or this_has_update
On 2011/11/04 22:29:07, cmp wrote:
> I don't see code using has_update.  I can imagine why you would want to use
it,
> but if it's not being used, then we can just call the handler and skip setting
> this_has_update and has_update.

Removed, it was used for debugging.

http://codereview.appspot.com/5331058/diff/5001/codereview/views_chromium.py#...
codereview/views_chromium.py:390: # TODO(maruel): Auth.
On 2011/11/04 22:29:07, cmp wrote:
> auth sooner, can we set a value on a config object in the datastore and use
that
> as a key sufficient for now?

Done.

[M-A, 2011-11-15 14:35:09]

ping

[cmp, 2011-11-18 00:52:13]

lgtm

[cmp, 2011-11-18 00:52:22]

http://codereview.appspot.com/5331058/diff/18001/codereview/views_chromium.py
File codereview/views_chromium.py (right):

http://codereview.appspot.com/5331058/diff/18001/codereview/views_chromium.py...
codereview/views_chromium.py:58: def key_required(func):
This is cool, nice.