[Findit] Change the criteria of sending notification to code review.

appengine/findit/waterfall/identify_try_job_culprit_pipeline.py

Index: appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
diff --git a/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py b/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
index 12be524fe7fe94d697e053b80b743b824dd5c3b3..c3e4f8b3f2076353699bbabec6e71d68ee7a3638 100644
--- a/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
+++ b/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
@@ -182,16 +182,56 @@ def _GetCulpritsForTestsFromResultsDict(blame_list, test_results):
   return culprit_map, list(failed_revisions)
 
 
-def _NotifyCulprits(master_name, builder_name, build_number, culprits):
-  """Sends notifications to the identified culprits."""
+def _GetSuspectedCLFoundByHeuristicForCompile(analysis):
+  """For compile failure, gets the suspected revision found by heuristic."""
+  if not analysis or not analysis.result:
+    return None
+  for failure in analysis.result.get('failures', []):

[lijeffrey1, 2016/09/03 00:31:53]

nit: add 1 empty line before this for loop

[chanli, 2016/09/03 01:27:02]

Done.

+    if (failure['step_name'].lower() == 'compile' and
+        len(failure['suspected_cls']) == 1):
+      # Based on confidence calculation, suspected_cl found by heuristic for
+      # compile is very likely to be the culprit.
+      # Since the current confidence calculation is for results with single
+      # suspected_cl, we might need to have the same regulation here.
+      return failure['suspected_cls'][0]
+  return None
+
+
+def _GetHeuristicSuspectedCLs(analysis):
+  """Gets revisions of suspected cls found by heuristic approach."""
+  if analysis and analysis.suspected_cls:
+    return [(cl['repo_name'], cl['revision']) for cl in analysis.suspected_cls]
+  return []
+
+
+def _StartSendNotificationPipeline(
+    master_name, builder_name, build_number, repo_name, revision):
   try:
-    for culprit in (culprits or {}).itervalues():
-      pipeline = SendNotificationForCulpritPipeline(
-          master_name, builder_name, build_number,
-          culprit['repo_name'], culprit['revision'])
-      pipeline.start()
+    pipeline = SendNotificationForCulpritPipeline(
+      master_name, builder_name, build_number, repo_name, revision)
+    pipeline.start()
   except Exception:  # pragma: no cover.
-    logging.exception('Failed to notify culprits.')
+    logging.exception(
+        'Failed to notify culprits which was found by both approaches.')

[lijeffrey1, 2016/09/03 00:31:53]

nit: Shouldn't this be 'Failed to notify culprit which was found ...'?

[stgao, 2016/09/03 00:41:34]

It seems this message only cover one case.

[chanli, 2016/09/03 01:27:01]

Just simplify it to 'Failed to notify culprit which was found'

+
+
+def _NotifyCulprits(master_name, builder_name, build_number, culprits,
+                    heuristic_cls, compile_suspected_cl):
+  """Sends notifications to the identified culprits."""
+
+  if culprits:
+    # There is try job result, we need to check if any of the culprit
+    # was also found by heuristic.
+    for culprit in culprits.itervalues():

[lijeffrey1, 2016/09/03 00:31:53]

nit: # There is a try job result, so check if any of the culprits was also found
by heuristic analysis.

[chanli, 2016/09/03 01:27:01]

Done.

+      if (culprit['repo_name'], culprit['revision']) in heuristic_cls:

[stgao, 2016/09/03 00:41:34]

If we don't try to send, for this culprit, there is no record.

As we also want to send if two build failures were due to the same CL, we still
have to try to send?
Scenario: compile failed just on Win64 and Linux(in two groups, so two triggered
try-jobs will identify the same CL), but heuristic doesn't identify the culprit.

[chanli, 2016/09/03 01:27:02]

Done.

+        _StartSendNotificationPipeline(
+            master_name, builder_name, build_number,
+            culprit['repo_name'], culprit['revision'])
+  elif compile_suspected_cl:
+    # Need to check if the failure is compile and heuristic found a culprit.
+    _StartSendNotificationPipeline(
+        master_name, builder_name, build_number,
+        compile_suspected_cl['repo_name'], compile_suspected_cl['revision'])
 
 
 class IdentifyTryJobCulpritPipeline(BasePipeline):
@@ -324,7 +364,6 @@ class IdentifyTryJobCulpritPipeline(BasePipeline):
 
       # Update analysis result and suspected CLs with results of this try job if
       # culprits were found.
-      analysis = WfAnalysis.Get(master_name, builder_name, build_number)
       updated_result_status = _GetResultAnalysisStatus(analysis, result)

[stgao, 2016/09/03 00:41:34]

analysis is not defined yet? Any reason to move the original line down?

[chanli, 2016/09/03 01:27:02]

I need to save suspected cls found by heuristic, etc. So move it out.

[stgao, 2016/09/03 06:14:23]

But where is `analysis` defined within this function? If we read outside a
transaction, how can we ensure it is atomic update here?

       updated_suspected_cls = _GetSuspectedCLs(analysis, result)
 
@@ -334,10 +373,18 @@ class IdentifyTryJobCulpritPipeline(BasePipeline):
         analysis.suspected_cls = updated_suspected_cls
         analysis.put()
 
-    # Store try-job results.
+      # Store try-job results.

[lijeffrey1, 2016/09/03 00:31:53]

nit: remove extra white space

[chanli, 2016/09/03 01:27:01]

Done.

     UpdateTryJobResult()
+
+    analysis = WfAnalysis.Get(master_name, builder_name, build_number)
+    heuristic_cls = _GetHeuristicSuspectedCLs(analysis)
+    compile_suspected_cl = (
+        _GetSuspectedCLFoundByHeuristicForCompile(analysis)
+        if try_job_type == failure_type.COMPILE else None)
+
     # Add try-job results to WfAnalysis.
     UpdateWfAnalysisWithTryJobResult()
 
-    _NotifyCulprits(master_name, builder_name, build_number, culprits)
-    return result.get('culprit') if result else None
+    _NotifyCulprits(master_name, builder_name, build_number, culprits,
+                    heuristic_cls, compile_suspected_cl)
+    return result.get('culprit') if result else None