update-work-items should never delete items
authorap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 2 Oct 2014 23:49:30 +0000 (23:49 +0000)
committerap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 2 Oct 2014 23:49:30 +0000 (23:49 +0000)
https://bugs.webkit.org/show_bug.cgi?id=137366

Reviewed by Ryosuke Niwa.

As we don't just replace the whole list any more, indicate which items are high
priority, and which are not. Hight priority ones will be prepended to the queue,
others will be appended.

This creates a bit of unfairness in that high priority item queue becomes a LIFO.
But hopefully we will never have many rollouts competing like that.

* QueueStatusServer/app.yaml: Update version.

* QueueStatusServer/handlers/updateworkitems.py: Never remove items. Pass high
priority and regular items separately. Removed some error handling that used to
end up in returning status 500 - an uncaught exception does that automatically.

* QueueStatusServer/main.py: Removed unnecessary regexps from URL matching code.

* QueueStatusServer/model/workitems.py: Added a way to add high priority items.

* QueueStatusServer/templates/updateworkitems.html: Added a field for high
priority items.

* Scripts/webkitpy/common/net/bugzilla/bugzilla.py:
(BugzillaQueries._parse_attachment_ids_request_query): Removed an annoying log
line that complicated testing.

* Scripts/webkitpy/common/net/statusserver.py: Pass high priority items separately.

* Scripts/webkitpy/tool/bot/feeders.py: For commit queue, split high and regular
priority items into separate lists.

* Scripts/webkitpy/common/net/statusserver_mock.py:
* Scripts/webkitpy/tool/bot/feeders_unittest.py:
* Scripts/webkitpy/tool/commands/queues_unittest.py:
Updated tests.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@174248 268f45cc-cd09-0410-ab3c-d52691b4dbfc

12 files changed:
Tools/ChangeLog
Tools/QueueStatusServer/app.yaml
Tools/QueueStatusServer/handlers/updateworkitems.py
Tools/QueueStatusServer/main.py
Tools/QueueStatusServer/model/workitems.py
Tools/QueueStatusServer/templates/updateworkitems.html
Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py
Tools/Scripts/webkitpy/common/net/statusserver.py
Tools/Scripts/webkitpy/common/net/statusserver_mock.py
Tools/Scripts/webkitpy/tool/bot/feeders.py
Tools/Scripts/webkitpy/tool/bot/feeders_unittest.py
Tools/Scripts/webkitpy/tool/commands/queues_unittest.py

index e34fa88..7d65afe 100644 (file)
@@ -1,3 +1,44 @@
+2014-10-02  Alexey Proskuryakov  <ap@apple.com>
+
+        update-work-items should never delete items
+        https://bugs.webkit.org/show_bug.cgi?id=137366
+
+        Reviewed by Ryosuke Niwa.
+
+        As we don't just replace the whole list any more, indicate which items are high
+        priority, and which are not. Hight priority ones will be prepended to the queue,
+        others will be appended.
+
+        This creates a bit of unfairness in that high priority item queue becomes a LIFO.
+        But hopefully we will never have many rollouts competing like that.
+
+        * QueueStatusServer/app.yaml: Update version.
+
+        * QueueStatusServer/handlers/updateworkitems.py: Never remove items. Pass high
+        priority and regular items separately. Removed some error handling that used to
+        end up in returning status 500 - an uncaught exception does that automatically.
+
+        * QueueStatusServer/main.py: Removed unnecessary regexps from URL matching code.
+
+        * QueueStatusServer/model/workitems.py: Added a way to add high priority items.
+
+        * QueueStatusServer/templates/updateworkitems.html: Added a field for high
+        priority items.
+
+        * Scripts/webkitpy/common/net/bugzilla/bugzilla.py:
+        (BugzillaQueries._parse_attachment_ids_request_query): Removed an annoying log
+        line that complicated testing.
+
+        * Scripts/webkitpy/common/net/statusserver.py: Pass high priority items separately.
+
+        * Scripts/webkitpy/tool/bot/feeders.py: For commit queue, split high and regular
+        priority items into separate lists.
+
+        * Scripts/webkitpy/common/net/statusserver_mock.py:
+        * Scripts/webkitpy/tool/bot/feeders_unittest.py:
+        * Scripts/webkitpy/tool/commands/queues_unittest.py:
+        Updated tests.
+
 2014-10-02  Brendan Long  <b.long@cablelabs.com>
 
         Annoying build warnings in WTFString API tests
index 3771af5..b281261 100644 (file)
@@ -1,5 +1,5 @@
 application: webkit-queues
-version: 174158 # Bugzilla bug ID of last major change
+version: 174248 # Bugzilla bug ID of last major change
 runtime: python
 api_version: 1
 
index 6b3dded..ac3cec2 100644 (file)
@@ -1,4 +1,5 @@
 # Copyright (C) 2013 Google Inc. All rights reserved.
+# Copyright (C) 2014 Apple Inc. All rights reserved.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
@@ -42,21 +43,15 @@ class UpdateWorkItems(UpdateBase):
         self.response.out.write(template.render("templates/updateworkitems.html", None))
 
     def _parse_work_items_string(self, items_string):
-        try:
-            item_strings = items_string.split(" ") if items_string else []
-            return map(int, item_strings)
-        except ValueError:
-            return None
+        item_strings = items_string.split(" ") if items_string else []
+        return map(int, item_strings)
 
-    def _update_work_items_from_request(self, work_items):
+    def _work_items_from_request(self):
+        high_priority_items_string = self.request.get("high_priority_items")
         items_string = self.request.get("work_items")
-        new_work_items = self._parse_work_items_string(items_string)
-        if new_work_items == None:
-            self.response.out.write("Failed to parse work items: %s" % items_string)
-            return False
-        work_items.item_ids = new_work_items
-        work_items.date = datetime.utcnow()
-        return True
+        high_priority_work_items = self._parse_work_items_string(high_priority_items_string)
+        work_items = self._parse_work_items_string(items_string)
+        return high_priority_work_items, work_items
 
     def _queue_from_request(self):
         queue_name = self.request.get("queue_name")
@@ -71,17 +66,12 @@ class UpdateWorkItems(UpdateBase):
         if not queue:
             self.response.set_status(500)
             return
-        work_items = queue.work_items()
-        old_items = set(work_items.item_ids)
 
-        success = self._update_work_items_from_request(work_items)
-        if not success:
-            self.response.set_status(500)
-            return
-        new_items = set(work_items.item_ids)
-        work_items.put()
+        high_priority_items, items = self._work_items_from_request()
+
+        # Add items that are not currently in the work queue. Never remove any items,
+        # as that should be done by the queue, feeder only adds them.
+        added_items = queue.work_items().add_work_items(high_priority_items, items)
 
-        for work_item in new_items - old_items:
+        for work_item in added_items:
             RecordPatchEvent.added(work_item, queue.name())
-        for work_item in old_items - new_items:
-            RecordPatchEvent.stopped(work_item, queue.name())
index 2a7d318..2892819 100644 (file)
@@ -63,7 +63,7 @@ routes = [
     ('/sync-queue-logs', SyncQueueLogs),
     (r'/patch-status/(.*)/(.*)', PatchStatus),
     (r'/patch/(.*)', Patch),
-    (r'/submit-to-ews', SubmitToEWS),
+    ('/submit-to-ews', SubmitToEWS),
     (r'/results/(.*)', ShowResults),
     (r'/status-bubble/(.*)', StatusBubble),
     (r'/svn-revision/(.*)', SVNRevision),
@@ -73,8 +73,8 @@ routes = [
     (r'/queue-status/(.*)', QueueStatus),
     (r'/queue-status-json/(.*)', QueueStatusJSON),
     (r'/next-patch/(.*)', NextPatch),
-    (r'/release-patch', ReleasePatch),
-    (r'/release-lock', ReleaseLock),
+    ('/release-patch', ReleasePatch),
+    ('/release-lock', ReleaseLock),
     ('/update-status', UpdateStatus),
     ('/update-work-items', UpdateWorkItems),
     ('/update-svn-revision', UpdateSVNRevision),
index 81a2517..d306276 100644 (file)
@@ -1,4 +1,5 @@
 # Copyright (C) 2010 Google Inc. All rights reserved.
+# Copyright (C) 2014 Apple Inc. All rights reserved.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
@@ -53,16 +54,28 @@ class WorkItems(db.Model, QueuePropertyMixin):
         return None
 
     @staticmethod
-    def _unguarded_add(key, attachment_id):
+    def _unguarded_add(key, high_priority_items, items):
         work_items = db.get(key)
-        if attachment_id in work_items.item_ids:
-            return
-        work_items.item_ids.append(attachment_id)
+        added_items = []
+        for item in high_priority_items[::-1]:
+            if item in work_items.item_ids:
+                continue
+            work_items.item_ids.insert(0, item)
+            added_items.insert(0, item)
+        for item in items:
+            if item in work_items.item_ids:
+                continue
+            work_items.item_ids.append(item)
+            added_items.append(item)
         work_items.put()
+        return added_items
 
     # Because this uses .key() self.is_saved() must be True or this will throw NotSavedError.
     def add_work_item(self, attachment_id):
-        db.run_in_transaction(self._unguarded_add, self.key(), attachment_id)
+        db.run_in_transaction(self._unguarded_add, self.key(), [], [attachment_id])
+
+    def add_work_items(self, high_priority_items, items):
+        return db.run_in_transaction(self._unguarded_add, self.key(), high_priority_items, items)
 
     @staticmethod
     def _unguarded_remove(key, attachment_id):
index b086fc3..8e62f40 100644 (file)
@@ -1,8 +1,8 @@
 <form name="update_work_items" enctype="multipart/form-data" method="post">
 Update work items for a queue: <input name="queue_name">
  <div>
-     Work Items:
-    <input name="work_items">
+     Work items: <input name="work_items">
+     High priority work items: <input name="high_priority_work_items">
  </div>
  <div><input type="submit" value="Update Work Items"></div>
 </form>
index 7142bcf..0468321 100644 (file)
@@ -204,7 +204,6 @@ class BugzillaQueries(object):
             patch_id = int(digits.search(patch_tag["href"]).group(0))
             date_tag = row.find("td", text=date_format)
             if date_tag and datetime.strptime(date_format.search(date_tag).group(0), "%Y-%m-%d %H:%M") < since:
-                _log.info("Patch is old: %d (%s)" % (patch_id, date_tag))
                 continue
             patch_ids.append(patch_id)
         return patch_ids
index 55312d6..d4b79ff 100644 (file)
@@ -100,13 +100,15 @@ class StatusServer:
         self._browser["broken_bot"] = broken_bot
         return self._browser.submit().read()
 
-    def _post_work_items_to_server(self, queue_name, work_items):
+    def _post_work_items_to_server(self, queue_name, high_priority_work_items, work_items):
         update_work_items_url = "%s/update-work-items" % self.url
         self._browser.open(update_work_items_url)
         self._browser.select_form(name="update_work_items")
         self._browser["queue_name"] = queue_name
         work_items = map(unicode, work_items)  # .join expects strings
         self._browser["work_items"] = " ".join(work_items)
+        high_priority_work_items = map(unicode, high_priority_work_items)
+        self._browser["high_priority_work_items"] = " ".join(high_priority_work_items)
         return self._browser.submit().read()
 
     def _post_work_item_to_ews(self, attachment_id):
@@ -149,9 +151,9 @@ class StatusServer:
         _log.info("Releasing lock for work item %s from %s" % (patch.id(), queue_name))
         return NetworkTransaction(convert_404_to_None=True).run(lambda: self._post_release_lock(queue_name, patch))
 
-    def update_work_items(self, queue_name, work_items):
-        _log.debug("Recording work items: %s for %s" % (work_items, queue_name))
-        return NetworkTransaction().run(lambda: self._post_work_items_to_server(queue_name, work_items))
+    def update_work_items(self, queue_name, high_priority_work_items, work_items):
+        _log.debug("Recording work items: %s for %s" % (high_priority_work_items + work_items, queue_name))
+        return NetworkTransaction().run(lambda: self._post_work_items_to_server(queue_name, high_priority_work_items, work_items))
 
     def update_status(self, queue_name, status, patch=None, results_file=None):
         _log.info(status)
index c9ec6a6..0fdd0eb 100644 (file)
@@ -55,9 +55,9 @@ class MockStatusServer(object):
     def release_lock(self, queue_name, patch):
         _log.info("MOCK: release_lock: %s %s" % (queue_name, patch.id()))
 
-    def update_work_items(self, queue_name, work_items):
+    def update_work_items(self, queue_name, high_priority_work_items, work_items):
         self._work_items = work_items
-        _log.info("MOCK: update_work_items: %s %s" % (queue_name, work_items))
+        _log.info("MOCK: update_work_items: %s %s" % (queue_name, high_priority_work_items + work_items))
 
     def submit_to_ews(self, patch_id):
         _log.info("MOCK: submit_to_ews: %s" % (patch_id))
index 3a0f42e..7d673a2 100644 (file)
@@ -50,18 +50,16 @@ class CommitQueueFeeder(AbstractFeeder):
         AbstractFeeder.__init__(self, tool)
         self.committer_validator = CommitterValidator(self._tool)
 
-    def _update_work_items(self, item_ids):
-        # FIXME: This is the last use of update_work_items, the commit-queue
-        # should move to feeding patches one at a time like the EWS does.
-        self._tool.status_server.update_work_items(self.queue_name, item_ids)
-        _log.info("Feeding %s items %s" % (self.queue_name, item_ids))
-
     def feed(self):
         patches = self._validate_patches()
         patches = self._patches_with_acceptable_review_flag(patches)
         patches = sorted(patches, self._patch_cmp)
-        patch_ids = [patch.id() for patch in patches]
-        self._update_work_items(patch_ids)
+
+        high_priority_item_ids = [patch.id() for patch in patches if patch.is_rollout()]
+        item_ids = [patch.id() for patch in patches if not patch.is_rollout()]
+
+        _log.info("Feeding %s high priority items %s, regular items %s" % (self.queue_name, high_priority_item_ids, item_ids))
+        self._tool.status_server.update_work_items(self.queue_name, high_priority_item_ids, item_ids)
 
     def _patches_for_bug(self, bug_id):
         return self._tool.bugs.fetch_bug(bug_id).commit_queued_patches(include_invalid=True)
@@ -77,11 +75,6 @@ class CommitQueueFeeder(AbstractFeeder):
         return self.committer_validator.patches_after_rejecting_invalid_commiters_and_reviewers(all_patches)
 
     def _patch_cmp(self, a, b):
-        # Sort first by is_rollout, then by attach_date.
-        # Reversing the order so that is_rollout is first.
-        rollout_cmp = cmp(b.is_rollout(), a.is_rollout())
-        if rollout_cmp != 0:
-            return rollout_cmp
         return cmp(a.attach_date(), b.attach_date())
 
 
index 3bbe529..3520654 100644 (file)
@@ -37,6 +37,7 @@ from webkitpy.tool.mocktool import MockTool
 
 class FeedersTest(unittest.TestCase):
     def test_commit_queue_feeder(self):
+        self.maxDiff = None
         feeder = CommitQueueFeeder(MockTool())
         expected_logs = """Warning, attachment 10001 on bug 50000 has invalid committer (non-committer@example.com)
 Warning, attachment 10001 on bug 50000 has invalid committer (non-committer@example.com)
@@ -45,8 +46,8 @@ MOCK setting flag 'commit-queue' to '-' on attachment '10001' with comment 'Reje
 - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.
 
 - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.'
+Feeding commit-queue high priority items [10005], regular items [10000]
 MOCK: update_work_items: commit-queue [10005, 10000]
-Feeding commit-queue items [10005, 10000]
 """
         OutputCapture().assert_outputs(self, feeder.feed, expected_logs=expected_logs)
 
@@ -56,19 +57,6 @@ Feeding commit-queue items [10005, 10000]
         attachment.attach_date = lambda: attach_date
         return attachment
 
-    def test_patch_cmp(self):
-        long_ago_date = datetime(1900, 1, 21)
-        recent_date = datetime(2010, 1, 21)
-        attachment1 = self._mock_attachment(is_rollout=False, attach_date=recent_date)
-        attachment2 = self._mock_attachment(is_rollout=False, attach_date=long_ago_date)
-        attachment3 = self._mock_attachment(is_rollout=True, attach_date=recent_date)
-        attachment4 = self._mock_attachment(is_rollout=True, attach_date=long_ago_date)
-        attachments = [attachment1, attachment2, attachment3, attachment4]
-        expected_sort = [attachment4, attachment3, attachment2, attachment1]
-        queue = CommitQueueFeeder(MockTool())
-        attachments.sort(queue._patch_cmp)
-        self.assertEqual(attachments, expected_sort)
-
     def test_patches_with_acceptable_review_flag(self):
         class MockPatch(object):
             def __init__(self, patch_id, review):
index 8526d1a..fb0b69d 100644 (file)
@@ -128,6 +128,7 @@ class AbstractQueueTest(CommandsTest):
 
 class FeederQueueTest(QueuesTest):
     def test_feeder_queue(self):
+        self.maxDiff = None
         queue = TestFeederQueue()
         tool = MockTool(log_executive=True)
         expected_logs = {
@@ -139,8 +140,8 @@ MOCK setting flag 'commit-queue' to '-' on attachment '10001' with comment 'Reje
 - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.
 
 - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.'
+Feeding commit-queue high priority items [10005], regular items [10000]
 MOCK: update_work_items: commit-queue [10005, 10000]
-Feeding commit-queue items [10005, 10000]
 Feeding EWS (1 r? patch, 1 new)
 MOCK: submit_to_ews: 10002
 """,