From 312ba307148552501dcc6dd8e7f82d61310a8cf9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 15 Feb 2019 05:24:17 -0800 Subject: [PATCH] Don't report search indexing errors to the daemon log except from "bin/search index" Summary: Depends on D20177. Fixes T12425. See . Search indexing currently reports failures to load objects to the log. This log is noisy, not concerning, not actionable, and not locally debuggable (it depends on the reporting user's entire state). I think one common, fully legitimate case of this is indexing temporary files: they may fully legitimately be deleted by the time the indexer runs. Instead of sending these errors to the log, eat them. If users don't notice the indexes aren't working: no harm, no foul. If users do notice, we'll run or have them run `bin/search index` as a first diagnostic step anyway, which will now report an actionable/reproducible error. Test Plan: - Faked errors in both cases (initial load, re-load inside the locked section). - Ran indexes in strict/non-strict mode. - Got exception reports from both branches in strict mode. - Got task success without errors in both cases in non-strict mode. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12425 Differential Revision: https://secure.phabricator.com/D20178 --- ...abricatorSearchManagementIndexWorkflow.php | 9 ++- .../search/worker/PhabricatorSearchWorker.php | 55 +++++++++++++++---- 2 files changed, 51 insertions(+), 13 deletions(-) diff --git a/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php b/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php index 6b6d25cb6c..99ee3a3123 100644 --- a/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php +++ b/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php @@ -116,6 +116,10 @@ final class PhabricatorSearchManagementIndexWorkflow // them a hint that they might want to use "--force". $track_skips = (!$is_background && !$is_force); + // Activate "strict" error reporting if we're running in the foreground + // so we'll report a wider range of conditions as errors. + $is_strict = !$is_background; + $count_updated = 0; $count_skipped = 0; @@ -125,7 +129,10 @@ final class PhabricatorSearchManagementIndexWorkflow $old_versions = $this->loadIndexVersions($phid); } - PhabricatorSearchWorker::queueDocumentForIndexing($phid, $parameters); + PhabricatorSearchWorker::queueDocumentForIndexing( + $phid, + $parameters, + $is_strict); if ($track_skips) { $new_versions = $this->loadIndexVersions($phid); diff --git a/src/applications/search/worker/PhabricatorSearchWorker.php b/src/applications/search/worker/PhabricatorSearchWorker.php index 361d9c9b6f..31c68d45c8 100644 --- a/src/applications/search/worker/PhabricatorSearchWorker.php +++ b/src/applications/search/worker/PhabricatorSearchWorker.php @@ -2,7 +2,11 @@ final class PhabricatorSearchWorker extends PhabricatorWorker { - public static function queueDocumentForIndexing($phid, $parameters = null) { + public static function queueDocumentForIndexing( + $phid, + $parameters = null, + $is_strict = false) { + if ($parameters === null) { $parameters = array(); } @@ -12,6 +16,7 @@ final class PhabricatorSearchWorker extends PhabricatorWorker { array( 'documentPHID' => $phid, 'parameters' => $parameters, + 'strict' => $is_strict, ), array( 'priority' => parent::PRIORITY_INDEX, @@ -23,7 +28,25 @@ final class PhabricatorSearchWorker extends PhabricatorWorker { $data = $this->getTaskData(); $object_phid = idx($data, 'documentPHID'); - $object = $this->loadObjectForIndexing($object_phid); + // See T12425. By the time we run an indexing task, the object it indexes + // may have been deleted. This is unusual, but not concerning, and failing + // to index these objects is correct. + + // To avoid showing these non-actionable errors to users, don't report + // indexing exceptions unless we're in "strict" mode. This mode is set by + // the "bin/search index" tool. + + $is_strict = idx($data, 'strict', false); + + try { + $object = $this->loadObjectForIndexing($object_phid); + } catch (PhabricatorWorkerPermanentFailureException $ex) { + if ($is_strict) { + throw $ex; + } else { + return; + } + } $engine = id(new PhabricatorIndexEngine()) ->setObject($object); @@ -35,8 +58,11 @@ final class PhabricatorSearchWorker extends PhabricatorWorker { return; } - $key = "index.{$object_phid}"; - $lock = PhabricatorGlobalLock::newLock($key); + $lock = PhabricatorGlobalLock::newLock( + 'index', + array( + 'objectPHID' => $object_phid, + )); try { $lock->lock(1); @@ -48,29 +74,34 @@ final class PhabricatorSearchWorker extends PhabricatorWorker { throw new PhabricatorWorkerYieldException(15); } + $caught = null; try { // Reload the object now that we have a lock, to make sure we have the // most current version. $object = $this->loadObjectForIndexing($object->getPHID()); $engine->setObject($object); - $engine->indexObject(); } catch (Exception $ex) { - $lock->unlock(); + $caught = $ex; + } + + // Release the lock before we deal with the exception. + $lock->unlock(); - if (!($ex instanceof PhabricatorWorkerPermanentFailureException)) { - $ex = new PhabricatorWorkerPermanentFailureException( + if ($caught) { + if (!($caught instanceof PhabricatorWorkerPermanentFailureException)) { + $caught = new PhabricatorWorkerPermanentFailureException( pht( 'Failed to update search index for document "%s": %s', $object_phid, - $ex->getMessage())); + $caught->getMessage())); } - throw $ex; + if ($is_strict) { + throw $caught; + } } - - $lock->unlock(); } private function loadObjectForIndexing($phid) { -- 2.51.2