diff --git a/src/applications/feed/worker/FeedPublisherWorker.php b/src/applications/feed/worker/FeedPublisherWorker.php --- a/src/applications/feed/worker/FeedPublisherWorker.php +++ b/src/applications/feed/worker/FeedPublisherWorker.php @@ -7,7 +7,7 @@ $uris = PhabricatorEnv::getEnvConfig('feed.http-hooks'); foreach ($uris as $uri) { - PhabricatorWorker::scheduleTask( + $this->queueTask( 'FeedPublisherHTTPWorker', array( 'key' => $story->getChronologicalKey(), @@ -27,7 +27,7 @@ if (!$worker->isEnabled()) { continue; } - PhabricatorWorker::scheduleTask( + $this->queueTask( get_class($worker), array( 'key' => $story->getChronologicalKey(), diff --git a/src/applications/harbormaster/worker/HarbormasterTargetWorker.php b/src/applications/harbormaster/worker/HarbormasterTargetWorker.php --- a/src/applications/harbormaster/worker/HarbormasterTargetWorker.php +++ b/src/applications/harbormaster/worker/HarbormasterTargetWorker.php @@ -8,7 +8,7 @@ public function getRequiredLeaseTime() { // This worker performs actual build work, which may involve a long wait // on external systems. - return 60 * 60 * 24; + return phutil_units('24 hours in seconds'); } private function loadBuildTarget() { diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php --- a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php @@ -5,7 +5,7 @@ public function getRequiredLeaseTime() { // Herald rules may take a long time to process. - return 4 * 60 * 60; + return phutil_units('4 hours in seconds'); } public function parseCommit( diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php --- a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php @@ -3,11 +3,6 @@ final class PhabricatorRepositoryCommitOwnersWorker extends PhabricatorRepositoryCommitParserWorker { - public function getRequiredLeaseTime() { - // Commit owner parser may take a while to process for huge commits. - return 60 * 60; - } - protected function parseCommit( PhabricatorRepository $repository, PhabricatorRepositoryCommit $commit) { @@ -18,7 +13,7 @@ PhabricatorRepositoryCommit::IMPORTED_OWNERS); if ($this->shouldQueueFollowupTasks()) { - PhabricatorWorker::scheduleTask( + $this->queueTask( 'PhabricatorRepositoryCommitHeraldWorker', array( 'commitID' => $commit->getID(), diff --git a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php --- a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php +++ b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php @@ -6,7 +6,7 @@ public function getRequiredLeaseTime() { // It can take a very long time to parse commits; some commits in the // Facebook repository affect many millions of paths. Acquire 24h leases. - return 60 * 60 * 24; + return phutil_units('24 hours in seconds'); } abstract protected function parseCommitChanges( @@ -98,7 +98,7 @@ PhabricatorOwnersPackagePathValidator::updateOwnersPackagePaths($commit); if ($this->shouldQueueFollowupTasks()) { - PhabricatorWorker::scheduleTask( + $this->queueTask( 'PhabricatorRepositoryCommitOwnersWorker', array( 'commitID' => $commit->getID(), diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php @@ -15,7 +15,7 @@ $this->updateCommitData($ref); if ($this->shouldQueueFollowupTasks()) { - PhabricatorWorker::scheduleTask( + $this->queueTask( 'PhabricatorRepositoryGitCommitChangeParserWorker', array( 'commitID' => $commit->getID(), diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryMercurialCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryMercurialCommitMessageParserWorker.php --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryMercurialCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryMercurialCommitMessageParserWorker.php @@ -15,7 +15,7 @@ $this->updateCommitData($ref); if ($this->shouldQueueFollowupTasks()) { - PhabricatorWorker::scheduleTask( + $this->queueTask( 'PhabricatorRepositoryMercurialCommitChangeParserWorker', array( 'commitID' => $commit->getID(), diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositorySvnCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositorySvnCommitMessageParserWorker.php --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositorySvnCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositorySvnCommitMessageParserWorker.php @@ -15,7 +15,7 @@ $this->updateCommitData($ref); if ($this->shouldQueueFollowupTasks()) { - PhabricatorWorker::scheduleTask( + $this->queueTask( 'PhabricatorRepositorySvnCommitChangeParserWorker', array( 'commitID' => $commit->getID(), diff --git a/src/infrastructure/daemon/workers/PhabricatorWorker.php b/src/infrastructure/daemon/workers/PhabricatorWorker.php --- a/src/infrastructure/daemon/workers/PhabricatorWorker.php +++ b/src/infrastructure/daemon/workers/PhabricatorWorker.php @@ -9,6 +9,7 @@ private $data; private static $runAllTasksInProcess = false; + private $queuedTasks = array(); /* -( Configuring Retries and Failures )----------------------------------- */ @@ -17,13 +18,13 @@ /** * Return the number of seconds this worker needs hold a lease on the task for * while it performs work. For most tasks you can leave this at `null`, which - * will give you a short default lease (currently 60 seconds). + * will give you a default lease (currently 2 hours). * * For tasks which may take a very long time to complete, you should return * an upper bound on the amount of time the task may require. * * @return int|null Number of seconds this task needs to remain leased for, - * or null for a default (currently 60 second) lease. + * or null for a default lease. * * @task config */ @@ -194,4 +195,29 @@ return $this; } + + /** + * Queue a task to be executed after this one suceeds. + * + * The followup task will be queued only if this task completes cleanly. + * + * @param string Task class to queue. + * @param array Data for the followup task. + * @return this + */ + protected function queueTask($class, array $data) { + $this->queuedTasks[] = array($class, $data); + return $this; + } + + + /** + * Get tasks queued as followups by @{method:queueTask}. + * + * @return list> Queued task specifications. + */ + public function getQueuedTasks() { + return $this->queuedTasks; + } + } 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 @@ -10,11 +10,17 @@ const PHASE_UNLEASED = 'unleased'; const PHASE_EXPIRED = 'expired'; - const DEFAULT_LEASE_DURATION = 60; // Seconds - private $ids; private $limit; + public static function getDefaultWaitBeforeRetry() { + return phutil_units('5 minutes in seconds'); + } + + public static function getDefaultLeaseDuration() { + return phutil_units('2 hours in seconds'); + } + public function withIDs(array $ids) { $this->ids = $ids; return $this; @@ -78,7 +84,7 @@ %Q', $task_table->getTableName(), $lease_ownership_name, - self::DEFAULT_LEASE_DURATION, + self::getDefaultLeaseDuration(), $this->buildUpdateWhereClause($conn_w, $phase, $rows)); $leased += $conn_w->getAffectedRows(); diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php --- a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php +++ b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php @@ -100,6 +100,7 @@ // to release the lease otherwise. $this->checkLease(); + $did_succeed = false; try { $worker = $this->getWorkerInstance(); @@ -126,6 +127,7 @@ $result = $this->archiveTask( PhabricatorWorkerArchiveTask::RESULT_SUCCESS, $duration); + $did_succeed = true; } catch (PhabricatorWorkerPermanentFailureException $ex) { $result = $this->archiveTask( PhabricatorWorkerArchiveTask::RESULT_FAILURE, @@ -139,7 +141,7 @@ $retry = $worker->getWaitBeforeRetry($this); $retry = coalesce( $retry, - PhabricatorWorkerLeaseQuery::DEFAULT_LEASE_DURATION); + PhabricatorWorkerLeaseQuery::getDefaultWaitBeforeRetry()); // NOTE: As a side effect, this saves the object. $this->setLeaseDuration($retry); @@ -147,6 +149,15 @@ $result = $this; } + // NOTE: If this throws, we don't want it to cause the task to fail again, + // so execute it out here and just let the exception escape. + if ($did_succeed) { + foreach ($worker->getQueuedTasks() as $task) { + list($class, $data) = $task; + PhabricatorWorker::scheduleTask($class, $data); + } + } + return $result; }