[Findit] Pipeline change to save suspected cls to data store.

[Findit] Pipeline change to save suspected cls to data store.

BUG=636199
Committed: https://chromium.googlesource.com/infra/infra/+/29add55a8e77c144218429a140a017ae82d9b775

[chanli, 2016-08-10 02:50:07]

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

[chanli, 2016-08-10 02:50:08]

PTAL

[lijeffrey, 2016-08-10 08:12:39]

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

https://codereview.chromium.org/2230103002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_culprit_pipeline.py:42: suspected_cl =
WfSuspectedCL.Get(repo_name, revision)
how about suspected_cl = WfSuspectedCL.Get(...) or WfSusupectedCl.Create(...)

https://codereview.chromium.org/2230103002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_culprit_pipeline.py:50:
suspected_cl.approach = analysis_approach_type.BOTH
this seems not right, can you guarantee both heuristic and try job analysis are
used if there is a try job?

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

https://codereview.chromium.org/2230103002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:79:
suspected_cl_revisions = []
Move the test suspected_cls logic to a separate function for testability

https://codereview.chromium.org/2230103002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:81: for
test_cl_info in results['tests'].values():
nit: if using itervalues() above, why not use it here too?

https://codereview.chromium.org/2230103002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:335: def
CreateOrUpdateSuspectedCL(repo_name, revision, commit_position):
nit: Why not just name this UpdateSuspectedCL?

https://codereview.chromium.org/2230103002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:335: def
CreateOrUpdateSuspectedCL(repo_name, revision, commit_position):
this looks almost the same as _SaveSuspectedCL in identify_culprit_pipeline, is
there any way they can be refactored into a common module?

https://codereview.chromium.org/2230103002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:336:
suspected_cl = WfSuspectedCL.Get(repo_name, revision)
suspected_cl = WfSuspectedCl.Get(...) or WfSuspectedCL.Create(...)

[chanli, 2016-08-11 23:39:36]

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

https://codereview.chromium.org/2230103002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_culprit_pipeline.py:42: suspected_cl =
WfSuspectedCL.Get(repo_name, revision)
On 2016/08/10 08:12:38, lijeffrey wrote:
> how about suspected_cl = WfSuspectedCL.Get(...) or WfSusupectedCl.Create(...)

Done.

https://codereview.chromium.org/2230103002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_culprit_pipeline.py:50:
suspected_cl.approach = analysis_approach_type.BOTH
On 2016/08/10 08:12:38, lijeffrey wrote:
> this seems not right, can you guarantee both heuristic and try job analysis
are
> used if there is a try job?

This update is for a heuristic result and if suspected_cl.approach has been set
to analysis_approach_type.TRY_JOB that means another try job has found the same
CL

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

https://codereview.chromium.org/2230103002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:79:
suspected_cl_revisions = []
On 2016/08/10 08:12:39, lijeffrey wrote:
> Move the test suspected_cls logic to a separate function for testability

Done.

https://codereview.chromium.org/2230103002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:81: for
test_cl_info in results['tests'].values():
On 2016/08/10 08:12:39, lijeffrey wrote:
> nit: if using itervalues() above, why not use it here too?

Done.

https://codereview.chromium.org/2230103002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:335: def
CreateOrUpdateSuspectedCL(repo_name, revision, commit_position):
On 2016/08/10 08:12:39, lijeffrey wrote:
> nit: Why not just name this UpdateSuspectedCL?

Done.

https://codereview.chromium.org/2230103002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:335: def
CreateOrUpdateSuspectedCL(repo_name, revision, commit_position):
On 2016/08/10 08:12:39, lijeffrey wrote:
> this looks almost the same as _SaveSuspectedCL in identify_culprit_pipeline,
is
> there any way they can be refactored into a common module?

Done.

https://codereview.chromium.org/2230103002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:336:
suspected_cl = WfSuspectedCL.Get(repo_name, revision)
On 2016/08/10 08:12:39, lijeffrey wrote:
> suspected_cl = WfSuspectedCl.Get(...) or WfSuspectedCL.Create(...)

Done.

[Sharu Jiang, 2016-08-12 17:45:04]

https://codereview.chromium.org/2230103002/diff/20001/appengine/findit/model/...
File appengine/findit/model/wf_suspected_cl.py (right):

https://codereview.chromium.org/2230103002/diff/20001/appengine/findit/model/...
appengine/findit/model/wf_suspected_cl.py:28: is_culprit =
ndb.BooleanProperty(indexed=True, default=None)
The property default is already None, so this can be omitted.

https://codereview.chromium.org/2230103002/diff/20001/appengine/findit/model/...
appengine/findit/model/wf_suspected_cl.py:31: approach =
ndb.IntegerProperty(indexed=True, default=None)
Ditto.

[chanli, 2016-08-16 16:53:52]

https://codereview.chromium.org/2230103002/diff/20001/appengine/findit/model/...
File appengine/findit/model/wf_suspected_cl.py (right):

https://codereview.chromium.org/2230103002/diff/20001/appengine/findit/model/...
appengine/findit/model/wf_suspected_cl.py:28: is_culprit =
ndb.BooleanProperty(indexed=True, default=None)
On 2016/08/12 17:45:04, sharu jiang wrote:
> The property default is already None, so this can be omitted.

Done.

https://codereview.chromium.org/2230103002/diff/20001/appengine/findit/model/...
appengine/findit/model/wf_suspected_cl.py:31: approach =
ndb.IntegerProperty(indexed=True, default=None)
On 2016/08/12 17:45:04, sharu jiang wrote:
> Ditto.

Done.

[lijeffrey, 2016-08-17 19:31:12]

lgtm with nits

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

https://codereview.chromium.org/2230103002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:98:
is_compile_try_job: True if the try job is for compile failure,
where is this arg?

https://codereview.chromium.org/2230103002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:354: # Updates
suspected_cl
nit: comment ends with .

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

https://codereview.chromium.org/2230103002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/suspected_cl_util.py:17: WfSuspectedCL.Get(repo_name,
revision) or
nit: 4 spaces on new lines

[chanli, 2016-09-12 20:13:18]

chanli@chromium.org changed reviewers:
+ stgao@chromium.org

[chanli, 2016-09-12 20:13:19]

Hi,

I have redone the pipeline change after we modified design. Please take a look
when you have time.

Thanks,
Chan

[chanli, 2016-09-12 20:56:16]

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

https://codereview.chromium.org/2230103002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:98:
is_compile_try_job: True if the try job is for compile failure,
On 2016/08/17 19:31:11, lijeffrey wrote:
> where is this arg?

Done.

https://codereview.chromium.org/2230103002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:354: # Updates
suspected_cl
On 2016/08/17 19:31:11, lijeffrey wrote:
> nit: comment ends with .

Done.

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

https://codereview.chromium.org/2230103002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/suspected_cl_util.py:17: WfSuspectedCL.Get(repo_name,
revision) or
On 2016/08/17 19:31:11, lijeffrey wrote:
> nit: 4 spaces on new lines

Done.

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

https://codereview.chromium.org/2230103002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:766: 
Have removed one line.

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

https://codereview.chromium.org/2230103002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:90: #   return
_GetTestTryJobSuspectedCLs(culprit)
Have deleted this part.

https://codereview.chromium.org/2230103002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:387: print
culprits
Have removed unnecessary lines.

https://codereview.chromium.org/2230103002/diff/60001/appengine/findit/waterf...
File appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py
(right):

https://codereview.chromium.org/2230103002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py:296:
print updated_cls
Removed

https://codereview.chromium.org/2230103002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py:782:
print [a_test1_suspected_cl, a_test2_suspected_cl]
Removed

https://codereview.chromium.org/2230103002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py:855: 
Removed one line.

https://codereview.chromium.org/2230103002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py:928: 
Removed one line

https://codereview.chromium.org/2230103002/diff/60001/appengine/findit/waterf...
File appengine/findit/waterfall/test/suspected_cl_util_test.py (right):

https://codereview.chromium.org/2230103002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/test/suspected_cl_util_test.py:218: #  
self.assertEqual(expected_builds, suspected_cl.builds)
This part has been removed.

[lijeffrey, 2016-09-12 22:55:11]

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:605: class _CLInfo(object):
So how about making this depend on your other cl
https://codereview.chromium.org/2320153003/ and make this a BaseCL object and
get rid of _CLInfo?

[stgao, 2016-09-15 17:35:12]

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:615: def
not_none_failures(self):
What does this mean?

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:630:
cl_failure_map[cl_key].top_score = top_score
What if the original score is higher than this one?

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:762: _SaveFailureToMap(
I'm wondering if it would be possible and cleaner if we do a post-analysis
process for saving the data into datastore?

What do you think?

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
File appengine/findit/waterfall/suspected_cl_util.py (right):

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/suspected_cl_util.py:29: suspected_cl.approach |
approach if suspected_cl.approach and
This is not that readable, how about expanding it to if/else statements?

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/suspected_cl_util.py:29: suspected_cl.approach |
approach if suspected_cl.approach and
why "suspected_cl.approach | approach"? Bit operation?

[chanli, 2016-09-16 21:24:19]

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:605: class _CLInfo(object):
On 2016/09/12 22:55:11, lijeffrey wrote:
> So how about making this depend on your other cl
> https://codereview.chromium.org/2320153003/ and make this a BaseCL object and
> get rid of _CLInfo?

Based on our discussion offline, I'll leave it as is for now.

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:615: def
not_none_failures(self):
On 2016/09/15 17:35:12, stgao (slow) wrote:
> What does this mean?

For compile and other non-swarming steps, we will have None in the list, I just
remove those Nones from the list.

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:630:
cl_failure_map[cl_key].top_score = top_score
On 2016/09/15 17:35:12, stgao (slow) wrote:
> What if the original score is higher than this one?

The real problem should be what if the original score is different from this
one... Since this should be a rare case and I don't want to handle it here, I
just override the value.

I can do 
cl_failure_map[cl_key].top_score = cl_failure_map[cl_key].top_score or top_score

if you think it should be more reasonable.

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:762: _SaveFailureToMap(
On 2016/09/15 17:35:12, stgao (slow) wrote:
> I'm wondering if it would be possible and cleaner if we do a post-analysis
> process for saving the data into datastore?
> 
> What do you think?

I actually do datastore operations in a patch here. But I moved the part doing
datastore operations to identify_culprit_pipeline while leave the process part
here.

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
File appengine/findit/waterfall/suspected_cl_util.py (right):

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/suspected_cl_util.py:29: suspected_cl.approach |
approach if suspected_cl.approach and
On 2016/09/15 17:35:12, stgao (slow) wrote:
> why "suspected_cl.approach | approach"? Bit operation?

Yes.

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/suspected_cl_util.py:29: suspected_cl.approach |
approach if suspected_cl.approach and
On 2016/09/15 17:35:12, stgao (slow) wrote:
> This is not that readable, how about expanding it to if/else statements?

Done.

[chanli, 2016-09-16 22:53:33]

ptal

[Sharu Jiang, 2016-09-19 23:17:54]

https://codereview.chromium.org/2230103002/diff/140001/appengine/findit/water...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/2230103002/diff/140001/appengine/findit/water...
appengine/findit/waterfall/build_failure_analysis.py:650: """Analyze the given
failure signals, and figure out culprit CLs.
nit: Analyzes

https://codereview.chromium.org/2230103002/diff/140001/appengine/findit/water...
appengine/findit/waterfall/build_failure_analysis.py:659: A dict with the
following form:
We should update the returns.

https://codereview.chromium.org/2230103002/diff/140001/appengine/findit/water...
File appengine/findit/waterfall/suspected_cl_util.py (right):

https://codereview.chromium.org/2230103002/diff/140001/appengine/findit/water...
appengine/findit/waterfall/suspected_cl_util.py:17: def
_ModifyApproach(current_approach, new_approach):
Is it possible that new_approach is None? if not then how about:

if not current_approach:
  return new_approach

return analysis_approach_type.BOTH if current_approach != new_approach else
new_approach

[chanli, 2016-09-21 22:04:25]

https://codereview.chromium.org/2230103002/diff/140001/appengine/findit/water...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/2230103002/diff/140001/appengine/findit/water...
appengine/findit/waterfall/build_failure_analysis.py:650: """Analyze the given
failure signals, and figure out culprit CLs.
On 2016/09/19 23:17:54, sharu jiang wrote:
> nit: Analyzes

Done.

https://codereview.chromium.org/2230103002/diff/140001/appengine/findit/water...
appengine/findit/waterfall/build_failure_analysis.py:659: A dict with the
following form:
On 2016/09/19 23:17:54, sharu jiang wrote:
> We should update the returns.

Done.

https://codereview.chromium.org/2230103002/diff/140001/appengine/findit/water...
File appengine/findit/waterfall/suspected_cl_util.py (right):

https://codereview.chromium.org/2230103002/diff/140001/appengine/findit/water...
appengine/findit/waterfall/suspected_cl_util.py:17: def
_ModifyApproach(current_approach, new_approach):
On 2016/09/19 23:17:54, sharu jiang wrote:
> Is it possible that new_approach is None? if not then how about:
> 
> if not current_approach:
>   return new_approach
> 
> return analysis_approach_type.BOTH if current_approach != new_approach else
> new_approach

Done.

[Sharu Jiang, 2016-09-22 01:36:14]

lgtm

[stgao, 2016-09-23 18:30:21]

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:615: def
not_none_failures(self):
On 2016/09/16 21:24:19, chanli wrote:
> On 2016/09/15 17:35:12, stgao (slow) wrote:
> > What does this mean?
> 
> For compile and other non-swarming steps, we will have None in the list, I
just
> remove those Nones from the list.

Why we will have None in the list? Can we avoid that at the first place?

If this turns out to be a necessary function, should we add comments as the
functionality is not obvious?

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:630:
cl_failure_map[cl_key].top_score = top_score
On 2016/09/16 21:24:19, chanli wrote:
> On 2016/09/15 17:35:12, stgao (slow) wrote:
> > What if the original score is higher than this one?
> 
> The real problem should be what if the original score is different from this
> one... Since this should be a rare case and I don't want to handle it here, I
> just override the value.
Add a comment for that?

> 
> I can do 
> cl_failure_map[cl_key].top_score = cl_failure_map[cl_key].top_score or
top_score
> 
> if you think it should be more reasonable.
>

https://codereview.chromium.org/2230103002/diff/200001/appengine/findit/water...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/2230103002/diff/200001/appengine/findit/water...
appengine/findit/waterfall/build_failure_analysis.py:637: for cl_key, cl_object
in cl_failure_map.iteritems():
name nit: cl_object -> cl_info.

Matching the class name will improve readability.

https://codereview.chromium.org/2230103002/diff/200001/appengine/findit/water...
File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right):

https://codereview.chromium.org/2230103002/diff/200001/appengine/findit/water...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:62:
try_job_type: Type of the try_job, HEURISTIC or TRY_JOB.
analysis_approach? Or wrong comment here?

https://codereview.chromium.org/2230103002/diff/200001/appengine/findit/water...
File appengine/findit/waterfall/suspected_cl_util.py (right):

https://codereview.chromium.org/2230103002/diff/200001/appengine/findit/water...
appengine/findit/waterfall/suspected_cl_util.py:42:
suspected_cl.builds[build_key] = build
Inline it in #35?

https://codereview.chromium.org/2230103002/diff/200001/appengine/findit/water...
appengine/findit/waterfall/suspected_cl_util.py:45:
build['approaches'].append(approach)
What if the approach is already in the list?

[stgao, 2016-09-23 18:56:54]

https://codereview.chromium.org/2230103002/diff/200001/appengine/findit/water...
File appengine/findit/waterfall/suspected_cl_util.py (right):

https://codereview.chromium.org/2230103002/diff/200001/appengine/findit/water...
appengine/findit/waterfall/suspected_cl_util.py:33: build_key = '%s/%s/%d'
%(master_name, builder_name, build_number)
Can we extract this logic to some place to share with other code?

The encoding and decoding should be abstracted in one place, instead of
spreading in multiple place in the code.

[chanli, 2016-09-24 00:05:00]

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:615: def
not_none_failures(self):
On 2016/09/23 18:30:21, stgao (slow) wrote:
> On 2016/09/16 21:24:19, chanli wrote:
> > On 2016/09/15 17:35:12, stgao (slow) wrote:
> > > What does this mean?
> > 
> > For compile and other non-swarming steps, we will have None in the list, I
> just
> > remove those Nones from the list.
> 
> Why we will have None in the list? Can we avoid that at the first place?
> 
> If this turns out to be a necessary function, should we add comments as the
> functionality is not obvious?

Since I'm using defaultdict here, I need to make sure to include steps like
compile(just step_name but no test_name) to the dict, and I use none as the
test_name to ensure that.

https://codereview.chromium.org/2230103002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/build_failure_analysis.py:630:
cl_failure_map[cl_key].top_score = top_score
On 2016/09/23 18:30:21, stgao (slow) wrote:
> On 2016/09/16 21:24:19, chanli wrote:
> > On 2016/09/15 17:35:12, stgao (slow) wrote:
> > > What if the original score is higher than this one?
> > 
> > The real problem should be what if the original score is different from this
> > one... Since this should be a rare case and I don't want to handle it here,
I
> > just override the value.
> Add a comment for that?
> 
> > 
> > I can do 
> > cl_failure_map[cl_key].top_score = cl_failure_map[cl_key].top_score or
> top_score
> > 
> > if you think it should be more reasonable.
> > 
> 

Done.

https://codereview.chromium.org/2230103002/diff/200001/appengine/findit/water...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/2230103002/diff/200001/appengine/findit/water...
appengine/findit/waterfall/build_failure_analysis.py:637: for cl_key, cl_object
in cl_failure_map.iteritems():
On 2016/09/23 18:30:21, stgao (slow) wrote:
> name nit: cl_object -> cl_info.
> 
> Matching the class name will improve readability.

Done.

https://codereview.chromium.org/2230103002/diff/200001/appengine/findit/water...
File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right):

https://codereview.chromium.org/2230103002/diff/200001/appengine/findit/water...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:62:
try_job_type: Type of the try_job, HEURISTIC or TRY_JOB.
On 2016/09/23 18:30:21, stgao (slow) wrote:
> analysis_approach? Or wrong comment here?

Oops, done.

https://codereview.chromium.org/2230103002/diff/200001/appengine/findit/water...
File appengine/findit/waterfall/suspected_cl_util.py (right):

https://codereview.chromium.org/2230103002/diff/200001/appengine/findit/water...
appengine/findit/waterfall/suspected_cl_util.py:33: build_key = '%s/%s/%d'
%(master_name, builder_name, build_number)
On 2016/09/23 18:56:54, stgao (slow) wrote:
> Can we extract this logic to some place to share with other code?
> 
> The encoding and decoding should be abstracted in one place, instead of
> spreading in multiple place in the code.

Done.

https://codereview.chromium.org/2230103002/diff/200001/appengine/findit/water...
appengine/findit/waterfall/suspected_cl_util.py:42:
suspected_cl.builds[build_key] = build
On 2016/09/23 18:30:21, stgao (slow) wrote:
> Inline it in #35?

Done.

https://codereview.chromium.org/2230103002/diff/200001/appengine/findit/water...
appengine/findit/waterfall/suspected_cl_util.py:45:
build['approaches'].append(approach)
On 2016/09/23 18:30:21, stgao (slow) wrote:
> What if the approach is already in the list?

This is for a specific build, so logically the approach should not be in the
list (In case of re-run, we should remove all data for that build before
re-run). I can add the check but to me it's not necessary.

[stgao, 2016-09-28 00:13:27]

lgtm with nits.

https://codereview.chromium.org/2230103002/diff/260001/appengine/findit/handl...
File appengine/findit/handlers/test/build_failure_test.py (right):

https://codereview.chromium.org/2230103002/diff/260001/appengine/findit/handl...
appengine/findit/handlers/test/build_failure_test.py:126: build_key =
BaseBuildModel.CreateBuildId(
There is another new code for this in build_util.py
Why do we have the duplicate?

https://codereview.chromium.org/2230103002/diff/260001/appengine/findit/water...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/2230103002/diff/260001/appengine/findit/water...
appengine/findit/waterfall/build_failure_analysis.py:614: def
not_none_failures(self):
can we have a better name here too?

https://codereview.chromium.org/2230103002/diff/260001/appengine/findit/water...
appengine/findit/waterfall/build_failure_analysis.py:616: # to ensure we include
them in the dict, used None as placeholder.
Can we use a singleton object like below?

NO_TEST_PLACEHOLDER = object()

https://codereview.chromium.org/2230103002/diff/260001/appengine/findit/water...
File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right):

https://codereview.chromium.org/2230103002/diff/260001/appengine/findit/water...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:62:
try_job_type: failure type, COMPILE or TEST.
var name is inconsistent with comment.

https://codereview.chromium.org/2230103002/diff/260001/appengine/findit/water...
File appengine/findit/waterfall/suspected_cl_util.py (right):

https://codereview.chromium.org/2230103002/diff/260001/appengine/findit/water...
appengine/findit/waterfall/suspected_cl_util.py:11: def GetCLInfo(cl_info_str):
Can we have docstring for this?

[chanli, 2016-09-30 20:41:01]

https://codereview.chromium.org/2230103002/diff/260001/appengine/findit/handl...
File appengine/findit/handlers/test/build_failure_test.py (right):

https://codereview.chromium.org/2230103002/diff/260001/appengine/findit/handl...
appengine/findit/handlers/test/build_failure_test.py:126: build_key =
BaseBuildModel.CreateBuildId(
On 2016/09/28 00:13:27, stgao (slow) wrote:
> There is another new code for this in build_util.py
> Why do we have the duplicate?

I thought I deleted that part of code... Done

https://codereview.chromium.org/2230103002/diff/260001/appengine/findit/water...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/2230103002/diff/260001/appengine/findit/water...
appengine/findit/waterfall/build_failure_analysis.py:614: def
not_none_failures(self):
On 2016/09/28 00:13:27, stgao (slow) wrote:
> can we have a better name here too?

Removed it.

https://codereview.chromium.org/2230103002/diff/260001/appengine/findit/water...
appengine/findit/waterfall/build_failure_analysis.py:616: # to ensure we include
them in the dict, used None as placeholder.
On 2016/09/28 00:13:27, stgao (slow) wrote:
> Can we use a singleton object like below?
> 
> NO_TEST_PLACEHOLDER = object()

I don't understand your suggestion... Used another way.

https://codereview.chromium.org/2230103002/diff/260001/appengine/findit/water...
File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right):

https://codereview.chromium.org/2230103002/diff/260001/appengine/findit/water...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:62:
try_job_type: failure type, COMPILE or TEST.
On 2016/09/28 00:13:27, stgao (slow) wrote:
> var name is inconsistent with comment.

Done.

https://codereview.chromium.org/2230103002/diff/260001/appengine/findit/water...
File appengine/findit/waterfall/suspected_cl_util.py (right):

https://codereview.chromium.org/2230103002/diff/260001/appengine/findit/water...
appengine/findit/waterfall/suspected_cl_util.py:11: def GetCLInfo(cl_info_str):
On 2016/09/28 00:13:27, stgao (slow) wrote:
> Can we have docstring for this?

Done.

[chanli, 2016-10-05 20:09:09]

The CQ bit was checked by chanli@chromium.org

[chanli, 2016-10-05 20:09:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
lijeffrey@chromium.org, katesonia@chromium.org, stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/2230103002/#ps320001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-05 20:09:16]

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-10-05 20:23:35]

Description was changed from

==========
[Findit] Pipeline change to save suspected cls to data store.

BUG=636199
==========

to

==========
[Findit] Pipeline change to save suspected cls to data store.

BUG=636199

Committed:
https://chromium.googlesource.com/infra/infra/+/29add55a8e77c144218429a140a01...
==========

[commit-bot: I haz the power, 2016-10-05 20:23:36]

Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/infra/infra/+/29add55a8e77c144218429a140a01...