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

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)
+            ->setMessage($message);
-            ->addRelatedPhabricatorConfig('cluster.search');
+
         } 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 '.
+            'or Elasticsearch created the index automatically. '.
-            '`./bin/search init` to correct the index.');
+            '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);
         }

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',
+          'fields' => array(
-          'search_analyzer'       => 'english',
+            'raw' => array(
-          'search_quote_analyzer' => 'english_exact',
+              '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);

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');

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;
-  }
-
 }

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);
+      $hosts = $service->getAllHostsForRole('write');
-      $indexed++;
+      if (count($hosts)) {
+        $service->getEngine()->reindexAbstractDocument($doc);
+        $indexed++;
+      }
     }
     if ($indexed == 0) {
       throw new PhabricatorClusterNoHostForRoleException('write');