From b5f7e9eec60aea093a3430518f061610db7bdb7b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 24 Nov 2014 11:10:35 -0800
Subject: [PATCH] Reverse meaning of task priority column

Summary:
Ref T6615. Mixing ASC and DESC ordering on a multipart key makes it dramatically less effective (or perhaps totally ineffective).

Reverse the meaning of the `priority` column so it goes in the same direction as the `id` column (both ascending, lower values execute sooner).

Test Plan:
  - Queued 1.2M tasks with `bin/worker flood`.
  - Processed ~1 task/second with `bin/phd debug taskmaster` before patch.
  - Applied patch, took ~5 seconds for ~1.2M rows.
  - Processed ~100-200 tasks/second with `bin/phd debug taskmaster` after patch.
  - "Next in Queue" query on daemon page dropped from 1.5s to <1ms.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aklapper, 20after4, epriestley

Maniphest Tasks: T6615

Differential Revision: https://secure.phabricator.com/D10895
---
 .../sql/autopatches/20141123.taskpriority.1.sql      |  2 ++
 .../sql/autopatches/20141123.taskpriority.2.sql      |  2 ++
 .../daemon/workers/PhabricatorWorker.php             | 12 ++++++++----
 .../workers/__tests__/PhabricatorWorkerTestCase.php  | 10 +++++-----
 .../workers/query/PhabricatorWorkerLeaseQuery.php    |  4 ++--
 5 files changed, 19 insertions(+), 11 deletions(-)
 create mode 100644 resources/sql/autopatches/20141123.taskpriority.1.sql
 create mode 100644 resources/sql/autopatches/20141123.taskpriority.2.sql

diff --git a/resources/sql/autopatches/20141123.taskpriority.1.sql b/resources/sql/autopatches/20141123.taskpriority.1.sql
new file mode 100644
index 0000000000..b7790e711e
--- /dev/null
+++ b/resources/sql/autopatches/20141123.taskpriority.1.sql
@@ -0,0 +1,2 @@
+UPDATE {$NAMESPACE}_worker.worker_activetask
+  SET priority = 5000 - priority;
diff --git a/resources/sql/autopatches/20141123.taskpriority.2.sql b/resources/sql/autopatches/20141123.taskpriority.2.sql
new file mode 100644
index 0000000000..91b8979112
--- /dev/null
+++ b/resources/sql/autopatches/20141123.taskpriority.2.sql
@@ -0,0 +1,2 @@
+UPDATE {$NAMESPACE}_worker.worker_archivetask
+  SET priority = 5000 - priority;
diff --git a/src/infrastructure/daemon/workers/PhabricatorWorker.php b/src/infrastructure/daemon/workers/PhabricatorWorker.php
index 699afdfc50..6d3fcb52e9 100644
--- a/src/infrastructure/daemon/workers/PhabricatorWorker.php
+++ b/src/infrastructure/daemon/workers/PhabricatorWorker.php
@@ -9,10 +9,14 @@ abstract class PhabricatorWorker {
   private static $runAllTasksInProcess = false;
   private $queuedTasks = array();
 
-  const PRIORITY_ALERTS  = 4000;
-  const PRIORITY_DEFAULT = 3000;
-  const PRIORITY_BULK    = 2000;
-  const PRIORITY_IMPORT  = 1000;
+  // NOTE: Lower priority numbers execute first. The priority numbers have to
+  // have the same ordering that IDs do (lowest first) so MySQL can use a
+  // multipart key across both of them efficiently.
+
+  const PRIORITY_ALERTS  = 1000;
+  const PRIORITY_DEFAULT = 2000;
+  const PRIORITY_BULK    = 3000;
+  const PRIORITY_IMPORT  = 4000;
 
 
 /* -(  Configuring Retries and Failures  )----------------------------------- */
diff --git a/src/infrastructure/daemon/workers/__tests__/PhabricatorWorkerTestCase.php b/src/infrastructure/daemon/workers/__tests__/PhabricatorWorkerTestCase.php
index 659a83821e..c5c9b0959f 100644
--- a/src/infrastructure/daemon/workers/__tests__/PhabricatorWorkerTestCase.php
+++ b/src/infrastructure/daemon/workers/__tests__/PhabricatorWorkerTestCase.php
@@ -160,14 +160,14 @@ final class PhabricatorWorkerTestCase extends PhabricatorTestCase {
     $this->expectNextLease($task1);
   }
 
-  public function testLeasedIsHighestPriority() {
-    $task1 = $this->scheduleTask(array(), 1);
-    $task2 = $this->scheduleTask(array(), 1);
-    $task3 = $this->scheduleTask(array(), 2);
+  public function testLeasedIsLowestPriority() {
+    $task1 = $this->scheduleTask(array(), 2);
+    $task2 = $this->scheduleTask(array(), 2);
+    $task3 = $this->scheduleTask(array(), 1);
 
     $this->expectNextLease(
       $task3,
-      'Tasks with a higher priority should be scheduled first.');
+      'Tasks with a lower priority should be scheduled first.');
     $this->expectNextLease(
       $task1,
       'Tasks with the same priority should be FIFO.');
diff --git a/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php b/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php
index fff5e85fa2..ea8e74e1be 100644
--- a/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php
+++ b/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php
@@ -215,8 +215,8 @@ final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery {
     switch ($phase) {
       case self::PHASE_UNLEASED:
         // When selecting new tasks, we want to consume them in order of
-        // decreasing priority (and then FIFO).
-        return qsprintf($conn_w, 'ORDER BY priority DESC, id ASC');
+        // increasing priority (and then FIFO).
+        return qsprintf($conn_w, 'ORDER BY priority ASC, id ASC');
       case self::PHASE_EXPIRED:
         // When selecting failed tasks, we want to consume them in roughly
         // FIFO order of their failures, which is not necessarily their original