From ba4ebf28adef76465eb711364be10ef8fbf8bc43 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 30 Dec 2014 15:54:56 -0800 Subject: [PATCH] Allow archived tasks to be queried by object PHID and order by id Summary: Ref T5402. Test Plan: - Queried archived tasks. - Grepped for use sites and verified no other callsites are order-sensitive. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T5402 Differential Revision: https://secure.phabricator.com/D11089 --- .../PhabricatorDaemonConsoleController.php | 7 +- .../PhabricatorWorkerArchiveTaskQuery.php | 16 ++++- .../query/PhabricatorWorkerLeaseQuery.php | 68 +++++++++++++++++-- .../storage/PhabricatorWorkerActiveTask.php | 1 + .../storage/PhabricatorWorkerArchiveTask.php | 1 + 5 files changed, 83 insertions(+), 10 deletions(-) diff --git a/src/applications/daemon/controller/PhabricatorDaemonConsoleController.php b/src/applications/daemon/controller/PhabricatorDaemonConsoleController.php index 98fe5ecc87..38b7ed7c6e 100644 --- a/src/applications/daemon/controller/PhabricatorDaemonConsoleController.php +++ b/src/applications/daemon/controller/PhabricatorDaemonConsoleController.php @@ -131,8 +131,11 @@ final class PhabricatorDaemonConsoleController $daemon_panel->appendChild($daemon_table); - $tasks = id(new PhabricatorWorkerActiveTask())->loadAllWhere( - 'leaseOwner IS NOT NULL'); + $tasks = id(new PhabricatorWorkerLeaseQuery()) + ->setSkipLease(true) + ->withLeasedTasks(true) + ->setLimit(100) + ->execute(); $tasks_table = id(new PhabricatorDaemonTasksTableView()) ->setTasks($tasks) diff --git a/src/infrastructure/daemon/workers/query/PhabricatorWorkerArchiveTaskQuery.php b/src/infrastructure/daemon/workers/query/PhabricatorWorkerArchiveTaskQuery.php index f026e13456..93e829cc97 100644 --- a/src/infrastructure/daemon/workers/query/PhabricatorWorkerArchiveTaskQuery.php +++ b/src/infrastructure/daemon/workers/query/PhabricatorWorkerArchiveTaskQuery.php @@ -6,6 +6,7 @@ final class PhabricatorWorkerArchiveTaskQuery private $ids; private $dateModifiedSince; private $dateCreatedBefore; + private $objectPHIDs; private $limit; public function withIDs(array $ids) { @@ -23,20 +24,24 @@ final class PhabricatorWorkerArchiveTaskQuery return $this; } + public function withObjectPHIDs(array $phids) { + $this->objectPHIDs = $phids; + return $this; + } + public function setLimit($limit) { $this->limit = $limit; return $this; } public function execute() { - $task_table = new PhabricatorWorkerArchiveTask(); $conn_r = $task_table->establishConnection('r'); $rows = queryfx_all( $conn_r, - 'SELECT * FROM %T %Q %Q', + 'SELECT * FROM %T %Q ORDER BY id DESC %Q', $task_table->getTableName(), $this->buildWhereClause($conn_r), $this->buildLimitClause($conn_r)); @@ -54,6 +59,13 @@ final class PhabricatorWorkerArchiveTaskQuery $this->ids); } + if ($this->objectPHIDs !== null) { + $where[] = qsprintf( + $conn_r, + 'objectPHID IN (%Ls)', + $this->objectPHIDs); + } + if ($this->dateModifiedSince) { $where[] = qsprintf( $conn_r, diff --git a/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php b/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php index b4cded74e3..456a042776 100644 --- a/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php +++ b/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php @@ -5,6 +5,7 @@ */ final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery { + const PHASE_LEASED = 'leased'; const PHASE_UNLEASED = 'unleased'; const PHASE_EXPIRED = 'expired'; @@ -12,6 +13,7 @@ final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery { private $objectPHIDs; private $limit; private $skipLease; + private $leased = false; public static function getDefaultWaitBeforeRetry() { return phutil_units('5 minutes in seconds'); @@ -45,6 +47,26 @@ final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery { return $this; } + /** + * Select only leased tasks, only unleased tasks, or both types of task. + * + * By default, queries select only unleased tasks (equivalent to passing + * `false` to this method). You can pass `true` to select only leased tasks, + * or `null` to ignore the lease status of tasks. + * + * If your result set potentially includes leased tasks, you must disable + * leasing using @{method:setSkipLease}. These options are intended for use + * when displaying task status information. + * + * @param mixed `true` to select only leased tasks, `false` to select only + * unleased tasks (default), or `null` to select both. + * @return this + */ + public function withLeasedTasks($leased) { + $this->leased = $leased; + return $this; + } + public function setLimit($limit) { $this->limit = $limit; return $this; @@ -52,7 +74,17 @@ final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery { public function execute() { if (!$this->limit) { - throw new Exception('You must setLimit() when leasing tasks.'); + throw new Exception( + pht('You must setLimit() when leasing tasks.')); + } + + if ($this->leased !== false) { + if (!$this->skipLease) { + throw new Exception( + pht( + 'If you potentially select leased tasks using withLeasedTasks(), '. + 'you MUST disable lease acquisition by calling setSkipLease().')); + } } $task_table = new PhabricatorWorkerActiveTask(); @@ -65,10 +97,16 @@ final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery { // find enough tasks, try tasks with expired leases (i.e., tasks which have // previously failed). - $phases = array( - self::PHASE_UNLEASED, - self::PHASE_EXPIRED, - ); + // If we're selecting leased tasks, look for them first. + + $phases = array(); + if ($this->leased !== false) { + $phases[] = self::PHASE_LEASED; + } + if ($this->leased !== true) { + $phases[] = self::PHASE_UNLEASED; + $phases[] = self::PHASE_EXPIRED; + } $limit = $this->limit; $leased = 0; @@ -160,6 +198,11 @@ final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery { $tasks[$row['id']]->setData($task_data); } + if ($this->skipLease) { + // Reorder rows into the original phase order if this is a status query. + $tasks = array_select_keys($tasks, $task_ids); + } + return $tasks; } @@ -167,6 +210,10 @@ final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery { $where = array(); switch ($phase) { + case self::PHASE_LEASED: + $where[] = 'leaseOwner IS NOT NULL'; + $where[] = 'leaseExpires >= UNIX_TIMESTAMP()'; + break; case self::PHASE_UNLEASED: $where[] = 'leaseOwner IS NULL'; break; @@ -177,7 +224,7 @@ final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery { throw new Exception("Unknown phase '{$phase}'!"); } - if ($this->ids) { + if ($this->ids !== null) { $where[] = qsprintf($conn_w, 'id IN (%Ld)', $this->ids); } @@ -199,6 +246,11 @@ final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery { // `IN (NULL)` doesn't match NULL. switch ($phase) { + case self::PHASE_LEASED: + throw new Exception( + pht( + 'Trying to lease tasks selected in the leased phase! This is '. + 'intended to be imposssible.')); case self::PHASE_UNLEASED: $where[] = qsprintf($conn_w, 'leaseOwner IS NULL'); $where[] = qsprintf($conn_w, 'id IN (%Ld)', ipull($rows, 'id')); @@ -223,6 +275,10 @@ final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery { private function buildOrderClause(AphrontDatabaseConnection $conn_w, $phase) { switch ($phase) { + case self::PHASE_LEASED: + // Ideally we'd probably order these by lease acquisition time, but + // we don't have that handy and this is a good approximation. + return qsprintf($conn_w, 'ORDER BY priority ASC, id ASC'); case self::PHASE_UNLEASED: // When selecting new tasks, we want to consume them in order of // increasing priority (and then FIFO). diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php index a254b3eb23..1f108e1b1d 100644 --- a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php +++ b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php @@ -119,6 +119,7 @@ final class PhabricatorWorkerActiveTask extends PhabricatorWorkerTask { ->setFailureCount($this->getFailureCount()) ->setDataID($this->getDataID()) ->setPriority($this->getPriority()) + ->setObjectPHID($this->getObjectPHID()) ->setResult($result) ->setDuration($duration); diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerArchiveTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerArchiveTask.php index 3076b36b03..d28f13c8d0 100644 --- a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerArchiveTask.php +++ b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerArchiveTask.php @@ -84,6 +84,7 @@ final class PhabricatorWorkerArchiveTask extends PhabricatorWorkerTask { ->setFailureCount(0) ->setDataID($this->getDataID()) ->setPriority($this->getPriority()) + ->setObjectPHID($this->getObjectPHID()) ->insert(); $this->setDataID(null);