| Summary: | EWS should test patches with r+ | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||
| Component: | Tools / Tests | Assignee: | Eric Seidel (no email) <eric> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abecsi, commit-queue, darin, ddkilzer, eric, ossy, wsiegrist | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Bug Depends on: | 47869, 47940, 47943, 48093, 48171, 48173 | ||||||||
| Bug Blocks: | 44292 | ||||||||
| Attachments: |
|
||||||||
|
Description
Adam Barth
2010-02-26 14:47:26 PST
*** Bug 36302 has been marked as a duplicate of this bug. *** 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. 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. Created attachment 68723 [details]
the fix, minus testing
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.
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. 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. 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. > 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. (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. 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. > 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.
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). 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. Bah. Uploading is blocked by bug 47940. Created attachment 71251 [details]
manually created patch file
Comment on attachment 71251 [details]
manually created patch file
ok. Looks kind of obvious at the end, no?
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 (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. :) 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). Bah. Got committed as r70171 but git got confused. Committed r70193: <http://trac.webkit.org/changeset/70193> 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. 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. |