From 699228c73b74e2a3ea2e8355ed822c9314fb9f88 Mon Sep 17 00:00:00 2001 From: Mukunda Modell Date: Tue, 28 Mar 2017 20:19:38 +0000 Subject: [PATCH] Address some New Search Configuration Errata Summary: [ ] Write an "Upgrading: ..." guidance task with narrow instructions for installs that are upgrading. [ ] Do we need to add an indexing activity (T11932) for installs with ElasticSearch? [ ] We should more clearly detail exactly which versions of ElasticSearch are supported (for example, is ElasticSearch <2 no longer supported)? From T9893 it seems like we may //only// have supported ElasticSearch <2 before, so are the two regions of support totally nonoverlapping and all ElasticSearch users will need to upgrade? [ ] Documentation should provide stronger guidance toward MySQL and away from Elastic for the vast majority of installs, because we've historically seen users choosing Elastic when they aren't actually trying to solve any specific problem. [ ] When users search for fulltext results in Maniphest and hit too many documents, the current behavior is approximately silent failure (see T12443). D17384 has also lowered the ceiling for ElasticSearch, although previous changes lowered it for MySQL search. We should not fail silently, and ideally should build toward T12003. [ ] D17384 added a new "keywords" field, but MySQL does not search it (I think?). The behavior should be as consistent across MySQL and Elastic as we can make it. Likely cleaner is giving "Project" objects a body, with "slugs" and "description" separated by newlines? [ ] `PhabricatorSearchEngineTestCase` is now pointless and only detects local misconfigurations. [ ] It would be nice to build a practical test suite instead, where we put specific documents into the index and then search for them. The upstream test could run against MySQL, and some `bin/search test` could run against a configured engine like ElasticSearch. This would make it easier to make sure that behavior was as uniform as possible across engine implementations. [ ] Does every assigned task now match "user" in ElasticSearch? [x] `PhabricatorElasticFulltextStorageEngine` has a `json_encode()` which should be `phutil_json_encode()`. [ ] `PhabricatorSearchService` throws an untranslated exception. [ ] When a search cluster is down, we probably don't degrade with much grace (unhandled exception)? [ ] I haven't run bin/search init, but bin/search index doesn't warn me that I may want to. This might be worth adding. The UI does warn me. [ ] bin/search init warns me that the index is "incorrect". It might be more clear to distinguish between "missing" and "incorrect", since it's more comforting to users to see "everything is as we expect, doing normal first-time setup now" than "something is wrong, fixing it". [ ] CLI message "Initializing search service "ElasticSearch"" does not end with a period, which is inconsistent with other UI messages. [ ] It might be nice to let bin/search commands like init and index select a specific service (or even service + host) to act on, as bin/storage --ref ... now does. You can generally get the result you want by fiddling with config. [ ] When a service isn't writable, bin/search init reports "Search cluster has no hosts for role "write".". This is accurate but does not provide guidance: it might be more useful to the user to explain "This service is not writable, so we're skipping index check for it.". [x] Even with write off for MySQL, bin/search index --type task --trace still updates MySQL, I think? I may be misreading the trace output. But this behavior doesn't make sense if it is the actual behavior, and it seems like reindexAbstractDocument() uses "all services", not "writable services", and the MySQL engine doesn't make sure it's writable before indexing. [x] Searching or user fails to find task Grant users tokens when a mention is created, suggesting that stemming is not working. [x] Searching for users finds that task, but fails to find a task containing "per user per month" in a comment, also suggesting that stemming is not working. [x] Searching for maniphest fails to find task maniphest.query elephant, suggesting that tokenization in ElasticSearch is not as good as the MySQL tokenization for these words (see D17330). [x] The "index incorrect" warning UI uses inconsistent title case. [x] The "index incorrect" warning UI could format the command to be run more cleanly (with addCommand(), I think). refs T12450 Test Plan: * Stared blankly at the code. * Disabled 'write' role on mysql fulltext service. * Edited a task, ran search indexer, verified that the mysql index wasn't being updated. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Maniphest Tasks: T12450 Differential Revision: https://secure.phabricator.com/D17564 --- .../PhabricatorElasticSearchSetupCheck.php | 16 +++--- ...habricatorElasticFulltextStorageEngine.php | 55 +++++++++++++++++-- .../search/PhabricatorMySQLSearchHost.php | 9 +++ .../cluster/search/PhabricatorSearchHost.php | 40 -------------- .../search/PhabricatorSearchService.php | 25 +++++---- 5 files changed, 80 insertions(+), 65 deletions(-) diff --git a/src/applications/config/check/PhabricatorElasticSearchSetupCheck.php b/src/applications/config/check/PhabricatorElasticSearchSetupCheck.php index d8864b5740..dab886b92a 100644 --- a/src/applications/config/check/PhabricatorElasticSearchSetupCheck.php +++ b/src/applications/config/check/PhabricatorElasticSearchSetupCheck.php @@ -49,26 +49,28 @@ final class PhabricatorElasticSearchSetupCheck extends PhabricatorSetupCheck { $message = pht( 'You likely enabled cluster.search without creating the '. - 'index. Run `./bin/search init` to correct the index.'); + 'index. Use the following command to create a new index.'); $this ->newIssue('elastic.missing-index') - ->setName(pht('Elasticsearch index Not Found')) + ->setName(pht('Elasticsearch Index Not Found')) + ->addCommand('./bin/search init') ->setSummary($summary) - ->setMessage($message) - ->addRelatedPhabricatorConfig('cluster.search'); + ->setMessage($message); + } else if (!$index_sane) { $summary = pht( 'Elasticsearch index exists but needs correction.'); $message = pht( 'Either the Phabricator schema for Elasticsearch has changed '. - 'or Elasticsearch created the index automatically. Run '. - '`./bin/search init` to correct the index.'); + 'or Elasticsearch created the index automatically. '. + 'Use the following command to rebuild the index.'); $this ->newIssue('elastic.broken-index') - ->setName(pht('Elasticsearch index Incorrect')) + ->setName(pht('Elasticsearch Index Schema Mismatch')) + ->addCommand('./bin/search init') ->setSummary($summary) ->setMessage($message); } diff --git a/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php b/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php index bc32da5ef4..6d2cd9adc5 100644 --- a/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php +++ b/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php @@ -161,9 +161,11 @@ class PhabricatorElasticFulltextStorageEngine 'simple_query_string' => array( 'query' => $query_string, 'fields' => array( - '_all', + PhabricatorSearchDocumentFieldType::FIELD_TITLE.'.*', + PhabricatorSearchDocumentFieldType::FIELD_BODY.'.*', + PhabricatorSearchDocumentFieldType::FIELD_COMMENT.'.*', ), - 'default_operator' => 'OR', + 'default_operator' => 'AND', ), )); @@ -175,6 +177,7 @@ class PhabricatorElasticFulltextStorageEngine 'simple_query_string' => array( 'query' => $query_string, 'fields' => array( + '*.raw', PhabricatorSearchDocumentFieldType::FIELD_TITLE.'^4', PhabricatorSearchDocumentFieldType::FIELD_BODY.'^3', PhabricatorSearchDocumentFieldType::FIELD_COMMENT.'^1.2', @@ -332,11 +335,38 @@ class PhabricatorElasticFulltextStorageEngine 'index' => array( 'auto_expand_replicas' => '0-2', 'analysis' => array( + 'filter' => array( + 'english_stop' => array( + 'type' => 'stop', + 'stopwords' => '_english_', + ), + 'english_stemmer' => array( + 'type' => 'stemmer', + 'language' => 'english', + ), + 'english_possessive_stemmer' => array( + 'type' => 'stemmer', + 'language' => 'possessive_english', + ), + ), 'analyzer' => array( 'english_exact' => array( 'tokenizer' => 'standard', 'filter' => array('lowercase'), ), + 'letter_stop' => array( + 'tokenizer' => 'letter', + 'filter' => array('lowercase', 'english_stop'), + ), + 'english_stem' => array( + 'tokenizer' => 'standard', + 'filter' => array( + 'english_possessive_stemmer', + 'lowercase', + 'english_stop', + 'english_stemmer', + ), + ), ), ), ), @@ -356,9 +386,22 @@ class PhabricatorElasticFulltextStorageEngine // Use the custom analyzer for the corpus of text $properties[$field] = array( 'type' => $text_type, - 'analyzer' => 'english_exact', - 'search_analyzer' => 'english', - 'search_quote_analyzer' => 'english_exact', + 'fields' => array( + 'raw' => array( + 'type' => $text_type, + 'analyzer' => 'english_exact', + 'search_analyzer' => 'english', + 'search_quote_analyzer' => 'english_exact', + ), + 'keywords' => array( + 'type' => $text_type, + 'analyzer' => 'letter_stop', + ), + 'stems' => array( + 'type' => $text_type, + 'analyzer' => 'english_stem', + ), + ), ); } @@ -505,7 +548,7 @@ class PhabricatorElasticFulltextStorageEngine array $data, $method = 'GET') { $uri = $host->getURI($path); - $data = json_encode($data); + $data = phutil_json_encode($data); $future = new HTTPSFuture($uri, $data); if ($method != 'GET') { $future->setMethod($method); diff --git a/src/infrastructure/cluster/search/PhabricatorMySQLSearchHost.php b/src/infrastructure/cluster/search/PhabricatorMySQLSearchHost.php index ced23cd542..742b5713d3 100644 --- a/src/infrastructure/cluster/search/PhabricatorMySQLSearchHost.php +++ b/src/infrastructure/cluster/search/PhabricatorMySQLSearchHost.php @@ -24,6 +24,15 @@ final class PhabricatorMySQLSearchHost return 'mysql'; } + public function getHealthRecord() { + if (!$this->healthRecord) { + $ref = PhabricatorDatabaseRef::getMasterDatabaseRefForApplication( + 'search'); + $this->healthRecord = $ref->getHealthRecord(); + } + return $this->healthRecord; + } + public function getConnectionStatus() { PhabricatorDatabaseRef::queryAll(); $ref = PhabricatorDatabaseRef::getMasterDatabaseRefForApplication('search'); diff --git a/src/infrastructure/cluster/search/PhabricatorSearchHost.php b/src/infrastructure/cluster/search/PhabricatorSearchHost.php index 834e786789..93c3c4d938 100644 --- a/src/infrastructure/cluster/search/PhabricatorSearchHost.php +++ b/src/infrastructure/cluster/search/PhabricatorSearchHost.php @@ -13,7 +13,6 @@ abstract class PhabricatorSearchHost protected $disabled; protected $host; protected $port; - protected $hostRefs = array(); const STATUS_OKAY = 'okay'; const STATUS_FAIL = 'fail'; @@ -121,43 +120,4 @@ abstract class PhabricatorSearchHost abstract public function getConnectionStatus(); - public static function reindexAbstractDocument( - PhabricatorSearchAbstractDocument $doc) { - - $services = self::getAllServices(); - $indexed = 0; - foreach (self::getWritableHostForEachService() as $host) { - $host->getEngine()->reindexAbstractDocument($doc); - $indexed++; - } - if ($indexed == 0) { - throw new PhabricatorClusterNoHostForRoleException('write'); - } - } - - public static function executeSearch(PhabricatorSavedQuery $query) { - $services = self::getAllServices(); - foreach ($services as $service) { - $hosts = $service->getAllHostsForRole('read'); - // try all hosts until one succeeds - foreach ($hosts as $host) { - $last_exception = null; - try { - $res = $host->getEngine()->executeSearch($query); - // return immediately if we get results without an exception - $host->didHealthCheck(true); - return $res; - } catch (Exception $ex) { - // try each server in turn, only throw if none succeed - $last_exception = $ex; - $host->didHealthCheck(false); - } - } - } - if ($last_exception) { - throw $last_exception; - } - return $res; - } - } diff --git a/src/infrastructure/cluster/search/PhabricatorSearchService.php b/src/infrastructure/cluster/search/PhabricatorSearchService.php index 20c0664456..0a563207b1 100644 --- a/src/infrastructure/cluster/search/PhabricatorSearchService.php +++ b/src/infrastructure/cluster/search/PhabricatorSearchService.php @@ -46,6 +46,7 @@ class PhabricatorSearchService public function setConfig($config) { $this->config = $config; + $this->setRoles(idx($config, 'roles', array())); if (!isset($config['hosts'])) { $config['hosts'] = array( @@ -67,15 +68,6 @@ class PhabricatorSearchService return $this->config; } - public function setDisabled($disabled) { - $this->disabled = $disabled; - return $this; - } - - public function getDisabled() { - return $this->disabled; - } - public static function getConnectionStatusMap() { return array( self::STATUS_OKAY => array( @@ -100,7 +92,7 @@ class PhabricatorSearchService } public function hasRole($role) { - return isset($this->roles[$role]) && $this->roles[$role] === true; + return isset($this->roles[$role]) && $this->roles[$role] !== false; } public function setRoles(array $roles) { @@ -160,6 +152,12 @@ class PhabricatorSearchService * @return PhabricatorSearchHost[] */ public function getAllHostsForRole($role) { + // if the role is explicitly set to false at the top level, then all hosts + // have the role disabled. + if (idx($this->config, $role) === false) { + return array(); + } + $hosts = array(); foreach ($this->hosts as $host) { if ($host->hasRole($role)) { @@ -225,8 +223,11 @@ class PhabricatorSearchService PhabricatorSearchAbstractDocument $doc) { $indexed = 0; foreach (self::getAllServices() as $service) { - $service->getEngine()->reindexAbstractDocument($doc); - $indexed++; + $hosts = $service->getAllHostsForRole('write'); + if (count($hosts)) { + $service->getEngine()->reindexAbstractDocument($doc); + $indexed++; + } } if ($indexed == 0) { throw new PhabricatorClusterNoHostForRoleException('write');