Manybubbles has uploaded a new change for review.
https://gerrit.wikimedia.org/r/173298
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
(cherry picked from commit b8042c6ca24ed5c02ce0d76d9dcdcc4464ac2c59)
---
M includes/Searcher.php
M includes/Util.php
M tests/browser/selenium_exports_for_vagrant.sh
3 files changed, 54 insertions(+), 39 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/CirrusSearch
refs/changes/98/173298/1
diff --git a/includes/Searcher.php b/includes/Searcher.php
index 22c299b..06808b3 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;
@@ -824,15 +823,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 Util::doPoolCounterWork(
+ 'CirrusSearch-Search',
+ function() use ( $searcher, $pageIds, $sourceFiltering,
$indexType, $indexBaseName ) {
try {
global
$wgCirrusSearchClientSideSearchTimeout;
$searcher->start( "get of $indexType."
. implode( ', ', $pageIds ) );
@@ -851,22 +849,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 Util::doPoolCounterWork(
+ 'CirrusSearch-NamespaceLookup',
+ function() use ( $searcher, $name, $indexBaseName ) {
try {
$searcher->start( "lookup namespace for
$name" );
$pageType =
Connection::getNamespaceType( $indexBaseName );
@@ -880,14 +871,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 ) {
@@ -947,8 +931,7 @@
private function search( $type, $for ) {
global $wgCirrusSearchMoreAccurateScoringMode,
$wgCirrusSearchSearchShardTimeout,
- $wgCirrusSearchClientSideSearchTimeout,
- $wgCirrusSearchPoolCounterKey;
+ $wgCirrusSearchClientSideSearchTimeout;
$profiler = new ProfileSection( __METHOD__ );
@@ -1094,9 +1077,10 @@
// Perform the search
$searcher = $this;
- $key = $wgCirrusSearchPoolCounterKey;
- $work = new PoolCounterWorkViaCallback( $poolCounterType, $key,
array(
- 'doWork' => function() use ( $searcher, $search,
$description ) {
+ wfProfileIn( __METHOD__ . '-execute' );
+ $result = Util::doPoolCounterWork(
+ $poolCounterType,
+ function() use ( $searcher, $search, $description ) {
try {
$searcher->start( $description );
return $searcher->success(
$search->search() );
@@ -1104,20 +1088,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();
diff --git a/includes/Util.php b/includes/Util.php
index 664b589..0b7bf47 100644
--- a/includes/Util.php
+++ b/includes/Util.php
@@ -3,6 +3,7 @@
namespace CirrusSearch;
use \GenderCache;
use \MWNamespace;
+use \PoolCounterWorkViaCallback;
use \Title;
use \User;
@@ -84,4 +85,37 @@
}
return true;
}
+
+ /**
+ * Wraps the complex pool counter interface to force the single call
pattern
+ * that Cirrus always uses.
+ * @param $type same as type parameter on PoolCounter::factory
+ * @param $workCallback callback when pool counter is aquired. Called
with
+ * no parameters.
+ * @param $errorCallback optional callback called on errors. Called
with
+ * the error string and the key as parameters. If left undefined
defaults
+ * to a function that returns a fatal status and logs an warning.
+ */
+ public static function doPoolCounterWork( $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/tests/browser/selenium_exports_for_vagrant.sh
b/tests/browser/selenium_exports_for_vagrant.sh
index 1d621e4..1d63759 100644
--- a/tests/browser/selenium_exports_for_vagrant.sh
+++ b/tests/browser/selenium_exports_for_vagrant.sh
@@ -2,6 +2,7 @@
export MEDIAWIKI_PASSWORD=vagrant
export MEDIAWIKI_URL=http://127.0.0.1:8080/wiki/
export MEDIAWIKI_API_URL=http://127.0.0.1:8080/w/api.php
-export REUSE_BROWSER=false
+export REUSE_BROWSER=true
export SCREENSHOT_FAILURES=true
-export BROWSER=phantomjs
+export BROWSER=firefox
+export HEADLESS=true
--
To view, visit https://gerrit.wikimedia.org/r/173298
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: wmf/1.25wmf7
Gerrit-Owner: Manybubbles <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
