Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2302223002:
[Findit] Change the criteria of sending notification to code review. (Closed)
Description[Findit] Change the criteria of sending notification to code review.
Notifies code review when:
1. Heuristic found a culprit for compile failure,
2. Both approaches found the same culprit.
BUG=643461
Committed: https://chromium.googlesource.com/infra/infra/+/e94935097903706240aa39a85f2fcbfd26193b20
Patch Set 1 #
Total comments: 8
Patch Set 2 : For heuristic found culprit for compile, we still need to wait. #Patch Set 3 : Address comments #
Total comments: 6
Patch Set 4 : Address comments. #Patch Set 5 : fix format nits #
Total comments: 22
Patch Set 6 : Still need the criterion where 2 try jobs found the same culprit. #Patch Set 7 : address comments #Patch Set 8 : . #
Total comments: 4
Patch Set 9 : . #
Total comments: 6
Patch Set 10 : . #
Total comments: 6
Patch Set 11 : . #
Messages
Total messages: 26 (6 generated)
chanli@chromium.org changed reviewers: + katesonia@chromium.org, lijeffrey@chromium.org, stgao@chromium.org
ptal
Description was changed from ========== [Findit] Change the criteria of sending notification to code review. Notifies code review when: 1. Heuristic found a culprit for compile failure, the notification will be sent before try job (if there is one) completes. 2. Bother approaches found the same culprit. BUG=643461 ========== to ========== [Findit] Change the criteria of sending notification to code review. Notifies code review when: 1. Heuristic found a culprit for compile failure, 2. Both approaches found the same culprit. BUG=643461 ==========
Made some change on top of the first patch. PTAL
https://codereview.chromium.org/2302223002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/identify_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_culprit_pipeline.py:71: def _NotifyCompileCulprits( nit: Since this is for compile, shouldn't there only be 1 culprit, thus this should be named _NotifyCompileCulprit? https://codereview.chromium.org/2302223002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:197: if ((culprit['repo_name'], culprit['revision']) in heuristic_cls): nit: it looks like there are some unnecessary parenthesis around this if https://codereview.chromium.org/2302223002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/send_notification_for_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:40: criteria['time_limit_passed']) 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? https://codereview.chromium.org/2302223002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:113: criteria = { 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
https://codereview.chromium.org/2302223002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/identify_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_culprit_pipeline.py:71: def _NotifyCompileCulprits( On 2016/09/02 06:37:34, lijeffrey wrote: > nit: Since this is for compile, shouldn't there only be 1 culprit, thus this > should be named _NotifyCompileCulprit? The changes on identify_culprit_pipeline has been removed https://codereview.chromium.org/2302223002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:197: if ((culprit['repo_name'], culprit['revision']) in heuristic_cls): On 2016/09/02 06:37:34, lijeffrey wrote: > nit: it looks like there are some unnecessary parenthesis around this if Done. https://codereview.chromium.org/2302223002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/send_notification_for_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:40: criteria['time_limit_passed']) On 2016/09/02 06:37:34, lijeffrey wrote: > 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? Done. https://codereview.chromium.org/2302223002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:113: criteria = { On 2016/09/02 06:37:34, lijeffrey wrote: > 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 Changed to additional_criteria
https://codereview.chromium.org/2302223002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:222: if culprits: If culprits mean try_job_cls, I think rename it to try_job_cls would be more clear. https://codereview.chromium.org/2302223002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:379: nit: delete one empty line. https://codereview.chromium.org/2302223002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/send_notification_for_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:24: return additional_criteria['time_limit_passed'] We can generalize it to: not_pass = True for criteria in additional_criteria.itervalues(): not_pass = not_pass and criteria something like that, so later if you add other criteria, you don't need to change code here.
https://codereview.chromium.org/2302223002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:222: if culprits: On 2016/09/02 18:00:22, sharu jiang wrote: > If culprits mean try_job_cls, I think rename it to try_job_cls would be more > clear. It is a convention to call a suspected_cl found by try job as culprit because try_job should be able to give us the culprit if no flakiness. I would not change it here since we need to change a lot of places if we want to make this change and keep everything in sync. https://codereview.chromium.org/2302223002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:379: On 2016/09/02 18:00:22, sharu jiang wrote: > nit: delete one empty line. Done. https://codereview.chromium.org/2302223002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/send_notification_for_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:24: return additional_criteria['time_limit_passed'] On 2016/09/02 18:00:22, sharu jiang wrote: > We can generalize it to: > not_pass = True > for criteria in additional_criteria.itervalues(): > not_pass = not_pass and criteria > > something like that, so later if you add other criteria, you don't need to > change code here. Done.
lgtm
https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... File appengine/findit/waterfall/send_notification_for_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:25: for criterion in additional_criteria.values(): Sorry, just realize =_= You just can do for ...: if not criterion: return False return True
lijeffrey@google.com changed reviewers: + lijeffrey@google.com
https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:189: for failure in analysis.result.get('failures', []): nit: add 1 empty line before this for loop https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:215: 'Failed to notify culprits which was found by both approaches.') nit: Shouldn't this be 'Failed to notify culprit which was found ...'? https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:225: for culprit in culprits.itervalues(): nit: # There is a try job result, so check if any of the culprits was also found by heuristic analysis. https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:376: # Store try-job results. nit: remove extra white space https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... File appengine/findit/waterfall/send_notification_for_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:26: passed = passed and criterion so this is assuming every criterion must have a value of True? If this is the intended design add a docstring to this function explaining that this is the expected format of additional_criteria https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:95: """Returns True if it is too late to send notification.""" is this docstring now out of date? I think i preferred the old naming/returning true if the time limit has passed, or rename this _WithinNotificationTimeLimit() or something https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:116: # Additional criteria that will help decide if a notification nit: Too many spaces
https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:215: 'Failed to notify culprits which was found by both approaches.') It seems this message only cover one case. https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:226: if (culprit['repo_name'], culprit['revision']) in heuristic_cls: 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. https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:367: updated_result_status = _GetResultAnalysisStatus(analysis, result) analysis is not defined yet? Any reason to move the original line down?
https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:189: for failure in analysis.result.get('failures', []): On 2016/09/03 00:31:53, lijeffrey1 wrote: > nit: add 1 empty line before this for loop Done. https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:215: 'Failed to notify culprits which was found by both approaches.') On 2016/09/03 00:41:34, stgao wrote: > It seems this message only cover one case. Just simplify it to 'Failed to notify culprit which was found' https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:225: for culprit in culprits.itervalues(): On 2016/09/03 00:31:53, lijeffrey1 wrote: > nit: # There is a try job result, so check if any of the culprits was also found > by heuristic analysis. Done. https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:226: if (culprit['repo_name'], culprit['revision']) in heuristic_cls: On 2016/09/03 00:41:34, stgao wrote: > 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. Done. https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:367: updated_result_status = _GetResultAnalysisStatus(analysis, result) On 2016/09/03 00:41:34, stgao wrote: > analysis is not defined yet? Any reason to move the original line down? I need to save suspected cls found by heuristic, etc. So move it out. https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:376: # Store try-job results. On 2016/09/03 00:31:53, lijeffrey1 wrote: > nit: remove extra white space Done. https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... File appengine/findit/waterfall/send_notification_for_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:25: for criterion in additional_criteria.values(): On 2016/09/02 20:42:51, sharu jiang wrote: > Sorry, just realize =_= > You just can do > > for ...: > if not criterion: > return False > > return True > Thought of a one liner to do this : ) https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:26: passed = passed and criterion On 2016/09/03 00:31:53, lijeffrey1 wrote: > so this is assuming every criterion must have a value of True? If this is the > intended design add a docstring to this function explaining that this is the > expected format of additional_criteria Done. https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:95: """Returns True if it is too late to send notification.""" On 2016/09/03 00:31:53, lijeffrey1 wrote: > is this docstring now out of date? I think i preferred the old naming/returning > true if the time limit has passed, or rename this _WithinNotificationTimeLimit() > or something Done. https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:116: # Additional criteria that will help decide if a notification On 2016/09/03 00:31:53, lijeffrey1 wrote: > nit: Too many spaces Done.
https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:367: updated_result_status = _GetResultAnalysisStatus(analysis, result) On 2016/09/03 01:27:02, chanli wrote: > On 2016/09/03 00:41:34, stgao wrote: > > analysis is not defined yet? Any reason to move the original line down? > > I need to save suspected cls found by heuristic, etc. So move it out. But where is `analysis` defined within this function? If we read outside a transaction, how can we ensure it is atomic update here? https://codereview.chromium.org/2302223002/diff/140001/appengine/findit/water... File appengine/findit/waterfall/send_notification_for_culprit_pipeline.py (left): https://codereview.chromium.org/2302223002/diff/140001/appengine/findit/water... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:35: # 2. The culprit is for multiple failures in different builds to avoid false Should we still keep this one? https://codereview.chromium.org/2302223002/diff/140001/appengine/findit/water... File appengine/findit/waterfall/send_notification_for_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/140001/appengine/findit/water... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:28: return False if False in additional_criteria.values() else True return all(additional_criteria.values())
https://codereview.chromium.org/2302223002/diff/140001/appengine/findit/water... File appengine/findit/waterfall/send_notification_for_culprit_pipeline.py (left): https://codereview.chromium.org/2302223002/diff/140001/appengine/findit/water... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:35: # 2. The culprit is for multiple failures in different builds to avoid false On 2016/09/03 06:14:24, stgao wrote: > Should we still keep this one? Done. https://codereview.chromium.org/2302223002/diff/140001/appengine/findit/water... File appengine/findit/waterfall/send_notification_for_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/140001/appengine/findit/water... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:28: return False if False in additional_criteria.values() else True On 2016/09/03 06:14:23, stgao wrote: > return all(additional_criteria.values()) Done.
lgtm https://codereview.chromium.org/2302223002/diff/160001/appengine/findit/water... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/160001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:213: master_name, builder_name, build_number, repo_name, revision, nit: 4 spaces instead of 2 https://codereview.chromium.org/2302223002/diff/160001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:218: 'Failed to notify culprit.') nit: Keep this on 1 line https://codereview.chromium.org/2302223002/diff/160001/appengine/findit/water... File appengine/findit/waterfall/send_notification_for_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/160001/appengine/findit/water... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:23: def _PassAdditionalCriteria(additional_criteria): nit: rename this _AdditionalCriteriaAllPassed(additional_criteria)
https://codereview.chromium.org/2302223002/diff/160001/appengine/findit/water... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/160001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:213: master_name, builder_name, build_number, repo_name, revision, On 2016/09/06 16:53:56, lijeffrey1 wrote: > nit: 4 spaces instead of 2 Done. https://codereview.chromium.org/2302223002/diff/160001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:218: 'Failed to notify culprit.') On 2016/09/06 16:53:56, lijeffrey1 wrote: > nit: Keep this on 1 line Done. https://codereview.chromium.org/2302223002/diff/160001/appengine/findit/water... File appengine/findit/waterfall/send_notification_for_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/160001/appengine/findit/water... appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:23: def _PassAdditionalCriteria(additional_criteria): On 2016/09/06 16:53:56, lijeffrey1 wrote: > nit: rename this _AdditionalCriteriaAllPassed(additional_criteria) Done.
lgtm with nits. https://codereview.chromium.org/2302223002/diff/180001/appengine/findit/water... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/180001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:230: send_notification_right_now = True Just do: flag = data in list https://codereview.chromium.org/2302223002/diff/180001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:236: # Need to check if the failure is compile and heuristic found a culprit. What's this comment for? https://codereview.chromium.org/2302223002/diff/180001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:385: analysis = WfAnalysis.Get(master_name, builder_name, build_number) nit: need a comment for this as discussed.
https://codereview.chromium.org/2302223002/diff/180001/appengine/findit/water... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/2302223002/diff/180001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:230: send_notification_right_now = True On 2016/09/08 21:20:46, stgao wrote: > Just do: > > flag = data in list Done. https://codereview.chromium.org/2302223002/diff/180001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:236: # Need to check if the failure is compile and heuristic found a culprit. On 2016/09/08 21:20:46, stgao wrote: > What's this comment for? To explain the special case where try job didn't find any suspected cls, but heuristic found a suspected_cl. The case where both approaches have found the same culprit have been covered above. https://codereview.chromium.org/2302223002/diff/180001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:385: analysis = WfAnalysis.Get(master_name, builder_name, build_number) On 2016/09/08 21:20:46, stgao wrote: > nit: need a comment for this as discussed. Done.
The CQ bit was checked by chanli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from katesonia@chromium.org, lijeffrey@google.com, stgao@chromium.org Link to the patchset: https://codereview.chromium.org/2302223002/#ps200001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Findit] Change the criteria of sending notification to code review. Notifies code review when: 1. Heuristic found a culprit for compile failure, 2. Both approaches found the same culprit. BUG=643461 ========== to ========== [Findit] Change the criteria of sending notification to code review. Notifies code review when: 1. Heuristic found a culprit for compile failure, 2. Both approaches found the same culprit. BUG=643461 Committed: https://chromium.googlesource.com/infra/infra/+/e94935097903706240aa39a85f2fc... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/infra/infra/+/e94935097903706240aa39a85f2fc... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
