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

appengine/findit/waterfall/send_notification_for_culprit_pipeline.py

Index: appengine/findit/waterfall/send_notification_for_culprit_pipeline.py
diff --git a/appengine/findit/waterfall/send_notification_for_culprit_pipeline.py b/appengine/findit/waterfall/send_notification_for_culprit_pipeline.py
index 5f40f4a3996ffae07e88c67d2ea99d88d7b3f3bc..06bb57a188769d9aa28d825fb1df010963a7adc8 100644
--- a/appengine/findit/waterfall/send_notification_for_culprit_pipeline.py
+++ b/appengine/findit/waterfall/send_notification_for_culprit_pipeline.py
@@ -14,6 +14,7 @@ from common.http_client_appengine import HttpClientAppengine as HttpClient
 from common.pipeline_wrapper import BasePipeline
 from common.rietveld import Rietveld
 from model import analysis_status as status
+from model.wf_analysis import WfAnalysis
 from model.wf_culprit import WfCulprit
 from waterfall import build_util
 from waterfall import waterfall_config
@@ -22,7 +23,7 @@ from waterfall import waterfall_config
 @ndb.transactional
 def _ShouldSendNotification(
     master_name, builder_name, build_number, repo_name, revision,
-    commit_position, build_num_threshold, time_limit_passed):
+    commit_position, criteria):
   """Returns True if a notification for the culprit should be sent."""
   culprit = (WfCulprit.Get(repo_name, revision) or
              WfCulprit.Create(repo_name, revision, commit_position))
@@ -32,14 +33,11 @@ def _ShouldSendNotification(
   culprit.builds.append([master_name, builder_name, build_number])
   # Send notification only when:
   # 1. It was not processed yet.
-  # 2. The culprit is for multiple failures in different builds to avoid false
-  #    positive due to flakiness.
-  # 3. It is not too late after the culprit was committed.
+  # 2. It is not too late after the culprit was committed.
   #    * Try-job takes too long to complete and the failure got fixed.
   #    * The whole analysis was rerun a long time after the failure occurred.
   should_send = not (culprit.cr_notification_processed or
-                     len(culprit.builds) < build_num_threshold or
-                     time_limit_passed)
+                     criteria['time_limit_passed'])

[lijeffrey, 2016/09/02 06:37:34]

nit: since criteria will eventually contain multiple fields, how about make a
separate function that processes criteria that returns a bool and just calls
that function here instead of referencing time_limit_passed directly?

[chanli, 2016/09/02 16:57:19]

Done.

   if should_send:
     culprit.cr_notification_status = status.RUNNING
   culprit.put()
@@ -98,8 +96,6 @@ class SendNotificationForCulpritPipeline(BasePipeline):
   def run(self, master_name, builder_name, build_number, repo_name, revision):
     action_settings = waterfall_config.GetActionSettings()
     # Set some impossible default values to prevent notification by default.
-    build_threshold = action_settings.get(
-        'cr_notification_build_threshold', 100000)
     latency_limit_minutes = action_settings.get(
         'cr_notification_latency_limit_minutes', 1)
 
@@ -110,9 +106,17 @@ class SendNotificationForCulpritPipeline(BasePipeline):
     time_limit_passed = _NotificationTimeLimitPassed(
         build_end_time, latency_limit_minutes)
 
+    # criteria are additional parameters that will help decide if a notification
+    #  should be sent.
+    # TODO (chanli): Add check for if confidence for the culprit is
+    # over threshold.
+    criteria = {

[lijeffrey, 2016/09/02 06:37:34]

nit: since criteria is a dict of additional parameters, why not name it
additional_parameters or additional-params? Also the first C should be
capitalized for this comment

[chanli, 2016/09/02 16:57:19]

Changed to additional_criteria

+        'time_limit_passed': time_limit_passed
+    }
+
     if not _ShouldSendNotification(
       master_name, builder_name, build_number, repo_name,
-      revision, commit_position, build_threshold, time_limit_passed):
+      revision, commit_position, criteria):
       return False
     return _SendNotificationForCulprit(
         repo_name, revision, commit_position, code_review_url)