Page MenuHomePhabricator
Files F89265

D7746.diff
All Users
Actions

D7746.diff
View Options

diff --git a/src/infrastructure/daemon/workers/__tests__/PhabricatorWorkerTestCase.php b/src/infrastructure/daemon/workers/__tests__/PhabricatorWorkerTestCase.php
--- a/src/infrastructure/daemon/workers/__tests__/PhabricatorWorkerTestCase.php
+++ b/src/infrastructure/daemon/workers/__tests__/PhabricatorWorkerTestCase.php
@@ -49,7 +49,6 @@
$this->expectNextLease($task1);
}
-
public function testExecuteTask() {
$task = $this->scheduleAndExecuteTask();
@@ -155,6 +154,24 @@
$this->assertEqual(true, ($task->getLeaseExpires() - time()) > 1000);
}
+ public function testLeasedIsOldestFirst() {
+ // Tasks which expired earlier should lease first, all else being equal.
+
+ $task1 = $this->scheduleTask();
+ $task2 = $this->scheduleTask();
+
+ $task1->setLeaseOwner('test');
+ $task1->setLeaseExpires(time() - 100000);
+ $task1->forceSaveWithoutLease();
+
+ $task2->setLeaseOwner('test');
+ $task2->setLeaseExpires(time() - 200000);
+ $task2->forceSaveWithoutLease();
+
+ $this->expectNextLease($task2);
+ $this->expectNextLease($task1);
+ }
+
private function expectNextLease($task) {
$leased = id(new PhabricatorWorkerLeaseQuery())
->setLimit(1)
diff --git a/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php b/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php
--- a/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php
+++ b/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php
@@ -60,7 +60,7 @@
'SELECT id, leaseOwner FROM %T %Q %Q %Q',
$task_table->getTableName(),
$this->buildWhereClause($conn_w, $phase),
- $this->buildOrderClause($conn_w),
+ $this->buildOrderClause($conn_w, $phase),
$this->buildLimitClause($conn_w, $limit - $leased));
// NOTE: Sometimes, we'll race with another worker and they'll grab
@@ -102,7 +102,7 @@
$task_table->getTableName(),
$taskdata_table->getTableName(),
$lease_ownership_name,
- $this->buildOrderClause($conn_w),
+ $this->buildOrderClause($conn_w, $phase),
$this->buildLimitClause($conn_w, $limit));
$tasks = $task_table->loadAllFromArray($data);
@@ -188,8 +188,26 @@
}
- private function buildOrderClause(AphrontDatabaseConnection $conn_w) {
- return qsprintf($conn_w, 'ORDER BY id ASC');
+ private function buildOrderClause(AphrontDatabaseConnection $conn_w, $phase) {
+ switch ($phase) {
+ case self::PHASE_UNLEASED:
+ // When selecting new tasks, we want to consume them in roughly
+ // FIFO order, so we order by the task ID.
+ return qsprintf($conn_w, 'ORDER BY 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
+ // queue order.
+
+ // Particularly, this is important for tasks which use soft failures to
+ // indicate that they are waiting on other tasks to complete: we need to
+ // push them to the end of the queue after they fail, at least on
+ // average, so we don't deadlock retrying the same blocked task over
+ // and over again.
+ return qsprintf($conn_w, 'ORDER BY leaseExpires ASC');
+ default:
+ throw new Exception(pht('Unknown phase "%s"!', $phase));
+ }
}
private function buildLimitClause(AphrontDatabaseConnection $conn_w, $limit) {

File Metadata

Mime Type
text/x-diff
Storage Engine
amazon-s3
Storage Format
Raw Data
Storage Handle
phabricator/yu/j4/my6knje37vsfkkga
Default Alt Text
D7746.diff (3 KB)

Event Timeline

Phabricator · Built by Phacility