New issue
Advanced search Search tips

Issue 725190 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

When rebaseline-cl is executed before all try jobs finish, it will schedule duplicate try jobs

Project Member Reported by qyears...@chromium.org, May 22 2017

Issue description

Expected behavior:
webkit-patch rebaseline-cl should only trigger jobs when jobs there are no jobs started at all for the current CL/patchset for some builders. If some jobs are already started and just not finished yet, no new jobs should be started; the tool could instead just note that there are some pending jobs, and either abort or ask if the user wants to try to continue without all of the results ready.

Relevant code:
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py?l=70
 
Cc: jeffcarp@chromium.org
Note, I realized that the current behavior is that it will also trigger try jobs when a builder has a scheduled (but not started) job, which is incorrect.

Just uploaded refactoring change to clarify current behavior:
https://chromium-review.googlesource.com/c/512990/

Also, somewhat related: this morning I started to think more about how webkit-patch rebaseline-cl should behave in different situations with different try job states... https://docs.google.com/document/d/1667DqRpLE8CeXANmNZUNum21Sxm1ITroHq5-e5j-PIs/edit
Project Member

Comment 2 by bugdroid1@chromium.org, May 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c46f7e02cda139ad74ffd33893cda5105124f182

commit c46f7e02cda139ad74ffd33893cda5105124f182
Author: Quinten Yearsley <qyearsley@google.com>
Date: Tue May 23 23:47:11 2017

Refactor and clarify rebaseline_cl.py and unit test.

In this CL:
 - Extract method self.check_ok_to_run out of main execute method
 - Use the term "job" instead of "build" in names (seems more descriptive in this context)
 - Correct name "builders_with_pending_builds" -> "builders_with_scheduled_jobs",
 - Correct name "builders_with_no_results" -> "builders_with_no_jobs"
   and added TODO
 - Simplify and clarify example test names in the unit test.

The purpose of this CL is to clarify the current behavior of rebaseline-cl
in preparation for changes to be made for:

  crbug.com/725190  (Don't trigger duplicate try jobs)
  crbug.com/724635  (Prompt user when making decisions about filling in results)

Bug:  725190 
Change-Id: I50f956367c2f01507122ecd238cbb0307fc2716e
Reviewed-on: https://chromium-review.googlesource.com/512990
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Jeff Carpenter <jeffcarp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#474107}
[modify] https://crrev.com/c46f7e02cda139ad74ffd33893cda5105124f182/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py
[modify] https://crrev.com/c46f7e02cda139ad74ffd33893cda5105124f182/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl_unittest.py

Project Member

Comment 3 by bugdroid1@chromium.org, May 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fe19e14edbf3225b9c149d266e9f18b1a88cbb8f

commit fe19e14edbf3225b9c149d266e9f18b1a88cbb8f
Author: Quinten Yearsley <qyearsley@google.com>
Date: Wed May 24 21:41:04 2017

Fix "builders_with_no_jobs" to not return builders with scheduled jobs.

Bug:  725190 
Change-Id: I18662b68d39a61960620a81cb7585249a4f7d90d
Reviewed-on: https://chromium-review.googlesource.com/513476
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#474432}
[modify] https://crrev.com/fe19e14edbf3225b9c149d266e9f18b1a88cbb8f/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py
[modify] https://crrev.com/fe19e14edbf3225b9c149d266e9f18b1a88cbb8f/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl_unittest.py

Status: Fixed (was: Assigned)
I *believe* this is fixed now (let me know if you see it again).

Although when I tried it again, I notice that the message that is printed when you run "rebaseline-cl" twice in succession is not very clear:

There are existing pending builds for:
  android_blink_rel
  mac10.10_blink_rel
  mac10.11_blink_rel
  mac10.11_retina_blink_rel
Got 404 response from: ....
Got 404 response from: ....
Got 404 response from: ....
Got 404 response from: ....

Instead, it would be more helpful for it to print something like:
There are scheduled or started builds for:
  android_blink_rel
  linux_trusty_blink_rel
  mac10.10_blink_rel
  mac10.11_blink_rel
  mac10.11_retina_blink_rel
  mac10.12_blink_rel
  win7_blink_rel
  ...
Project Member

Comment 5 by bugdroid1@chromium.org, May 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4a21fd7fbad35d3105789fddd733b5e9a723ffaa

commit 4a21fd7fbad35d3105789fddd733b5e9a723ffaa
Author: Quinten Yearsley <qyearsley@google.com>
Date: Thu May 25 19:27:07 2017

rebaseline-cl: Rephrase log when printing scheduled builds.

Note: Later on, I think this should be changed so that it prints
scheduled builds (if any), started builds (if any), and completed
builds (if any) instead of just scheduled builds, which isn't
super helpful.

Bug:  725190 
Change-Id: Ic793d294b9e2f2bbaaa8fdbfd67d2343bce7f69a
Reviewed-on: https://chromium-review.googlesource.com/514510
Reviewed-by: Jeff Carpenter <jeffcarp@chromium.org>
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#474751}
[modify] https://crrev.com/4a21fd7fbad35d3105789fddd733b5e9a723ffaa/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py

Sign in to add a comment