mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-26 00:32:42 +01:00
Lift logic for queueing commit import tasks into RepositoryEngine
Summary: Ref T13591. There are currently two pathways to queue an import task for a commit: via repository discovery, or via a ref becoming permanent. These pathways duplicate some logic and have behavioral differences: one does not set `objectPHID` properly, one does not set the priority correctly. Unify these pathways, make them both set `objectPHID`, and make them both use the same priority logic. Test Plan: - Discovered refs. - See later changes in this series for more complete test cases. Maniphest Tasks: T13591 Differential Revision: https://secure.phabricator.com/D21516
This commit is contained in:
epriestley
parent
6716d4f6ae
commit
3cb543ef8f
3 changed files with 138 additions and 109 deletions
|
|
@ -655,7 +655,12 @@ final class PhabricatorRepositoryDiscoveryEngine
|
|||
$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 @@ final class PhabricatorRepositoryDiscoveryEngine
|
|||
}
|
||||
}
|
||||
|
||||
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 @@ final class PhabricatorRepositoryDiscoveryEngine
|
|||
$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;
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
|||
|
|
@ -83,4 +83,114 @@ abstract class PhabricatorRepositoryEngine extends Phobject {
|
|||
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;
|
||||
}
|
||||
|
||||
|
||||
}
|
||||
|
|
|
|||
|
|
@ -548,6 +548,22 @@ final class PhabricatorRepositoryRefEngine
|
|||
}
|
||||
}
|
||||
|
||||
$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 @@ final class PhabricatorRepositoryRefEngine
|
|||
$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');
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue