Bug 35460 – EWS should test patches with r+

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Adam Barth]

Contributors are surprised when the EWS doesn't process patches that reviewed before the EWS bots have a chance to run.  We should just run the patch and make folks happy.

[Eric Seidel (no email)]

*** Bug 36302 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

See duplicate.  The limiting factor here is that it's difficult to get a list of r+'d patches from bugzilla w/o making lots of queries.  Right now we make one query to get all r? patches:
https://bugs.webkit.org/request.cgi

To get all r+ patches we would have to make a query to:
http://webkit.org/pending-commit
and then fetch each bug to see which patches on it cause it to be in the bug list.

That would be a lot more load on bugzilla.  Since currently every EWS bot makes its own set of queries.

[William Siegrist]

If you can easily demo what the load would be, I'll let you know if its going to slow down the server. I think the bug pages are pretty light, so this probably isn't a problem.  Ping me on IRC if/when you want to try this out.

[Eric Seidel (no email)]

Created attachment 68723 [details]
the fix, minus testing

[Eric Seidel (no email)]

Comment on attachment 68723 [details]
the fix, minus testing

I can write some unit tests for this, and then we can land it and try it.  I don't expect it will send will's servers up in smoke, but it will be much lighter load once we move the EWS bots to use the feeder queue.

[Darin Adler]

I think that looking at request.cgi and making a new CGI script based on it to give exactly the right list would be possible.

[Eric Seidel (no email)]

OK.  My current thoughts on design for the EWS feeder queue (which will be used to solve this).

1.  Add a page to queues.webkit.org, which will expose the last status message across all queues for a single patch. (machine readable.  The query probably has to be "all status messages matching this patch id, limited to the last 1000" and then we'll filter for just one of each.  I don't think GQL can do unique/group style queries).
2.  Add a page to queues.webkit.org which allows a bot (or a user) to submit any attachment id to all EWSes (using checkboxes in the form).  (Careful to limit it only to EWSes, but otherwise no error checking.)
3.  Have an EWS feeder pull down all r? patches (and at a less frequent interval, all r+ patches).
4.  The EWS feeder will submit each patch to EWSes after checking the last-status page.  only submitting it to EWSes which either have no status, or the last status is RETRY (this allows a user to submit a patch again after PASS/FAIL, but the bot to only submit when RETRY).
5.  Change /next-patch to remove the patch from the active-work-items, or add a new /patch-completed action to do so.
6. Remove the update-work-items code from the EWS bots.  "Submitting" is a non-destructive action.  Never to we write to the server the entire list of patches to process.  We only add with submit-to-ews and remove with /patch-completed.

[Eric Seidel (no email)]

Note that in the above proposal I'm not polling for r+ patches.  By making the clients stop spamming the server's ActiveWorkItem list with a full new list everytime, we will end up processing r+ patches long after they may have been r+'d.  We could add an optimization to remove patches from the list once their bug gest closed if we wanted to.

[Adam Barth]

> 1.  Add a page to queues.webkit.org, which will expose the last status message across all queues for a single patch. (machine readable.  The query probably has to be "all status messages matching this patch id, limited to the last 1000" and then we'll filter for just one of each.  I don't think GQL can do unique/group style queries).

This page isn't needed.

> 2.  Add a page to queues.webkit.org which allows a bot (or a user) to submit any attachment id to all EWSes (using checkboxes in the form).  (Careful to limit it only to EWSes, but otherwise no error checking.)

This requires the server to know the global list of all EWS.  It should just add the attachment to the datastore as "an attachment that once upon a time had an r?".

> 3.  Have an EWS feeder pull down all r? patches (and at a less frequent interval, all r+ patches).

There's no need to poll for the r+ patches.  If the patch is only up for review for 30 seconds, its ok if we miss it.

> 4.  The EWS feeder will submit each patch to EWSes after checking the last-status page.  only submitting it to EWSes which either have no status, or the last status is RETRY (this allows a user to submit a patch again after PASS/FAIL, but the bot to only submit when RETRY).

It doesn't need to check the last-status page.  It can cache things in memory to avoid spamming the server all the time.  The server can do the final dedup.

> 5.  Change /next-patch to remove the patch from the active-work-items, or add a new /patch-completed action to do so.

In the first phase, we shouldn't need to worry about changing the mechanism by which the EWS bots avoid processing the same patch again.  We can support multiple bots in a separate patch.

> 6. Remove the update-work-items code from the EWS bots.  "Submitting" is a non-destructive action.  Never to we write to the server the entire list of patches to process.  We only add with submit-to-ews and remove with /patch-completed.

Ok.  We want to make sure we can still generate the #n parts of the status bubbles, but it sounds like we'll have a way to do this from the other data.

[Eric Seidel (no email)]

(In reply to comment #9)

Thank you for the reply.  I was attempting to engage you in a discussion about this feature with my previous post and you took the bait!

So you're suggestion is that the server does the logic necessary to maintain the individual ActiveWorkItem lists.  Every time something submits a new attachment for EWS processing, the server adds to a (new) table of EWSWorkItems to process (the existing Attachment class has no table backing it), and then adds it to any ActiveWorkItem lists for the various queues (which they pull from already to do their processing).  We'd need some additional logic whenever a queue sends its /item-completed message to remove the EWSWorkItem if all EWS queues have processed it.

I'm not sure this is exactly the model you were proposing, but what I attempted to explain just now seems like it would work w/o the additional /last-status page I originally proposed.

[Eric Seidel (no email)]

One complication my original proposal was trying to solve was differentiating between when a user proposes we (re-)process a patch, vs. when the feeder queue submits a patch as "btw, I've seen this r? patch, you should look at it if you haven't already".

But I know realize that could be easily solved by either two different request types, or by a checkbox on the page.

When the server gets a submission from a user (a v2 feature), it needs to mark the EWSWorkItem as such, so that it knows to add the patch to the ActiveWorkItems, even if the Attachment object already shows status for that patch.

I think I have this straight in my head now.  I'll post a patch to solve this all this afternoon.

[Adam Barth]

> I think I have this straight in my head now.  I'll post a patch to solve this all this afternoon.

Ok.  I didn't understand your explanation, I think partially because you're using the word "table" to mean something other than a database table.  Maybe the patch itself will be clearer.

[Eric Seidel (no email)]

The first step is now posted on bug 47869.

After that lands, I just need to add a /patch-completed handler, and then do the webkit-patch parts of the change (which should be pretty small).

[Eric Seidel (no email)]

There were several other patches too, but I forgot to add them to the dependency chain.

Anyway, the final piece is ready now.  I'll upload a patch shortly.

[Eric Seidel (no email)]

Bah.  Uploading is blocked by bug 47940.

[Eric Seidel (no email)]

Created attachment 71251 [details]
manually created patch file

[Adam Barth]

Comment on attachment 71251 [details]
manually created patch file

ok.  Looks kind of obvious at the end, no?

[WebKit Commit Bot]

Comment on attachment 71251 [details]
manually created patch file

Rejecting patch 71251 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
Last 500 characters of output:
bKitTools/Scripts/webkitpy/tool/commands/queuestest.py", line 77, in assert_outputs
    expected_exception=exception)
  File "/Users/abarth/git/webkit-queue/WebKitTools/Scripts/webkitpy/common/system/outputcapture.py", line 65, in assert_outputs
    testcase.assertEqual(stderr_string, expected_stderr)
AssertionError: '' != 'MOCK: update_work_items: style-queue [103]\n'

----------------------------------------------------------------------
Ran 645 tests in 34.472s

FAILED (failures=8, errors=1)

Full output: http://queues.webkit.org/results/4541008

[Eric Seidel (no email)]

(In reply to comment #17)
> (From update of attachment 71251 [details])
> ok.  Looks kind of obvious at the end, no?

Yes, the patch doesn't do justice to all the underlying work. :)

[Eric Seidel (no email)]

Oh, I should also note for those of you playing along at home:

This does not mean that existing r+ patches will get fed to the EWS queues.  But any patch which is r? for more than 30 seconds will always get processed now. Previously as soon as you marked it r+ the queues would forget about it.

It is also now possible under this infrastructure to submit any patch for review.  r?, r+, r- or not.

It is also possible under this infrastructure to run more than one copy of any EWS bot (there is a bug about doing that for the win-ews which I should now dup to this one).

[Eric Seidel (no email)]

Bah.  Got committed as r70171 but git got confused.

[Eric Seidel (no email)]

Committed r70193: <http://trac.webkit.org/changeset/70193>

[Csaba Osztrogonác]

After this patch, all EWS got crazy:

Chromium Linux EWS - 187 pending
Qt EWS - 9 pending
Gtk EWS - 198 pending
Mac EWS - 114 pending
Win EWS - 216 pending
Chromium Mac EWS - 157 pending

The problem is that EWS bots try to test r+ -ed and landed 
patches and it causes conflict like this:

Patch 71537 from bug 48116:  20 minutes ago Error: Failed to run "[u'/home/webkit/WebKit-qt-ews/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 2  [results]

I think EWS shouldn't run when the bug is closed or resolved.

[Eric Seidel (no email)]

Yup. I'm aware of the issues. Sorry. I should have sent an announcement to Webkit-dev that the boys were sick. I wrote a patch or the closed/obsolete issue last night. (See bug 48093) ill have the bots healthy by Monday.