1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-23 05:50:55 +01:00

Introduce storage patch "phases" to allow index-rebuilding patches to execute after worker queue schema changes

Summary:
Ref T13591. Some storage patches queue worker tasks, currently always to rebuild search indexes.

These patches can not execute in creation order if a later patch modifies the worker task table, since they'll try to perform a modern INSERT against an out-of-date table schema. Such a modification is desirable in the context of T13591, but making it causes these patches to fail.

Patches have an existing "after" mechanism which allows them to have explicit dependencies. This mechanism could be used to resolve this issue, but all patches with a dependency like this would need to be updated every time the queue table changes.

Instead, introduce "phases" to provide broader ordering rules. There are now two phases: "default" and "worker". Patches in the "worker" phase execute after patches in the "default" phase.

Phases may eventually be further separated, but

Test Plan:
  - Ran `bin/storage status`, saw patches annotated with phases.
  - Will apply `containerPHID` changes on top of this.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13591

Differential Revision: https://secure.phabricator.com/D21529
This commit is contained in:
epriestley 2021-01-28 10:28:23 -08:00
parent 6d5920fa2d
commit 32942f6232
8 changed files with 215 additions and 17 deletions

View file

@ -1,5 +1,7 @@
<?php <?php
// @phase worker
PhabricatorRebuildIndexesWorker::rebuildObjectsWithQuery( PhabricatorRebuildIndexesWorker::rebuildObjectsWithQuery(
'PhabricatorDashboardQuery'); 'PhabricatorDashboardQuery');

View file

@ -1,3 +1,5 @@
<?php <?php
// @phase worker
PhabricatorRebuildIndexesWorker::rebuildObjectsWithQuery('HeraldRuleQuery'); PhabricatorRebuildIndexesWorker::rebuildObjectsWithQuery('HeraldRuleQuery');

View file

@ -1,3 +1,5 @@
<?php <?php
// @phase worker
PhabricatorRebuildIndexesWorker::rebuildObjectsWithQuery('HeraldRuleQuery'); PhabricatorRebuildIndexesWorker::rebuildObjectsWithQuery('HeraldRuleQuery');

View file

@ -1,4 +1,6 @@
<?php <?php
// @phase worker
PhabricatorRebuildIndexesWorker::rebuildObjectsWithQuery( PhabricatorRebuildIndexesWorker::rebuildObjectsWithQuery(
'PhabricatorRepositoryQuery'); 'PhabricatorRepositoryQuery');

View file

@ -9,6 +9,10 @@ final class PhabricatorStoragePatch extends Phobject {
private $after; private $after;
private $legacy; private $legacy;
private $dead; private $dead;
private $phase;
const PHASE_DEFAULT = 'default';
const PHASE_WORKER = 'worker';
public function __construct(array $dict) { public function __construct(array $dict) {
$this->key = $dict['key']; $this->key = $dict['key'];
@ -18,6 +22,7 @@ final class PhabricatorStoragePatch extends Phobject {
$this->name = $dict['name']; $this->name = $dict['name'];
$this->after = $dict['after']; $this->after = $dict['after'];
$this->dead = $dict['dead']; $this->dead = $dict['dead'];
$this->phase = $dict['phase'];
} }
public function getLegacy() { public function getLegacy() {
@ -44,6 +49,10 @@ final class PhabricatorStoragePatch extends Phobject {
return $this->key; return $this->key;
} }
public function getPhase() {
return $this->phase;
}
public function isDead() { public function isDead() {
return $this->dead; return $this->dead;
} }
@ -52,4 +61,31 @@ final class PhabricatorStoragePatch extends Phobject {
return ($this->getType() == 'php'); return ($this->getType() == 'php');
} }
public static function getPhaseList() {
return array_keys(self::getPhaseMap());
}
public static function getDefaultPhase() {
return self::PHASE_DEFAULT;
}
private static function getPhaseMap() {
return array(
self::PHASE_DEFAULT => array(
'order' => 0,
),
self::PHASE_WORKER => array(
'order' => 1,
),
);
}
public function newSortVector() {
$map = self::getPhaseMap();
$phase = $this->getPhase();
return id(new PhutilSortVector())
->addInt($map[$phase]['order']);
}
} }

View file

@ -33,6 +33,7 @@ final class PhabricatorStorageManagementStatusWorkflow
$table = id(new PhutilConsoleTable()) $table = id(new PhutilConsoleTable())
->setShowHeader(false) ->setShowHeader(false)
->addColumn('id', array('title' => pht('ID'))) ->addColumn('id', array('title' => pht('ID')))
->addColumn('phase', array('title' => pht('Phase')))
->addColumn('host', array('title' => pht('Host'))) ->addColumn('host', array('title' => pht('Host')))
->addColumn('status', array('title' => pht('Status'))) ->addColumn('status', array('title' => pht('Status')))
->addColumn('duration', array('title' => pht('Duration'))) ->addColumn('duration', array('title' => pht('Duration')))
@ -49,12 +50,18 @@ final class PhabricatorStorageManagementStatusWorkflow
$duration = pht('%s us', new PhutilNumber($duration)); $duration = pht('%s us', new PhutilNumber($duration));
} }
$table->addRow(array( if (in_array($patch->getFullKey(), $applied)) {
$status = pht('Applied');
} else {
$status = pht('Not Applied');
}
$table->addRow(
array(
'id' => $patch->getFullKey(), 'id' => $patch->getFullKey(),
'phase' => $patch->getPhase(),
'host' => $ref->getRefKey(), 'host' => $ref->getRefKey(),
'status' => in_array($patch->getFullKey(), $applied) 'status' => $status,
? pht('Applied')
: pht('Not Applied'),
'duration' => $duration, 'duration' => $duration,
'type' => $patch->getType(), 'type' => $patch->getType(),
'name' => $patch->getName(), 'name' => $patch->getName(),

View file

@ -922,6 +922,10 @@ abstract class PhabricatorStorageManagementWorkflow
$patches = $this->patches; $patches = $this->patches;
$is_dryrun = $this->dryRun; $is_dryrun = $this->dryRun;
// We expect that patches should already be sorted properly. However,
// phase behavior will be wrong if they aren't, so make sure.
$patches = msortv($patches, 'newSortVector');
$api_map = array(); $api_map = array();
foreach ($apis as $api) { foreach ($apis as $api) {
$api_map[$api->getRef()->getRefKey()] = $api; $api_map[$api->getRef()->getRefKey()] = $api;

View file

@ -27,10 +27,20 @@ abstract class PhabricatorSQLPatchList extends Phobject {
$directory)); $directory));
} }
$patch_type = $matches[1];
$patch_full_path = rtrim($directory, '/').'/'.$patch;
$attributes = array();
if ($patch_type === 'php') {
$attributes = $this->getPHPPatchAttributes(
$patch,
$patch_full_path);
}
$patches[$patch] = array( $patches[$patch] = array(
'type' => $matches[1], 'type' => $patch_type,
'name' => rtrim($directory, '/').'/'.$patch, 'name' => $patch_full_path,
); ) + $attributes;
} }
return $patches; return $patches;
@ -45,8 +55,16 @@ abstract class PhabricatorSQLPatchList extends Phobject {
$specs = array(); $specs = array();
$seen_namespaces = array(); $seen_namespaces = array();
$phases = PhabricatorStoragePatch::getPhaseList();
$phases = array_fuse($phases);
$default_phase = PhabricatorStoragePatch::getDefaultPhase();
foreach ($patch_lists as $patch_list) { foreach ($patch_lists as $patch_list) {
$last_key = null; $last_keys = array_fill_keys(
array_keys($phases),
null);
foreach ($patch_list->getPatches() as $key => $patch) { foreach ($patch_list->getPatches() as $key => $patch) {
if (!is_array($patch)) { if (!is_array($patch)) {
throw new Exception( throw new Exception(
@ -63,6 +81,7 @@ abstract class PhabricatorSQLPatchList extends Phobject {
'after' => true, 'after' => true,
'legacy' => true, 'legacy' => true,
'dead' => true, 'dead' => true,
'phase' => true,
); );
foreach ($patch as $pkey => $pval) { foreach ($patch as $pkey => $pval) {
@ -128,8 +147,26 @@ abstract class PhabricatorSQLPatchList extends Phobject {
$patch['legacy'] = false; $patch['legacy'] = false;
} }
if (!array_key_exists('phase', $patch)) {
$patch['phase'] = $default_phase;
}
$patch_phase = $patch['phase'];
if (!isset($phases[$patch_phase])) {
throw new Exception(
pht(
'Storage patch "%s" specifies it should apply in phase "%s", '.
'but this phase is unrecognized. Valid phases are: %s.',
$full_key,
$patch_phase,
implode(', ', array_keys($phases))));
}
$last_key = $last_keys[$patch_phase];
if (!array_key_exists('after', $patch)) { if (!array_key_exists('after', $patch)) {
if ($last_key === null) { if ($last_key === null && $patch_phase === $default_phase) {
throw new Exception( throw new Exception(
pht( pht(
"Patch '%s' is missing key 'after', and is the first patch ". "Patch '%s' is missing key 'after', and is the first patch ".
@ -138,11 +175,15 @@ abstract class PhabricatorSQLPatchList extends Phobject {
"list the patch or patches it depends on explicitly.", "list the patch or patches it depends on explicitly.",
$full_key, $full_key,
get_class($patch_list))); get_class($patch_list)));
} else {
if ($last_key === null) {
$patch['after'] = array();
} else { } else {
$patch['after'] = array($last_key); $patch['after'] = array($last_key);
} }
} }
$last_key = $full_key; }
$last_keys[$patch_phase] = $full_key;
foreach ($patch['after'] as $after_key => $after) { foreach ($patch['after'] as $after_key => $after) {
if (strpos($after, ':') === false) { if (strpos($after, ':') === false) {
@ -186,6 +227,21 @@ abstract class PhabricatorSQLPatchList extends Phobject {
$key, $key,
$after)); $after));
} }
$patch_phase = $patch['phase'];
$after_phase = $specs[$after]['phase'];
if ($patch_phase !== $after_phase) {
throw new Exception(
pht(
'Storage patch "%s" executes in phase "%s", but depends on '.
'patch "%s" which is in a different phase ("%s"). Patches '.
'may not have dependencies across phases.',
$key,
$patch_phase,
$after,
$after_phase));
}
} }
} }
@ -196,7 +252,94 @@ abstract class PhabricatorSQLPatchList extends Phobject {
// TODO: Detect cycles? // TODO: Detect cycles?
$patches = msortv($patches, 'newSortVector');
return $patches; return $patches;
} }
private function getPHPPatchAttributes($patch_name, $full_path) {
$data = Filesystem::readFile($full_path);
$phase_list = PhabricatorStoragePatch::getPhaseList();
$phase_map = array_fuse($phase_list);
$attributes = array();
$lines = phutil_split_lines($data, false);
foreach ($lines as $line) {
// Skip over the "PHP" line.
if (preg_match('(^<\?)', $line)) {
continue;
}
// Skip over blank lines.
if (!strlen(trim($line))) {
continue;
}
// If this is a "//" comment...
if (preg_match('(^\s*//)', $line)) {
$matches = null;
if (preg_match('(^\s*//\s*@(\S+)(?:\s+(.*))?\z)', $line, $matches)) {
$attr_key = $matches[1];
$attr_value = trim(idx($matches, 2));
switch ($attr_key) {
case 'phase':
$phase_name = $attr_value;
if (!strlen($phase_name)) {
throw new Exception(
pht(
'Storage patch "%s" specifies a "@phase" attribute with '.
'no phase value. Phase attributes must specify a value, '.
'like "@phase default".',
$patch_name));
}
if (!isset($phase_map[$phase_name])) {
throw new Exception(
pht(
'Storage patch "%s" specifies a "@phase" value ("%s"), '.
'but this is not a recognized phase. Valid phases '.
'are: %s.',
$patch_name,
$phase_name,
implode(', ', $phase_list)));
}
if (isset($attributes['phase'])) {
throw new Exception(
pht(
'Storage patch "%s" specifies a "@phase" value ("%s"), '.
'but it already has a specified phase ("%s"). Patches '.
'may not specify multiple phases.',
$patch_name,
$phase_name,
$attributes['phase']));
}
$attributes[$attr_key] = $phase_name;
break;
default:
throw new Exception(
pht(
'Storage patch "%s" specifies attribute "%s", but this '.
'attribute is unknown.',
$patch_name,
$attr_key));
}
}
continue;
}
// If this is anything else, we're all done. Attributes must be marked
// in the header of the file.
break;
}
return $attributes;
}
} }