diff --git a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php --- a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php @@ -655,7 +655,12 @@ $epoch, $task_priority) { - $this->insertTask($repository, $commit, $task_priority); + $this->queueCommitImportTask( + $repository, + $commit->getID(), + $commit->getPHID(), + $task_priority, + $via = 'discovery'); // Update the repository summary table. queryfx( @@ -679,36 +684,6 @@ } } - private function insertTask( - PhabricatorRepository $repository, - PhabricatorRepositoryCommit $commit, - $task_priority, - $data = array()) { - - $vcs = $repository->getVersionControlSystem(); - switch ($vcs) { - case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: - $class = 'PhabricatorRepositoryGitCommitMessageParserWorker'; - break; - case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: - $class = 'PhabricatorRepositorySvnCommitMessageParserWorker'; - break; - case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: - $class = 'PhabricatorRepositoryMercurialCommitMessageParserWorker'; - break; - default: - throw new Exception(pht("Unknown repository type '%s'!", $vcs)); - } - - $data['commitID'] = $commit->getID(); - - $options = array( - 'priority' => $task_priority, - ); - - PhabricatorWorker::scheduleTask($class, $data, $options); - } - private function isInitialImport(array $refs) { $commit_count = count($refs); @@ -926,71 +901,4 @@ $data['epoch']); } - private function getImportTaskPriority( - PhabricatorRepository $repository, - array $refs) { - - // If the repository is importing for the first time, we schedule tasks - // at IMPORT priority, which is very low. Making progress on importing a - // new repository for the first time is less important than any other - // daemon task. - - // If the repository has finished importing and we're just catching up - // on recent commits, we usually schedule discovery at COMMIT priority, - // which is slightly below the default priority. - - // Note that followup tasks and triggered tasks (like those generated by - // Herald or Harbormaster) will queue at DEFAULT priority, so that each - // commit tends to fully import before we start the next one. This tends - // to give imports fairly predictable progress. See T11677 for some - // discussion. - - if ($repository->isImporting()) { - $this->log( - pht( - 'Importing %s commit(s) at low priority ("PRIORITY_IMPORT") '. - 'because this repository is still importing.', - phutil_count($refs))); - - return PhabricatorWorker::PRIORITY_IMPORT; - } - - // See T13369. If we've discovered a lot of commits at once, import them - // at lower priority. - - // This is mostly aimed at reducing the impact that synchronizing thousands - // of commits from a remote upstream has on other repositories. The queue - // is "mostly FIFO", so queueing a thousand commit imports can stall other - // repositories. - - // In a perfect world we'd probably give repositories round-robin queue - // priority, but we don't currently have the primitives for this and there - // isn't a strong case for building them. - - // Use "a whole lot of commits showed up at once" as a heuristic for - // detecting "someone synchronized an upstream", and import them at a lower - // priority to more closely approximate fair scheduling. - - if (count($refs) >= PhabricatorRepository::LOWPRI_THRESHOLD) { - $this->log( - pht( - 'Importing %s commit(s) at low priority ("PRIORITY_IMPORT") '. - 'because many commits were discovered at once.', - phutil_count($refs))); - - return PhabricatorWorker::PRIORITY_IMPORT; - } - - // Otherwise, import at normal priority. - - if ($refs) { - $this->log( - pht( - 'Importing %s commit(s) at normal priority ("PRIORITY_COMMIT").', - phutil_count($refs))); - } - - return PhabricatorWorker::PRIORITY_COMMIT; - } - } diff --git a/src/applications/repository/engine/PhabricatorRepositoryEngine.php b/src/applications/repository/engine/PhabricatorRepositoryEngine.php --- a/src/applications/repository/engine/PhabricatorRepositoryEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryEngine.php @@ -83,4 +83,114 @@ return $this; } + final protected function queueCommitImportTask( + PhabricatorRepository $repository, + $commit_id, + $commit_phid, + $task_priority, + $via = null) { + + $vcs = $repository->getVersionControlSystem(); + switch ($vcs) { + case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: + $class = 'PhabricatorRepositoryGitCommitMessageParserWorker'; + break; + case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: + $class = 'PhabricatorRepositorySvnCommitMessageParserWorker'; + break; + case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: + $class = 'PhabricatorRepositoryMercurialCommitMessageParserWorker'; + break; + default: + throw new Exception( + pht( + 'Unknown repository type "%s"!', + $vcs)); + } + + $data = array( + 'commitID' => $commit_id, + ); + + if ($via !== null) { + $data['via'] = $via; + } + + $options = array( + 'priority' => $task_priority, + 'objectPHID' => $commit_phid, + ); + + PhabricatorWorker::scheduleTask($class, $data, $options); + } + + final protected function getImportTaskPriority( + PhabricatorRepository $repository, + array $refs) { + assert_instances_of($refs, 'PhabricatorRepositoryCommitRef'); + + // If the repository is importing for the first time, we schedule tasks + // at IMPORT priority, which is very low. Making progress on importing a + // new repository for the first time is less important than any other + // daemon task. + + // If the repository has finished importing and we're just catching up + // on recent commits, we usually schedule discovery at COMMIT priority, + // which is slightly below the default priority. + + // Note that followup tasks and triggered tasks (like those generated by + // Herald or Harbormaster) will queue at DEFAULT priority, so that each + // commit tends to fully import before we start the next one. This tends + // to give imports fairly predictable progress. See T11677 for some + // discussion. + + if ($repository->isImporting()) { + $this->log( + pht( + 'Importing %s commit(s) at low priority ("PRIORITY_IMPORT") '. + 'because this repository is still importing.', + phutil_count($refs))); + + return PhabricatorWorker::PRIORITY_IMPORT; + } + + // See T13369. If we've discovered a lot of commits at once, import them + // at lower priority. + + // This is mostly aimed at reducing the impact that synchronizing thousands + // of commits from a remote upstream has on other repositories. The queue + // is "mostly FIFO", so queueing a thousand commit imports can stall other + // repositories. + + // In a perfect world we'd probably give repositories round-robin queue + // priority, but we don't currently have the primitives for this and there + // isn't a strong case for building them. + + // Use "a whole lot of commits showed up at once" as a heuristic for + // detecting "someone synchronized an upstream", and import them at a lower + // priority to more closely approximate fair scheduling. + + if (count($refs) >= PhabricatorRepository::LOWPRI_THRESHOLD) { + $this->log( + pht( + 'Importing %s commit(s) at low priority ("PRIORITY_IMPORT") '. + 'because many commits were discovered at once.', + phutil_count($refs))); + + return PhabricatorWorker::PRIORITY_IMPORT; + } + + // Otherwise, import at normal priority. + + if ($refs) { + $this->log( + pht( + 'Importing %s commit(s) at normal priority ("PRIORITY_COMMIT").', + phutil_count($refs))); + } + + return PhabricatorWorker::PRIORITY_COMMIT; + } + + } diff --git a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php --- a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php @@ -548,6 +548,22 @@ } } + $commit_refs = array(); + foreach ($identifiers as $identifier) { + + // See T13591. This construction is a bit ad-hoc, but the priority + // function currently only cares about the number of refs we have + // discovered, so we'll get the right result even without filling + // these records out in detail. + + $commit_refs[] = id(new PhabricatorRepositoryCommitRef()) + ->setIdentifier($identifier); + } + + $task_priority = $this->getImportTaskPriority( + $repository, + $commit_refs); + $permanent_flag = PhabricatorRepositoryCommit::IMPORTED_PERMANENT; $published_flag = PhabricatorRepositoryCommit::IMPORTED_PUBLISH; @@ -580,17 +596,12 @@ $import_status, $row['id']); - $data = array( - 'commitID' => $row['id'], - ); - - PhabricatorWorker::scheduleTask( - $class, - $data, - array( - 'priority' => PhabricatorWorker::PRIORITY_COMMIT, - 'objectPHID' => $row['phid'], - )); + $this->queueCommitImportTask( + $repository, + $row['id'], + $row['phid'], + $task_priority, + $via = 'ref'); } }