From de70a4ff267e9f996af9746168b562d3feb7a5eb Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Jan 2021 16:46:04 -0800 Subject: [PATCH] Improve consistency in use of "via", "objectPHID", and "containerPHID" parameters in repository workers Summary: Ref T13591. Improve how parameters are passed between commit worker tasks: - Always pass "via", to track where tasks came from. - Always provide "objectPHID" (with the commit PHID). - Always provide "containerPHID" (with the repository PHID). Test Plan: - Pushed a new commit. - Ran `bin/repository pull` + `bin/repository discover`, saw commit with all parameters. - Ran `bin/worker execute ...`, saw a Change worker and then a Publish worker with appropriate parameters. - Ran `bin/repository reparse ... --background`, saw workers queue with appropriate parameters. Maniphest Tasks: T13591 Differential Revision: https://secure.phabricator.com/D21532 --- .../engine/PhabricatorRepositoryEngine.php | 1 + ...torRepositoryManagementReparseWorkflow.php | 3 ++ ...habricatorRepositoryCommitParserWorker.php | 34 ++++++++++++++++++- .../PhabricatorChangeParserTestCase.php | 8 +++++ ...atorRepositoryCommitChangeParserWorker.php | 9 +---- ...torRepositoryCommitMessageParserWorker.php | 16 +-------- 6 files changed, 47 insertions(+), 24 deletions(-) diff --git a/src/applications/repository/engine/PhabricatorRepositoryEngine.php b/src/applications/repository/engine/PhabricatorRepositoryEngine.php index c1ad203f0b..4d89d03140 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryEngine.php @@ -119,6 +119,7 @@ abstract class PhabricatorRepositoryEngine extends Phobject { $options = array( 'priority' => $task_priority, 'objectPHID' => $commit_phid, + 'containerPHID' => $repository->getPHID(), ); PhabricatorWorker::scheduleTask($class, $data, $options); diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementReparseWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementReparseWorkflow.php index ebd6edff97..e9e1cd1d6c 100644 --- a/src/applications/repository/management/PhabricatorRepositoryManagementReparseWorkflow.php +++ b/src/applications/repository/management/PhabricatorRepositoryManagementReparseWorkflow.php @@ -249,6 +249,7 @@ final class PhabricatorRepositoryManagementReparseWorkflow $spec = array( 'commitID' => $commit->getID(), 'only' => !$importing, + 'via' => 'reparse', ); foreach ($classes as $class) { @@ -258,6 +259,8 @@ final class PhabricatorRepositoryManagementReparseWorkflow $spec, array( 'priority' => PhabricatorWorker::PRIORITY_IMPORT, + 'objectPHID' => $commit->getPHID(), + 'containerPHID' => $repository->getPHID(), )); } catch (PhabricatorWorkerPermanentFailureException $ex) { // See T13315. We expect some reparse steps to occasionally raise diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php index e1990eaec3..5fe8e03f75 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php @@ -51,10 +51,42 @@ abstract class PhabricatorRepositoryCommitParserWorker $this->parseCommit($repository, $this->commit); } - final protected function shouldQueueFollowupTasks() { + private function shouldQueueFollowupTasks() { return !idx($this->getTaskData(), 'only'); } + final protected function queueCommitTask($task_class) { + if (!$this->shouldQueueFollowupTasks()) { + return; + } + + $commit = $this->loadCommit(); + $repository = $commit->getRepository(); + + $data = array( + 'commitID' => $commit->getID(), + ); + + $task_data = $this->getTaskData(); + if (isset($task_data['via'])) { + $data['via'] = $task_data['via']; + } + + $options = array( + // We queue followup tasks at default priority so that the queue finishes + // work it has started before starting more work. If followups are queued + // at the same priority level, we do all message parses first, then all + // change parses, etc. This makes progress uneven. See T11677 for + // discussion. + 'priority' => parent::PRIORITY_DEFAULT, + + 'objectPHID' => $commit->getPHID(), + 'containerPHID' => $repository->getPHID(), + ); + + $this->queueTask($task_class, $data, $options); + } + protected function getImportStepFlag() { return null; } diff --git a/src/applications/repository/worker/__tests__/PhabricatorChangeParserTestCase.php b/src/applications/repository/worker/__tests__/PhabricatorChangeParserTestCase.php index 452d02ae40..c3dd6f3d60 100644 --- a/src/applications/repository/worker/__tests__/PhabricatorChangeParserTestCase.php +++ b/src/applications/repository/worker/__tests__/PhabricatorChangeParserTestCase.php @@ -396,6 +396,8 @@ final class PhabricatorChangeParserTestCase } public function testSubversionParser() { + $this->requireBinaryForTest('svn'); + $repository = $this->buildDiscoveredRepository('CHC'); $viewer = PhabricatorUser::getOmnipotentUser(); @@ -955,6 +957,8 @@ final class PhabricatorChangeParserTestCase } public function testSubversionPartialParser() { + $this->requireBinaryForTest('svn'); + $repository = $this->buildBareRepository('CHD'); $repository->setDetail('svn-subpath', 'trunk/'); @@ -1059,6 +1063,8 @@ final class PhabricatorChangeParserTestCase } public function testSubversionValidRootParser() { + $this->requireBinaryForTest('svn'); + // First, automatically configure the root correctly. $repository = $this->buildBareRepository('CHD'); id(new PhabricatorRepositoryPullEngine()) @@ -1104,6 +1110,8 @@ final class PhabricatorChangeParserTestCase } public function testSubversionForeignStubsParser() { + $this->requireBinaryForTest('svn'); + $repository = $this->buildBareRepository('CHE'); $repository->setDetail('svn-subpath', 'branch/'); diff --git a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php index 0e135cb66f..577f620dde 100644 --- a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php +++ b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php @@ -99,14 +99,7 @@ abstract class PhabricatorRepositoryCommitChangeParserWorker } protected function finishParse() { - $commit = $this->commit; - if ($this->shouldQueueFollowupTasks()) { - $this->queueTask( - 'PhabricatorRepositoryCommitPublishWorker', - array( - 'commitID' => $commit->getID(), - )); - } + $this->queueCommitTask('PhabricatorRepositoryCommitPublishWorker'); } private function writeCommitChanges( diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php index ea277a84c9..a0ea2d84b2 100644 --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php @@ -24,21 +24,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $this->updateCommitData($commit, $data); } - if ($this->shouldQueueFollowupTasks()) { - $this->queueTask( - $this->getFollowupTaskClass(), - array( - 'commitID' => $commit->getID(), - ), - array( - // We queue followup tasks at default priority so that the queue - // finishes work it has started before starting more work. If - // followups are queued at the same priority level, we do all - // message parses first, then all change parses, etc. This makes - // progress uneven. See T11677 for discussion. - 'priority' => PhabricatorWorker::PRIORITY_DEFAULT, - )); - } + $this->queueCommitTask($this->getFollowupTaskClass()); } final protected function updateCommitData(