[Findit] Record error if swarming task completes with exit_code as 2.

[Findit] Record error if swarming task completes with exit_code as 2.

BUG=643864, 643821
Committed: https://chromium.googlesource.com/infra/infra/+/398a698ca5a548510db4c83f6b09659b4c5e0523

[chanli, 2016-09-07 00:27:32]

chanli@chromium.org changed reviewers:
+ katesonia@chromium.org, lijeffrey@chromium.org, stgao@chromium.org

[chanli, 2016-09-07 00:27:33]

ptal

[lijeffrey, 2016-09-07 20:41:38]

https://codereview.chromium.org/2312413002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py
(right):

https://codereview.chromium.org/2312413002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:90:
else data['exit_code'])
how about the opposite for readability?

exit_code = data['exit_code'] if task_state == swarming_util.STATE_COMPLETED
else None

https://codereview.chromium.org/2312413002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:96: if
task_state == swarming_util.STATE_COMPLETED and int(exit_code) < 2:
why 2? Is there something special about anything < 2? How about explicitly
checking for specific error codes defined in constants to make the code more
clear/robust?

[chanli, 2016-09-07 22:37:12]

https://codereview.chromium.org/2312413002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py
(right):

https://codereview.chromium.org/2312413002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:90:
else data['exit_code'])
On 2016/09/07 20:41:38, lijeffrey wrote:
> how about the opposite for readability?
> 
> exit_code = data['exit_code'] if task_state == swarming_util.STATE_COMPLETED
> else None

Done.

https://codereview.chromium.org/2312413002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:96: if
task_state == swarming_util.STATE_COMPLETED and int(exit_code) < 2:
On 2016/09/07 20:41:38, lijeffrey wrote:
> why 2? Is there something special about anything < 2? How about explicitly
> checking for specific error codes defined in constants to make the code more
> clear/robust?

exit_code == 0 means all tests passed; exit_code == 1 means some tests failed;
exit_code == 2 means the task stopped unexpectedly because of some error, where
we'll not get any failure logs for tests - in fact, no log for any tests at all.

[chanli, 2016-09-08 00:02:36]

Description was changed from

==========
[Findit] Record error if swarming task completes with exit_code as 2.

BUG=643864
==========

to

==========
[Findit] Record error if swarming task completes with exit_code as 2.

BUG=643864, 643821
==========

[lijeffrey, 2016-09-08 03:41:00]

https://codereview.chromium.org/2312413002/diff/40001/appengine/findit/waterf...
File appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py
(right):

https://codereview.chromium.org/2312413002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:17:
_ACCEPTED_EXIT_CODE = [0, 1]
nit: _ACCEPTED_EXIT_CODES

on that note, how about moving these error codes explicit constants for each
type of error (ALL_TESTS_PASSED = 0, SOME_TESTS_FAILED = 1, etc) to a separate
module kind of like analysis_status.py, or even define them as constants here,
and include a dict that maps codes to messages? Then include the error message
in the log, not just status and code.

[stgao, 2016-09-08 04:16:00]

https://codereview.chromium.org/2312413002/diff/40001/appengine/findit/waterf...
File appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py
(right):

https://codereview.chromium.org/2312413002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:17:
_ACCEPTED_EXIT_CODE = [0, 1]
On 2016/09/08 03:41:00, lijeffrey wrote:
> nit: _ACCEPTED_EXIT_CODES
> 
> on that note, how about moving these error codes explicit constants for each
> type of error (ALL_TESTS_PASSED = 0, SOME_TESTS_FAILED = 1, etc) to a separate
> module kind of like analysis_status.py, or even define them as constants here,
> and include a dict that maps codes to messages? Then include the error message
> in the log, not just status and code. 

+1
We also need comments for possible exit codes, and some explanation for them.

[chanli, 2016-09-08 21:16:56]

https://codereview.chromium.org/2312413002/diff/40001/appengine/findit/waterf...
File appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py
(right):

https://codereview.chromium.org/2312413002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:17:
_ACCEPTED_EXIT_CODE = [0, 1]
On 2016/09/08 03:41:00, lijeffrey wrote:
> nit: _ACCEPTED_EXIT_CODES
> 
> on that note, how about moving these error codes explicit constants for each
> type of error (ALL_TESTS_PASSED = 0, SOME_TESTS_FAILED = 1, etc) to a separate
> module kind of like analysis_status.py, or even define them as constants here,
> and include a dict that maps codes to messages? Then include the error message
> in the log, not just status and code. 

Done.

https://codereview.chromium.org/2312413002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:17:
_ACCEPTED_EXIT_CODE = [0, 1]
On 2016/09/08 04:16:00, stgao wrote:
> On 2016/09/08 03:41:00, lijeffrey wrote:
> > nit: _ACCEPTED_EXIT_CODES
> > 
> > on that note, how about moving these error codes explicit constants for each
> > type of error (ALL_TESTS_PASSED = 0, SOME_TESTS_FAILED = 1, etc) to a
separate
> > module kind of like analysis_status.py, or even define them as constants
here,
> > and include a dict that maps codes to messages? Then include the error
message
> > in the log, not just status and code. 
> 
> +1
> We also need comments for possible exit codes, and some explanation for them.

Since the name for each exit_code is self_explained, I didn't add comments here.

[Sharu Jiang, 2016-09-08 21:41:55]

https://codereview.chromium.org/2312413002/diff/60001/appengine/findit/waterf...
File appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py
(right):

https://codereview.chromium.org/2312413002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:97:
print exit_code
Are these 2 prints intended?

[stgao, 2016-09-08 22:02:50]

lgtm

[chanli, 2016-09-08 22:07:12]

The CQ bit was checked by chanli@chromium.org

[chanli, 2016-09-08 22:07:12]

The patchset sent to the CQ was uploaded after l-g-t-m from stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/2312413002/#ps80001
(title: ".")

[commit-bot: I haz the power, 2016-09-08 22:07:21]

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

[chanli, 2016-09-08 22:07:21]

https://codereview.chromium.org/2312413002/diff/60001/appengine/findit/waterf...
File appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py
(right):

https://codereview.chromium.org/2312413002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:97:
print exit_code
On 2016/09/08 21:41:54, sharu jiang wrote:
> Are these 2 prints intended?

Done.

[Sharu Jiang, 2016-09-08 22:10:34]

lgtm

[lijeffrey, 2016-09-08 22:14:05]

https://codereview.chromium.org/2312413002/diff/60001/appengine/findit/waterf...
File appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py
(right):

https://codereview.chromium.org/2312413002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:109:
logging_str = 'Swarming task stopped with status: %s' % task_state
nit: rename this error_str, since it's used in the context of logging.error

https://codereview.chromium.org/2312413002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:110: if
exit_code:
is it possible for exit_code to be 0 here instead of None? If so this may be a
bug and we should check if exit_code is not None

https://codereview.chromium.org/2312413002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:112:
exit_code, swarming_util.EXIT_CODE_DESCRIPTIONS[int(exit_code)])
nit: since int(exit_code) is now being used in 2 places, how about move it out
to exit_code_int = int(exit_code) and use exit_code_int?

[commit-bot: I haz the power, 2016-09-08 22:21:38]

Description was changed from

==========
[Findit] Record error if swarming task completes with exit_code as 2.

BUG=643864, 643821
==========

to

==========
[Findit] Record error if swarming task completes with exit_code as 2.

BUG=643864, 643821

Committed:
https://chromium.googlesource.com/infra/infra/+/398a698ca5a548510db4c83f6b096...
==========

[commit-bot: I haz the power, 2016-09-08 22:21:39]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/infra/infra/+/398a698ca5a548510db4c83f6b096...