Changeset View
Changeset View
Standalone View
Standalone View
src/applications/config/check/PhabricatorElasticSearchSetupCheck.php
<?php | <?php | ||||
final class PhabricatorElasticSearchSetupCheck extends PhabricatorSetupCheck { | final class PhabricatorElasticSearchSetupCheck extends PhabricatorSetupCheck { | ||||
public function getDefaultGroup() { | public function getDefaultGroup() { | ||||
return self::GROUP_OTHER; | return self::GROUP_OTHER; | ||||
} | } | ||||
protected function executeChecks() { | protected function executeChecks() { | ||||
if (!$this->shouldUseElasticSearchEngine()) { | $services = PhabricatorSearchService::getAllServices(); | ||||
epriestley: We don't need this or the corresponding method anymore, right? The loop has the same effect… | |||||
return; | |||||
} | |||||
$engine = new PhabricatorElasticFulltextStorageEngine(); | |||||
foreach ($services as $service) { | |||||
try { | |||||
$host = $service->getAnyHostForRole('read'); | |||||
} catch (PhabricatorClusterNoHostForRoleException $e) { | |||||
// ignore the error | |||||
continue; | |||||
} | |||||
if ($host instanceof PhabricatorElasticSearchHost) { | |||||
$index_exists = null; | $index_exists = null; | ||||
$index_sane = null; | $index_sane = null; | ||||
try { | try { | ||||
$engine = $host->getEngine(); | |||||
$index_exists = $engine->indexExists(); | $index_exists = $engine->indexExists(); | ||||
if ($index_exists) { | if ($index_exists) { | ||||
$index_sane = $engine->indexIsSane(); | $index_sane = $engine->indexIsSane(); | ||||
} | } | ||||
} catch (Exception $ex) { | } catch (Exception $ex) { | ||||
$summary = pht('Elasticsearch is not reachable as configured.'); | $summary = pht('Elasticsearch is not reachable as configured.'); | ||||
$message = pht( | $message = pht( | ||||
'Elasticsearch is configured (with the %s setting) but Phabricator '. | 'Elasticsearch is configured (with the %s setting) but Phabricator'. | ||||
'encountered an exception when trying to test the index.'. | ' encountered an exception when trying to test the index.'. | ||||
"\n\n". | "\n\n". | ||||
'%s', | '%s', | ||||
phutil_tag('tt', array(), 'search.elastic.host'), | phutil_tag('tt', array(), 'cluster.search'), | ||||
phutil_tag('pre', array(), $ex->getMessage())); | phutil_tag('pre', array(), $ex->getMessage())); | ||||
Done Inline Actions(Does this throw PhabricatorClusterNoHostForRoleException?) epriestley: (Does this throw `PhabricatorClusterNoHostForRoleException`?) | |||||
Done Inline ActionsYes, I added that exception at the last minute and forgot to check it here... Or confused myself somehow. I'll add exception handling. 20after4: Yes, I added that exception at the last minute and forgot to check it here... Or confused… | |||||
$this->newIssue('elastic.misconfigured') | $this->newIssue('elastic.misconfigured') | ||||
->setName(pht('Elasticsearch Misconfigured')) | ->setName(pht('Elasticsearch Misconfigured')) | ||||
->setSummary($summary) | ->setSummary($summary) | ||||
->setMessage($message) | ->setMessage($message) | ||||
->addRelatedPhabricatorConfig('search.elastic.host'); | ->addRelatedPhabricatorConfig('cluster.search'); | ||||
return; | return; | ||||
} | } | ||||
if (!$index_exists) { | if (!$index_exists) { | ||||
$summary = pht( | $summary = pht( | ||||
'You enabled Elasticsearch but the index does not exist.'); | 'You enabled Elasticsearch but the index does not exist.'); | ||||
Done Inline ActionsI don't think this is necessarily an error -- it's OK to put a service into production with no nodes in "read" mode in order to generate a hot backup or do a migration. Instead, we should probably do something like:
epriestley: I don't think this is necessarily an error -- it's OK to put a service into production with no… | |||||
Done Inline Actionssounds good 20after4: sounds good | |||||
$message = pht( | $message = pht( | ||||
'You likely enabled search.elastic.host without creating the '. | 'You likely enabled cluster.search without creating the '. | ||||
'index. Run `./bin/search init` to correct the index.'); | 'index. Run `./bin/search init` to correct the index.'); | ||||
$this | $this | ||||
->newIssue('elastic.missing-index') | ->newIssue('elastic.missing-index') | ||||
->setName(pht('Elasticsearch index Not Found')) | ->setName(pht('Elasticsearch index Not Found')) | ||||
->setSummary($summary) | ->setSummary($summary) | ||||
->setMessage($message) | ->setMessage($message) | ||||
->addRelatedPhabricatorConfig('search.elastic.host'); | ->addRelatedPhabricatorConfig('cluster.search'); | ||||
} else if (!$index_sane) { | } else if (!$index_sane) { | ||||
$summary = pht( | $summary = pht( | ||||
'Elasticsearch index exists but needs correction.'); | 'Elasticsearch index exists but needs correction.'); | ||||
$message = pht( | $message = pht( | ||||
'Either the Phabricator schema for Elasticsearch has changed '. | 'Either the Phabricator schema for Elasticsearch has changed '. | ||||
'or Elasticsearch created the index automatically. Run '. | 'or Elasticsearch created the index automatically. Run '. | ||||
'`./bin/search init` to correct the index.'); | '`./bin/search init` to correct the index.'); | ||||
$this | $this | ||||
->newIssue('elastic.broken-index') | ->newIssue('elastic.broken-index') | ||||
->setName(pht('Elasticsearch index Incorrect')) | ->setName(pht('Elasticsearch index Incorrect')) | ||||
->setSummary($summary) | ->setSummary($summary) | ||||
->setMessage($message); | ->setMessage($message); | ||||
} | } | ||||
} | } | ||||
protected function shouldUseElasticSearchEngine() { | |||||
$search_engine = PhabricatorFulltextStorageEngine::loadEngine(); | |||||
return ($search_engine instanceof PhabricatorElasticFulltextStorageEngine); | |||||
} | } | ||||
} | |||||
Done Inline ActionsThis probably doesn't need to happen yet, but this setup check should probably change to be something like: foreach ($all_the_configured_elastic_search_engines as $engine) { run_setup_checks_on($engine); } ...at some point in the future. This is approximately what the Database setup checks do today, I think. (Feel free to temporarily disable these checks while transitioning, too, if that makes it easier to break changes up.) epriestley: This probably doesn't need to happen yet, but this setup check should probably change to be… | |||||
} | } |
We don't need this or the corresponding method anymore, right? The loop has the same effect (only doing anything if Elastic is actually configured).