Manybubbles has uploaded a new change for review. https://gerrit.wikimedia.org/r/173279
Change subject: Fix pool counter usage ...................................................................... Fix pool counter usage By default the pool counter allows you to lock the same key with multiple types. That might be useful but it isn't how Cirrus thinks. Instead, Cirrus now scopes all keys to their types. That means that locks for the _elasticsearch key using the Namespace type are independant from Search type. Change-Id: I7586162cfb32ddbe460a25c956c845f2f4a49b0f --- M CirrusSearch.php A includes/PoolCounterHelper.php M includes/Searcher.php 3 files changed, 67 insertions(+), 37 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/CirrusSearch refs/changes/79/173279/1 diff --git a/CirrusSearch.php b/CirrusSearch.php index 8b68420..ea702a3 100644 --- a/CirrusSearch.php +++ b/CirrusSearch.php @@ -573,6 +573,7 @@ $wgAutoloadClasses['CirrusSearch\Maintenance\UpdateVersionIndex'] = __DIR__ . '/maintenance/updateVersionIndex.php'; $wgAutoloadClasses['CirrusSearch\NearMatchPicker'] = $includes . 'NearMatchPicker.php'; $wgAutoloadClasses['CirrusSearch\OtherIndexes'] = $includes . 'OtherIndexes.php'; +$wgAutoloadClasses['CirrusSearch\PoolCounterHelper'] = $includes . 'PoolCounterHelper.php'; $wgAutoloadClasses['CirrusSearch\Sanity\Checker'] = $sanity . 'Checker.php'; $wgAutoloadClasses['CirrusSearch\Sanity\NoopRemediator'] = $sanity . 'Remediator.php'; $wgAutoloadClasses['CirrusSearch\Sanity\PrintingRemediator'] = $sanity . 'Remediator.php'; diff --git a/includes/PoolCounterHelper.php b/includes/PoolCounterHelper.php new file mode 100644 index 0000000..e2c38a7 --- /dev/null +++ b/includes/PoolCounterHelper.php @@ -0,0 +1,49 @@ +<?php + +namespace CirrusSearch; +use \PoolCounterWorkViaCallback; +use \Status; + +/** + * Wraps the complex pool counter interface to force the single call pattern + * that Cirrus always uses. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + * http://www.gnu.org/copyleft/gpl.html + */ +class PoolCounterHelper { + public static function doWork( $type, $workCallback, $errorCallback = null ) { + global $wgCirrusSearchPoolCounterKey; + + // By default the pool counter allows you to lock the same key with + // multiple types. That might be useful but it isn't how Cirrus thinks. + // Instead, all keys are scoped to their type. + $key = "$type:$wgCirrusSearchPoolCounterKey"; + if ( $errorCallback === null ) { + $errorCallback = function( $error, $key ) { + wfLogWarning( "Pool error on $key: $error" ); + return Status::newFatal( 'cirrussearch-backend-error' ); + }; + } + $work = new PoolCounterWorkViaCallback( $type, $key, array( + 'doWork' => $workCallback, + 'error' => function( $status ) use ( $errorCallback, $key ) { + $status = $status->getErrorsArray(); + return $errorCallback( $status[ 0 ][ 0 ], $key ); + } + ) ); + return $work->execute(); + } +} diff --git a/includes/Searcher.php b/includes/Searcher.php index c6d2207..9a5bc41 100644 --- a/includes/Searcher.php +++ b/includes/Searcher.php @@ -11,7 +11,6 @@ use \CirrusSearch\Search\ResultsType; use \Language; use \MWNamespace; -use \PoolCounterWorkViaCallback; use \ProfileSection; use \RequestContext; use \Sanitizer; @@ -829,15 +828,14 @@ * or an error if there was an error */ public function get( $pageIds, $sourceFiltering ) { - global $wgCirrusSearchPoolCounterKey; - $profiler = new ProfileSection( __METHOD__ ); $indexType = $this->pickIndexTypeFromNamespaces(); $searcher = $this; $indexBaseName = $this->indexBaseName; - $getWork = new PoolCounterWorkViaCallback( 'CirrusSearch-Search', $wgCirrusSearchPoolCounterKey, array( - 'doWork' => function() use ( $searcher, $pageIds, $sourceFiltering, $indexType, $indexBaseName ) { + return PoolCounterHelper::doWork( + 'CirrusSearch-Search', + function() use ( $searcher, $pageIds, $sourceFiltering, $indexType, $indexBaseName ) { try { global $wgCirrusSearchClientSideSearchTimeout; $searcher->start( "get of $indexType." . implode( ', ', $pageIds ) ); @@ -856,22 +854,15 @@ } catch ( \Elastica\Exception\ExceptionInterface $e ) { return $searcher->failure( $e ); } - }, - 'error' => function( $status ) { - $status = $status->getErrorsArray(); - wfLogWarning( 'Pool error performing a get against Elasticsearch: ' . $status[ 0 ][ 0 ] ); - return Status::newFatal( 'cirrussearch-backend-error' ); - } - ) ); - return $getWork->execute(); + }); } public function findNamespace( $name ) { - global $wgCirrusSearchPoolCounterKey; $searcher = $this; $indexBaseName = $this->indexBaseName; - $getWork = new PoolCounterWorkViaCallback( 'CirrusSearch-NamespaceLookup', $wgCirrusSearchPoolCounterKey, array( - 'doWork' => function() use ( $searcher, $name, $indexBaseName ) { + return PoolCounterHelper::doWork( + 'CirrusSearch-NamespaceLookup', + function() use ( $searcher, $name, $indexBaseName ) { try { $searcher->start( "lookup namespace for $name" ); $pageType = Connection::getNamespaceType( $indexBaseName ); @@ -885,14 +876,7 @@ } catch ( \Elastica\Exception\ExceptionInterface $e ) { return $searcher->failure( $e ); } - }, - 'error' => function( $status ) { - $status = $status->getErrorsArray(); - wfLogWarning( 'Pool error performing a namespace lookup against Elasticsearch: ' . $status[ 0 ][ 0 ] ); - return Status::newFatal( 'cirrussearch-backend-error' ); - } - ) ); - return $getWork->execute(); + }); } private function extractSpecialSyntaxFromTerm( $regex, $callback ) { @@ -952,8 +936,7 @@ private function search( $type, $for ) { global $wgCirrusSearchMoreAccurateScoringMode, $wgCirrusSearchSearchShardTimeout, - $wgCirrusSearchClientSideSearchTimeout, - $wgCirrusSearchPoolCounterKey; + $wgCirrusSearchClientSideSearchTimeout; $profiler = new ProfileSection( __METHOD__ ); @@ -1099,9 +1082,10 @@ // Perform the search $searcher = $this; - $key = $wgCirrusSearchPoolCounterKey; - $work = new PoolCounterWorkViaCallback( $poolCounterType, $key, array( - 'doWork' => function() use ( $searcher, $search, $description ) { + wfProfileIn( __METHOD__ . '-execute' ); + $result = PoolCounterHelper::doWork( + $poolCounterType, + function() use ( $searcher, $search, $description ) { try { $searcher->start( $description ); return $searcher->success( $search->search() ); @@ -1109,20 +1093,16 @@ return $searcher->failure( $e ); } }, - 'error' => function( $status ) use ( $type, $key, $description ) { - $status = $status->getErrorsArray(); - wfLogWarning( "Elasticsearch pool error on key $key during $description: " . $status[ 0 ][ 0 ] ); - if ( $status[ 0 ][ 0 ] === 'pool-queuefull' ) { + function( $error, $key ) use ( $type, $description ) { + wfLogWarning( "Pool error on key $key during $description: $error" ); + if ( $error === 'pool-queuefull' ) { if ( $type === 'regex' ) { return Status::newFatal( 'cirrussearch-regex-too-busy-error' ); } return Status::newFatal( 'cirrussearch-too-busy-error' ); } return Status::newFatal( 'cirrussearch-backend-error' ); - } - ) ); - wfProfileIn( __METHOD__ . '-execute' ); - $result = $work->execute(); + }); wfProfileOut( __METHOD__ . '-execute' ); if ( $result->isOK() ) { $responseData = $result->getValue()->getResponse()->getData(); -- To view, visit https://gerrit.wikimedia.org/r/173279 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I7586162cfb32ddbe460a25c956c845f2f4a49b0f Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/CirrusSearch Gerrit-Branch: master Gerrit-Owner: Manybubbles <[email protected]> _______________________________________________ MediaWiki-commits mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
