From c092492a53e0e50df8019f765065ea4a9062f94a Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 12 Aug 2019 11:11:26 -0700 Subject: [PATCH 1/4] Fix missing display cell in daemon summary table Summary: Fixes T13374. The "Temporary Failures" row is missing a cell definiton from the addition of "Average Queue Time". Test Plan: Viewed "/daemon/" with some temporary failures and and odd number of rows above the "Temporary Failures" row. Saw cell properly zebra-striped. Maniphest Tasks: T13374 Differential Revision: https://secure.phabricator.com/D20710 --- .../daemon/controller/PhabricatorDaemonConsoleController.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/applications/daemon/controller/PhabricatorDaemonConsoleController.php b/src/applications/daemon/controller/PhabricatorDaemonConsoleController.php index 421008082f..8d9a9986e4 100644 --- a/src/applications/daemon/controller/PhabricatorDaemonConsoleController.php +++ b/src/applications/daemon/controller/PhabricatorDaemonConsoleController.php @@ -85,6 +85,7 @@ final class PhabricatorDaemonConsoleController phutil_tag('em', array(), pht('Temporary Failures')), count($failed), null, + null, ); } From 006cb659cbf1a0153548c6fc287535b678addb48 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 12 Aug 2019 11:19:08 -0700 Subject: [PATCH 2/4] Make the success message from "bin/config" more clear Summary: Ref T13373. When you "bin/config set x ..." a value, the success message ("Set x ...") is somewhat ambiguous and can be interpreted as "First, you need to set x..." rather than "Success, wrote x...". Make the messaging more explicit. Also make this string more translatable. Test Plan: Ran `bin/config set ...` with various combinations of flags, saw more clear messaging. Maniphest Tasks: T13373 Differential Revision: https://secure.phabricator.com/D20711 --- ...PhabricatorConfigManagementSetWorkflow.php | 44 ++++++++++++------- .../env/PhabricatorConfigLocalSource.php | 5 +++ 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php b/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php index 22b760872e..9eb83bd61e 100644 --- a/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php +++ b/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php @@ -30,11 +30,10 @@ final class PhabricatorConfigManagementSetWorkflow } public function execute(PhutilArgumentParser $args) { - $console = PhutilConsole::getConsole(); $argv = $args->getArg('args'); - if (count($argv) == 0) { + if (!$argv) { throw new PhutilArgumentUsageException( - pht('Specify a configuration key and a value to set it to.')); + pht('Specify the configuration key you want to set.')); } $is_stdin = $args->getArg('stdin'); @@ -45,7 +44,8 @@ final class PhabricatorConfigManagementSetWorkflow if (count($argv) > 1) { throw new PhutilArgumentUsageException( pht( - 'Too many arguments: expected only a key when using "--stdin".')); + 'Too many arguments: expected only a configuration key when '. + 'using "--stdin".')); } fprintf(STDERR, tsprintf("%s\n", pht('Reading value from stdin...'))); @@ -54,7 +54,8 @@ final class PhabricatorConfigManagementSetWorkflow if (count($argv) == 1) { throw new PhutilArgumentUsageException( pht( - "Specify a value to set the key '%s' to.", + 'Specify a value to set the configuration key "%s" to, or '. + 'use "--stdin" to read a value from stdin.', $key)); } @@ -67,14 +68,13 @@ final class PhabricatorConfigManagementSetWorkflow $value = $argv[1]; } - $options = PhabricatorApplicationConfigOptions::loadAllOptions(); if (empty($options[$key])) { throw new PhutilArgumentUsageException( pht( - "No such configuration key '%s'! Use `%s` to list all keys.", - $key, - 'config list')); + 'Configuration key "%s" is unknown. Use "bin/config list" to list '. + 'all known keys.', + $key)); } $option = $options[$key]; @@ -99,7 +99,7 @@ final class PhabricatorConfigManagementSetWorkflow switch ($type) { default: $message = pht( - 'Config key "%s" is of type "%s". Specify it in JSON.', + 'Configuration key "%s" is of type "%s". Specify it in JSON.', $key, $type); break; @@ -128,7 +128,6 @@ final class PhabricatorConfigManagementSetWorkflow } if ($use_database) { - $config_type = 'database'; $config_entry = PhabricatorConfigEntry::loadConfigEntry($key); $config_entry->setValue($value); @@ -136,15 +135,28 @@ final class PhabricatorConfigManagementSetWorkflow $config_entry->setIsDeleted(0); $config_entry->save(); + + $write_message = pht( + 'Wrote configuration key "%s" to database storage.', + $key); } else { - $config_type = 'local'; - id(new PhabricatorConfigLocalSource()) + $config_source = id(new PhabricatorConfigLocalSource()) ->setKeys(array($key => $value)); + + $local_path = $config_source->getReadablePath(); + + $write_message = pht( + 'Wrote configuration key "%s" to local storage (in file "%s").', + $key, + $local_path); } - $console->writeOut( - "%s\n", - pht("Set '%s' in %s configuration.", $key, $config_type)); + echo tsprintf( + "** %s ** %s\n", + pht('DONE'), + $write_message); + + return 0; } } diff --git a/src/infrastructure/env/PhabricatorConfigLocalSource.php b/src/infrastructure/env/PhabricatorConfigLocalSource.php index 16dc43a9bc..fc1c83f812 100644 --- a/src/infrastructure/env/PhabricatorConfigLocalSource.php +++ b/src/infrastructure/env/PhabricatorConfigLocalSource.php @@ -65,4 +65,9 @@ final class PhabricatorConfigLocalSource extends PhabricatorConfigProxySource { return $path; } + public function getReadablePath() { + $path = $this->getConfigPath(); + return Filesystem::readablePath($path); + } + } From 82cf97ad65a910cb55e9c53c3e80f0740a724a10 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 12 Aug 2019 12:07:13 -0700 Subject: [PATCH 3/4] When many commits are discovered at once, import them at lower priority Summary: Ref T13369. See that task for discussion. When the discovery daemon finds more than 64 commits to import, demote the worker queue priority of the resulting tasks. Test Plan: - Pushed one commit, ran `bin/repository discover --verbose --trace ...`, saw commit import with "at normal priority" message and priority 2500 ("PRIORITY_COMMIT"). - Pushed 3 commits, set threshold to 3, ran `bin/repository discover ...`, saw commist import with "at lower priority" message and priority 4000 ("PRIORITY_IMPORT"). Maniphest Tasks: T13369 Differential Revision: https://secure.phabricator.com/D20712 --- .../PhabricatorRepositoryDiscoveryEngine.php | 106 +++++++++++++----- .../storage/PhabricatorRepository.php | 2 + 2 files changed, 81 insertions(+), 27 deletions(-) diff --git a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php index c1bb9190b3..790b95d775 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php @@ -93,6 +93,8 @@ final class PhabricatorRepositoryDiscoveryEngine // Clear the working set cache. $this->workingSet = array(); + $task_priority = $this->getImportTaskPriority($repository, $refs); + // Record discovered commits and mark them in the cache. foreach ($refs as $ref) { $this->recordCommit( @@ -100,7 +102,8 @@ final class PhabricatorRepositoryDiscoveryEngine $ref->getIdentifier(), $ref->getEpoch(), $ref->getCanCloseImmediately(), - $ref->getParents()); + $ref->getParents(), + $task_priority); $this->commitCache[$ref->getIdentifier()] = true; } @@ -536,7 +539,8 @@ final class PhabricatorRepositoryDiscoveryEngine $commit_identifier, $epoch, $close_immediately, - array $parents) { + array $parents, + $task_priority) { $commit = new PhabricatorRepositoryCommit(); $conn_w = $repository->establishConnection('w'); @@ -559,7 +563,7 @@ final class PhabricatorRepositoryDiscoveryEngine $commit_identifier); // After reviving a commit, schedule new daemons for it. - $this->didDiscoverCommit($repository, $commit, $epoch); + $this->didDiscoverCommit($repository, $commit, $epoch, $task_priority); return; } @@ -620,7 +624,7 @@ final class PhabricatorRepositoryDiscoveryEngine } $commit->saveTransaction(); - $this->didDiscoverCommit($repository, $commit, $epoch); + $this->didDiscoverCommit($repository, $commit, $epoch, $task_priority); if ($this->repairMode) { // Normally, the query should throw a duplicate key exception. If we @@ -648,9 +652,10 @@ final class PhabricatorRepositoryDiscoveryEngine private function didDiscoverCommit( PhabricatorRepository $repository, PhabricatorRepositoryCommit $commit, - $epoch) { + $epoch, + $task_priority) { - $this->insertTask($repository, $commit); + $this->insertTask($repository, $commit, $task_priority); // Update the repository summary table. queryfx( @@ -677,6 +682,7 @@ final class PhabricatorRepositoryDiscoveryEngine private function insertTask( PhabricatorRepository $repository, PhabricatorRepositoryCommit $commit, + $task_priority, $data = array()) { $vcs = $repository->getVersionControlSystem(); @@ -696,27 +702,6 @@ final class PhabricatorRepositoryDiscoveryEngine $data['commitID'] = $commit->getID(); - // 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 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()) { - $task_priority = PhabricatorWorker::PRIORITY_IMPORT; - } else { - $task_priority = PhabricatorWorker::PRIORITY_COMMIT; - } - $options = array( 'priority' => $task_priority, ); @@ -934,4 +919,71 @@ 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; + } + } diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 30eb56cfd5..9661584851 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -34,6 +34,8 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO */ const IMPORT_THRESHOLD = 7; + const LOWPRI_THRESHOLD = 64; + const TABLE_PATH = 'repository_path'; const TABLE_PATHCHANGE = 'repository_pathchange'; const TABLE_FILESYSTEM = 'repository_filesystem'; From d890c03ac348b3f9ced3ca73077e5934f18b7a6e Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 15 Aug 2019 12:00:16 -0700 Subject: [PATCH 4/4] Namespace all column references in ProjectQuery to fix ambiguity with Ferret constraints Summary: Fixes T13378. If we join Ferret tables and page, we can end up with an ambiguous `id` column here. Explicitly refer to "project.x" in all cases that we're interacting with the project table. Test Plan: - Changed page size to 3. - Issued a Projects query for "~e", matching more than 3 results. - Clicked "Next Page". - Before: ambiguous id column fatal. - After: next page. Maniphest Tasks: T13378 Differential Revision: https://secure.phabricator.com/D20714 --- .../project/query/PhabricatorProjectQuery.php | 48 +++++++++---------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/src/applications/project/query/PhabricatorProjectQuery.php b/src/applications/project/query/PhabricatorProjectQuery.php index b08a58501f..b68c638363 100644 --- a/src/applications/project/query/PhabricatorProjectQuery.php +++ b/src/applications/project/query/PhabricatorProjectQuery.php @@ -464,28 +464,28 @@ final class PhabricatorProjectQuery } $where[] = qsprintf( $conn, - 'status IN (%Ld)', + 'project.status IN (%Ld)', $filter); } if ($this->statuses !== null) { $where[] = qsprintf( $conn, - 'status IN (%Ls)', + 'project.status IN (%Ls)', $this->statuses); } if ($this->ids !== null) { $where[] = qsprintf( $conn, - 'id IN (%Ld)', + 'project.id IN (%Ld)', $this->ids); } if ($this->phids !== null) { $where[] = qsprintf( $conn, - 'phid IN (%Ls)', + 'project.phid IN (%Ls)', $this->phids); } @@ -513,7 +513,7 @@ final class PhabricatorProjectQuery if ($this->names !== null) { $where[] = qsprintf( $conn, - 'name IN (%Ls)', + 'project.name IN (%Ls)', $this->names); } @@ -522,7 +522,7 @@ final class PhabricatorProjectQuery foreach ($this->namePrefixes as $name_prefix) { $parts[] = qsprintf( $conn, - 'name LIKE %>', + 'project.name LIKE %>', $name_prefix); } $where[] = qsprintf($conn, '%LO', $parts); @@ -531,21 +531,21 @@ final class PhabricatorProjectQuery if ($this->icons !== null) { $where[] = qsprintf( $conn, - 'icon IN (%Ls)', + 'project.icon IN (%Ls)', $this->icons); } if ($this->colors !== null) { $where[] = qsprintf( $conn, - 'color IN (%Ls)', + 'project.color IN (%Ls)', $this->colors); } if ($this->parentPHIDs !== null) { $where[] = qsprintf( $conn, - 'parentProjectPHID IN (%Ls)', + 'project.parentProjectPHID IN (%Ls)', $this->parentPHIDs); } @@ -563,7 +563,7 @@ final class PhabricatorProjectQuery foreach ($ancestor_paths as $ancestor_path) { $sql[] = qsprintf( $conn, - '(projectPath LIKE %> AND projectDepth > %d)', + '(project.projectPath LIKE %> AND project.projectDepth > %d)', $ancestor_path['projectPath'], $ancestor_path['projectDepth']); } @@ -572,18 +572,18 @@ final class PhabricatorProjectQuery $where[] = qsprintf( $conn, - 'parentProjectPHID IS NOT NULL'); + 'project.parentProjectPHID IS NOT NULL'); } if ($this->isMilestone !== null) { if ($this->isMilestone) { $where[] = qsprintf( $conn, - 'milestoneNumber IS NOT NULL'); + 'project.milestoneNumber IS NOT NULL'); } else { $where[] = qsprintf( $conn, - 'milestoneNumber IS NULL'); + 'project.milestoneNumber IS NULL'); } } @@ -591,42 +591,42 @@ final class PhabricatorProjectQuery if ($this->hasSubprojects !== null) { $where[] = qsprintf( $conn, - 'hasSubprojects = %d', + 'project.hasSubprojects = %d', (int)$this->hasSubprojects); } if ($this->minDepth !== null) { $where[] = qsprintf( $conn, - 'projectDepth >= %d', + 'project.projectDepth >= %d', $this->minDepth); } if ($this->maxDepth !== null) { $where[] = qsprintf( $conn, - 'projectDepth <= %d', + 'project.projectDepth <= %d', $this->maxDepth); } if ($this->minMilestoneNumber !== null) { $where[] = qsprintf( $conn, - 'milestoneNumber >= %d', + 'project.milestoneNumber >= %d', $this->minMilestoneNumber); } if ($this->maxMilestoneNumber !== null) { $where[] = qsprintf( $conn, - 'milestoneNumber <= %d', + 'project.milestoneNumber <= %d', $this->maxMilestoneNumber); } if ($this->subtypes !== null) { $where[] = qsprintf( $conn, - 'subtype IN (%Ls)', + 'project.subtype IN (%Ls)', $this->subtypes); } @@ -646,7 +646,7 @@ final class PhabricatorProjectQuery if ($this->memberPHIDs !== null) { $joins[] = qsprintf( $conn, - 'JOIN %T e ON e.src = p.phid AND e.type = %d', + 'JOIN %T e ON e.src = project.phid AND e.type = %d', PhabricatorEdgeConfig::TABLE_NAME_EDGE, PhabricatorProjectMaterializedMemberEdgeType::EDGECONST); } @@ -654,7 +654,7 @@ final class PhabricatorProjectQuery if ($this->watcherPHIDs !== null) { $joins[] = qsprintf( $conn, - 'JOIN %T w ON w.src = p.phid AND w.type = %d', + 'JOIN %T w ON w.src = project.phid AND w.type = %d', PhabricatorEdgeConfig::TABLE_NAME_EDGE, PhabricatorObjectHasWatcherEdgeType::EDGECONST); } @@ -662,7 +662,7 @@ final class PhabricatorProjectQuery if ($this->slugs !== null) { $joins[] = qsprintf( $conn, - 'JOIN %T slug on slug.projectPHID = p.phid', + 'JOIN %T slug on slug.projectPHID = project.phid', id(new PhabricatorProjectSlug())->getTableName()); } @@ -672,7 +672,7 @@ final class PhabricatorProjectQuery $token_table = 'token_'.$key; $joins[] = qsprintf( $conn, - 'JOIN %T %T ON %T.projectID = p.id AND %T.token LIKE %>', + 'JOIN %T %T ON %T.projectID = project.id AND %T.token LIKE %>', PhabricatorProject::TABLE_DATASOURCE_TOKEN, $token_table, $token_table, @@ -689,7 +689,7 @@ final class PhabricatorProjectQuery } protected function getPrimaryTableAlias() { - return 'p'; + return 'project'; } private function linkProjectGraph(array $projects, array $ancestors) {