Get rid of Retry status in webkit-queues
authorap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 26 Sep 2014 16:58:41 +0000 (16:58 +0000)
committerap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 26 Sep 2014 16:58:41 +0000 (16:58 +0000)
https://bugs.webkit.org/show_bug.cgi?id=137135

Reviewed by Ryosuke Niwa.

* QueueStatusServer/config/messages.py: Removed Retry.

* QueueStatusServer/handlers/releasepatch.py: This is now straightforward, as it
no longer needs to check the latest status. It just always both unlocks the patch
and removes it from WorkItems.

* QueueStatusServer/handlers/submittoews.py: (SubmitToEWS._should_add_to_ews_queue):
I don't understand why we even needed to check for retries here, but now that there
are no retries, that code can go to /dev/null.

* QueueStatusServer/loggers/recordpatchevent.py:
(RecordPatchEvent.started):
(RecordPatchEvent.retrying): Deleted.
Fixed retry counting, it should work for all queues now.

* QueueStatusServer/model/queuestatus.py:
(QueueStatus.is_retry_request): Deleted. These are no more.

* Scripts/webkitpy/common/net/statusserver_mock.py:
(MockStatusServer.release_lock):
* Scripts/webkitpy/tool/commands/queues_unittest.py:
Did whatever it took to keep passing the tests. The particular test doesn't seem
quite right, but whatever.

* Scripts/webkitpy/tool/commands/queues.py:
(CommitQueue.process_work_item): Instead of posting a retry status, just unlock
and let others pick up. Also, added explicit returns for clarity.
(AbstractPatchQueue._did_retry): Deleted.

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

Tools/ChangeLog
Tools/QueueStatusServer/app.yaml
Tools/QueueStatusServer/config/messages.py
Tools/QueueStatusServer/handlers/releasepatch.py
Tools/QueueStatusServer/handlers/submittoews.py
Tools/QueueStatusServer/loggers/recordpatchevent.py
Tools/QueueStatusServer/model/queuestatus.py
Tools/Scripts/webkitpy/common/net/statusserver_mock.py
Tools/Scripts/webkitpy/tool/commands/queues.py
Tools/Scripts/webkitpy/tool/commands/queues_unittest.py

index 5d4e52f..0072176 100644 (file)
@@ -1,3 +1,39 @@
+2014-09-26  Alexey Proskuryakov  <ap@apple.com>
+
+        Get rid of Retry status in webkit-queues
+        https://bugs.webkit.org/show_bug.cgi?id=137135
+
+        Reviewed by Ryosuke Niwa.
+
+        * QueueStatusServer/config/messages.py: Removed Retry.
+
+        * QueueStatusServer/handlers/releasepatch.py: This is now straightforward, as it
+        no longer needs to check the latest status. It just always both unlocks the patch
+        and removes it from WorkItems.
+
+        * QueueStatusServer/handlers/submittoews.py: (SubmitToEWS._should_add_to_ews_queue):
+        I don't understand why we even needed to check for retries here, but now that there
+        are no retries, that code can go to /dev/null.
+
+        * QueueStatusServer/loggers/recordpatchevent.py:
+        (RecordPatchEvent.started):
+        (RecordPatchEvent.retrying): Deleted.
+        Fixed retry counting, it should work for all queues now.
+
+        * QueueStatusServer/model/queuestatus.py:
+        (QueueStatus.is_retry_request): Deleted. These are no more.
+
+        * Scripts/webkitpy/common/net/statusserver_mock.py:
+        (MockStatusServer.release_lock):
+        * Scripts/webkitpy/tool/commands/queues_unittest.py:
+        Did whatever it took to keep passing the tests. The particular test doesn't seem
+        quite right, but whatever.
+
+        * Scripts/webkitpy/tool/commands/queues.py:
+        (CommitQueue.process_work_item): Instead of posting a retry status, just unlock
+        and let others pick up. Also, added explicit returns for clarity.
+        (AbstractPatchQueue._did_retry): Deleted.
+
 2014-09-26  Csaba Osztrogon√°c  <ossy@webkit.org>
 
         [EFL] Fix the gst-libav build on ARM Thumb2
index a39cb4f..7b6f3dc 100644 (file)
@@ -1,5 +1,5 @@
 application: webkit-queues
-version: 173979 # Bugzilla bug ID of last major change
+version: 174009 # Bugzilla bug ID of last major change
 runtime: python
 api_version: 1
 
index 96547ab..6f95529 100644 (file)
@@ -29,5 +29,4 @@
 # These must be in sync with webkit-patch's AbstractQueue.
 pass_status = "Pass"
 fail_status = "Fail"
-retry_status = "Retry"
 error_status = "Error"
index 88f46b0..87d17c5 100644 (file)
@@ -54,13 +54,7 @@ class ReleasePatch(UpdateBase):
         # Ideally we should use a transaction for the calls to
         # WorkItems and ActiveWorkItems.
 
-        # Only remove it from the queue if the last message is not a retry request.
-        # Allow removing it from the queue even if there is no last_status for easier testing.
-        if not last_status or not last_status.is_retry_request():
-            queue.work_items().remove_work_item(attachment_id)
-            RecordPatchEvent.stopped(attachment_id, queue_name)
-        else:
-            RecordPatchEvent.retrying(attachment_id, queue_name)
+        queue.work_items().remove_work_item(attachment_id)
+        RecordPatchEvent.stopped(attachment_id, queue_name)
 
-        # Always release the lock on the item.
         queue.active_work_items().expire_item(attachment_id)
index f7669d2..5a79d0c 100644 (file)
@@ -45,14 +45,11 @@ class SubmitToEWS(UpdateBase):
         # in adding things to the commit-queue when they won't be processed by it.
         assert(queue.is_ews())
         latest_status = attachment.status_for_queue(queue)
-        if not latest_status:
-            return True
-        # Only ever re-submit to the EWS if the EWS specifically requested a retry.
-        # This allows us to restart the EWS feeder queue, without all r? patches
-        # being retried as a result of that restart!
-        # In some future version we might add a "force" button to allow the user
-        # to override this restriction.
-        return latest_status.is_retry_request()
+        # The feeder queue only submits each patch once normally, but it loses its memory
+        # when restarted. We do not want to re-add all patches if that happens.
+        # In the future we might add a "force" button to allow the user to retry patches
+        # that were previously submitted.
+        return not latest_status
 
     def _add_attachment_to_ews_queues(self, attachment):
         for queue in Queue.all_ews():  # all_ews() currently includes the style-queue
index 08c57cd..5974478 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
@@ -41,32 +42,25 @@ class RecordPatchEvent(object):
             queue_log.put()
 
     @classmethod
-    def retrying(cls, attachment_id, queue_name, bot_id=None):
+    def started(cls, attachment_id, queue_name, bot_id=None):
         patch_log = PatchLog.lookup_if_exists(attachment_id, queue_name)
         if not patch_log:
-            WarningLog.record("patchlog missing", "In retrying event.", attachment_id, queue_name, bot_id)
+            WarningLog.record("patchlog missing", "In started event.", attachment_id, queue_name, bot_id)
             return
 
         if bot_id:
             patch_log.bot_id = bot_id
-        patch_log.retry_count += 1
-        patch_log.put()
 
-        queue_log = QueueLog.get_current(queue_name, queue_log_duration)
-        queue_log.patch_retry_count += 1
-        queue_log.put()
-
-    @classmethod
-    def started(cls, attachment_id, queue_name, bot_id=None):
-        patch_log = PatchLog.lookup_if_exists(attachment_id, queue_name)
-        if not patch_log:
-            WarningLog.record("patchlog missing", "In started event.", attachment_id, queue_name, bot_id)
-            return
+        # An existing wait_duration implies the patch had been started previously and is
+        # being picked up again because it had expired, or was released.
+        if patch_log.wait_duration:
+            patch_log.retry_count += 1
+            patch_log.put()
 
-        # An existing wait_duration implies the patch had been started previously and is being picked up again because it had expired.
-        if not patch_log.wait_duration:
-            if bot_id:
-                patch_log.bot_id = bot_id
+            queue_log = QueueLog.get_current(queue_name, queue_log_duration)
+            queue_log.patch_retry_count += 1
+            queue_log.put()
+        else:
             patch_log.calculate_wait_duration()
             patch_log.put()
 
index 0f4d38b..d9bb6f6 100644 (file)
@@ -40,6 +40,3 @@ class QueueStatus(db.Model, QueuePropertyMixin):
     message = db.StringProperty(multiline=True)
     date = db.DateTimeProperty(auto_now_add=True)
     results_file = db.BlobProperty()
-
-    def is_retry_request(self):
-        return self.message == messages.retry_status
index 22fa12f..c9ec6a6 100644 (file)
@@ -52,6 +52,9 @@ class MockStatusServer(object):
     def release_work_item(self, queue_name, patch):
         _log.info("MOCK: release_work_item: %s %s" % (queue_name, patch.id()))
 
+    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):
         self._work_items = work_items
         _log.info("MOCK: update_work_items: %s %s" % (queue_name, work_items))
index 08780fa..b069ea6 100644 (file)
@@ -65,7 +65,6 @@ class AbstractQueue(Command, QueueEngineDelegate):
 
     _pass_status = "Pass"
     _fail_status = "Fail"
-    _retry_status = "Retry"
     _error_status = "Error"
 
     def __init__(self, options=None): # Default values should never be collections (like []) as default values are shared between invocations
@@ -239,10 +238,6 @@ class AbstractPatchQueue(AbstractQueue):
         self._update_status(self._fail_status, patch)
         self._release_work_item(patch)
 
-    def _did_retry(self, patch):
-        self._update_status(self._retry_status, patch)
-        self._release_work_item(patch)
-
     def _did_error(self, patch, reason):
         message = "%s: %s" % (self._error_status, reason)
         self._update_status(message, patch)
@@ -325,7 +320,8 @@ class CommitQueue(PatchProcessingQueue, StepSequenceErrorHandler, CommitQueueTas
             if task.run():
                 self._did_pass(patch)
                 return True
-            self._did_retry(patch)
+            self._unlock_patch(patch)
+            return False
         except ScriptError, e:
             validator = CommitterValidator(self._tool)
             validator.reject_patch_from_commit_queue(patch.id(), self._error_message_for_bug(task, patch, e))
@@ -333,6 +329,7 @@ class CommitQueue(PatchProcessingQueue, StepSequenceErrorHandler, CommitQueueTas
             if results_archive:
                 self._upload_results_archive_for_patch(patch, results_archive)
             self._did_fail(patch)
+            return False
 
     def _failing_tests_message(self, task, patch):
         results = task.results_from_patch_test_run(patch)
index 5303c99..8526d1a 100644 (file)
@@ -405,8 +405,7 @@ Running: webkit-patch --status-host=example.com build --no-clean --no-update --b
 MOCK: update_status: commit-queue Built patch
 Running: webkit-patch --status-host=example.com build-and-test --no-clean --no-update --test --non-interactive --port=mac
 MOCK: update_status: commit-queue Passed tests
-MOCK: update_status: commit-queue Retry
-MOCK: release_work_item: commit-queue 10000
+MOCK: release_lock: commit-queue 10000
 """
         self.maxDiff = None
         OutputCapture().assert_outputs(self, queue.process_work_item, [QueuesTest.mock_work_item], expected_logs=expected_logs)