jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/325849 )
Change subject: Revert "Add configuration value to run interwiki load test"
......................................................................
Revert "Add configuration value to run interwiki load test"
Load test is complete, remove all the extra complexity we added
to run the test.
This reverts commit a8f6f8c88eb690c2d4a13f4bf0560bd8527547a6.
Change-Id: I19a6506ead56ef4469237c4688e06ec5f8c28fce
---
M CirrusSearch.php
M includes/CirrusSearch.php
M includes/ElasticsearchIntermediary.php
M includes/InterwikiSearcher.php
M includes/RequestLogger.php
D tests/unit/InterwikiSearcherTest.php
6 files changed, 13 insertions(+), 208 deletions(-)
Approvals:
Cindy-the-browser-test-bot: Looks good to me, but someone else must approve
jenkins-bot: Verified
DCausse: Looks good to me, approved
diff --git a/CirrusSearch.php b/CirrusSearch.php
index 2782a77..2072727 100644
--- a/CirrusSearch.php
+++ b/CirrusSearch.php
@@ -538,18 +538,6 @@
// Results are cached.
$wgCirrusSearchInterwikiSources = [];
-/**
- * Temporary special configuration for load testing the addition of interwiki
- * search results to a wiki. If this value is null then nothing special
- * happens, and wgCirrusSearchInterwikiSources is treated as usual. If this is
- * set to a value between 0 and 1 that is treated as the % of requests to
- * Special:Search that should use wgCirrusSearchInterwikiSources to make a
- * query. The results of this query will not be attached to the
- * SearchResultSet, and will not be displayed to the user. This is to estimate
- * the effect of adding this additional load onto a search cluster.
- */
-$wgCirrusSearchInterwikiLoadTest = null;
-
// How long to cache interwiki search results for (in seconds)
$wgCirrusSearchInterwikiCacheTime = 7200;
diff --git a/includes/CirrusSearch.php b/includes/CirrusSearch.php
index a31e310..7424603 100644
--- a/includes/CirrusSearch.php
+++ b/includes/CirrusSearch.php
@@ -393,12 +393,6 @@
$iwSearch->setReturnQuery( $dumpQuery );
$iwSearch->setDumpResult( $dumpResult );
$iwSearch->setReturnExplain( $returnExplain );
- // This is not strictly true, or even really half true.
It is a stand
- // in for the purposes of interwiki load testing using
the full
- // search configuration, rather than the limited
InterwikiResultsType.
- // InterwikiSearcher will use this or
InterwikiResultsType depending
- // on it's needs.
- $iwSearch->setResultsType( $resultsType );
$interwikiResults = $iwSearch->getInterwikiResults(
$term );
if ( $interwikiResults !== null ) {
// If we are dumping we need to convert into an
array that can be appended to
diff --git a/includes/ElasticsearchIntermediary.php
b/includes/ElasticsearchIntermediary.php
index e29235f..823e447 100644
--- a/includes/ElasticsearchIntermediary.php
+++ b/includes/ElasticsearchIntermediary.php
@@ -254,28 +254,11 @@
}
/**
- * Append a payload entry to the last request that was performed.
- *
* @param string $key
* @param string $value
*/
static public function appendLastLogPayload( $key, $value ) {
- if ( self::$requestLogger !== null ) {
- self::$requestLogger->appendLastLogPayload( $key,
$value );
- }
- }
-
- /**
- * Append a top-level payload entry to the request set log.
- *
- * @param string $key
- * @param string $value
- */
- static public function appendPayload( $key, $value ) {
- if ( self::$requestLogger === null ) {
- self::$requestLogger = new RequestLogger;
- }
- self::$requestLogger->appendPayload( $key, $value );
+ self::$requestLogger->appendLastLogPayload( $key, $value );
}
/**
diff --git a/includes/InterwikiSearcher.php b/includes/InterwikiSearcher.php
index 4ced44f..2c166df 100644
--- a/includes/InterwikiSearcher.php
+++ b/includes/InterwikiSearcher.php
@@ -33,16 +33,6 @@
const MAX_RESULTS = 5;
/**
- * @var bool Is the interwiki load test configured?
- */
- private $isLoadTest = false;
-
- /**
- * @var bool Is the interwiki load test enabled?
- */
- private $isLoadTestEnabled = false;
-
- /**
* Constructor
* @param Connection $connection
* @param SearchConfig $config
@@ -57,27 +47,7 @@
return $namespace <= 15;
} );
}
-
- $limit = self::MAX_RESULTS;
-
- $loadTestPercent = $config->get(
'CirrusSearchInterwikiLoadTest' );
- if ($loadTestPercent !== null ) {
- $this->isLoadTest = true;
-
- $requestedTitle =
\RequestContext::getMain()->getTitle();
-
- $isSpecialSearch = $requestedTitle &&
- $requestedTitle->getNamespace() === NS_SPECIAL
&&
- SpecialPageFactory::resolveAlias(
$requestedTitle->getText() )[0] === 'Search';
- $rand = mt_rand( 1, PHP_INT_MAX ) / PHP_INT_MAX;
- if ( $isSpecialSearch && $rand <= $loadTestPercent ) {
- $this->isLoadTestEnabled = true;
- ElasticsearchIntermediary::appendPayload(
'interwikiLoadTest', 'true' );
- $limit = 1;
- }
- }
-
- parent::__construct( $connection, 0, $limit, $config,
$namespaces, $user );
+ parent::__construct( $connection, 0, self::MAX_RESULTS,
$config, $namespaces, $user );
}
/**
@@ -104,11 +74,6 @@
if ( !$sources ) {
return null;
}
-
- if ( $this->isLoadTest && !$this->isLoadTestEnabled ) {
- return null;
- }
-
$this->searchContext->setCacheTtl(
$this->config->get( 'CirrusSearchInterwikiCacheTime' )
);
@@ -128,16 +93,11 @@
$searches = [];
$resultsTypes = [];
foreach ( $sources as $interwiki => $index ) {
- // Note that this is a hack, $this->resultsType is not
properly
- // specialized to the interwiki use case, but because
we are not
- // returning load test results to the users that is
acceptable.
- if (!$this->isLoadTestEnabled ) {
- // TODO: remove when getWikiCode is removed.
- // In theory we should be able to reuse the same
- // Results type for all searches
- $resultsTypes[$interwiki] = new
InterwikiResultsType( $this->config->newInterwikiConfig( $index, false ) );
- $this->setResultsType(
$resultsTypes[$interwiki] );
- }
+ // TODO: remove when getWikiCode is removed.
+ // In theory we should be able to reuse the same
+ // Results type for all searches
+ $resultsTypes[$interwiki] = new InterwikiResultsType(
$this->config->newInterwikiConfig( $index, false ) );
+ $this->setResultsType( $resultsTypes[$interwiki] );
$this->indexBaseName = $index;
$this->searchContext = clone $context;
$search = $this->buildSearch();
@@ -153,13 +113,7 @@
return null;
}
- if ($this->isLoadTest) {
- // For the load test we are generating the results, but
not
- // returning them to the user.
- return null;
- } else {
- return array_merge( $retval, $results->getValue() );
- }
+ return array_merge( $retval, $results->getValue() );
}
/**
diff --git a/includes/RequestLogger.php b/includes/RequestLogger.php
index f3c2389..f3334d0 100644
--- a/includes/RequestLogger.php
+++ b/includes/RequestLogger.php
@@ -47,12 +47,7 @@
* @var string[][] Extra payload for the logs, indexed first by the log
index
* in self::$logs, and second by the payload item name.
*/
- private $extraLogPayload = [];
-
- /**
- * @var string[] Extra top-level payload
- */
- private $payload = [];
+ private $extraPayload = [];
/**
* Summarizes all the requests made in this process and reports
@@ -63,13 +58,6 @@
$this->buildRequestSetLog();
$this->logs = [];
}
- }
-
- /**
- * @return int The number of logs held
- */
- public function count() {
- return count( $this->logs );
}
/**
@@ -113,26 +101,14 @@
}
/**
- * Append a payload entry to the last request that was performed.
- *
* @param string $key
* @param string $value
*/
public function appendLastLogPayload( $key, $value ) {
$idx = count( $this->logs ) - 1;
if ( isset( $this->logs[$idx] ) ) {
- $this->extraLogPayload[$idx][(string)$key] =
(string)$value;
+ $this->extraPayload[$idx][$key] = $value;
}
- }
-
- /**
- * Append a top-level payload entry to the request set log.
- *
- * @param string $key
- * @param string $value
- */
- public function appendPayload( $key, $value ) {
- $this->payload[(string)$key] = (string)$value;
}
/**
@@ -226,8 +202,8 @@
} else {
$allCached = false;
}
- if ( isset( $this->extraLogPayload[$idx] ) ) {
- foreach ( $this->extraLogPayload[$idx]
as $key => $value ) {
+ if ( isset( $this->extraPayload[$idx] ) ) {
+ foreach ( $this->extraPayload[$idx] as
$key => $value ) {
$request['payload'][$key] =
(string)$value;
}
}
@@ -274,7 +250,7 @@
'backendUserTests' =>
UserTesting::getInstance()->getActiveTestNamesWithBucket(),
'tookMs' => $this->getPhpRequestTookMs(),
'hits' => $resultHits,
- 'payload' => $this->payload + [
+ 'payload' => [
// useful while we are testing accept-lang
based interwiki
'acceptLang' => (string)
($wgRequest->getHeader( 'Accept-Language' ) ?: ''),
// Helps to track down what actually caused the
request. Will promote to full
diff --git a/tests/unit/InterwikiSearcherTest.php
b/tests/unit/InterwikiSearcherTest.php
deleted file mode 100644
index c746bd8..0000000
--- a/tests/unit/InterwikiSearcherTest.php
+++ /dev/null
@@ -1,90 +0,0 @@
-<?php
-
-namespace CirrusSearch;
-
-use CirrusSearch;
-use RequestContext;
-use Title;
-
-/**
- * @group CirrusSearch
- */
-class InterwikiSearcherTest extends CirrusTestCase {
- public function loadTestProvider() {
- return [
- 'when load test is null normal interwiki search
applies' => [
- true,
- 'Special:Search',
- null,
- ],
- 'when load test is 0 no interwiki search is performed'
=> [
- false,
- 'Special:Search',
- 0
- ],
- 'when load test is >= 1 interwiki search is performed;'
=> [
- true,
- 'Special:Search',
- 1
- ],
- 'load testing only applies to Special:Search' => [
- false,
- 'Special:AbuseFilter',
- 1
- ],
- 'load testing appropriatly handles aliases of
Special:Search' => [
- false,
- 'Служебная:Поиск',
- 1
- ],
- ];
- }
-
- /**
- * @dataProvider loadTestProvider
- */
- public function testLoadTest( $expectInterwiki, $titleString, $loadTest
) {
- // Maybe much of this should be extracted into some sort of
MockConnection
- // or some such...
- $calls = 0;
- $client = $this->getMockBuilder( \Elastica\Client::class )
- ->setMethods( ['request'] )
- ->getMock();
- $client->expects( $this->any() )
- ->method( 'request' )
- ->will( $this->returnCallback( function ( $path,
$method, $data, $options ) use ( &$calls ) {
- $newCalls = substr_count($data, "\n");
- if ( $newCalls > 1 ) {
- // bulk calls are two at a time
- $newCalls /= 2;
- }
- $calls += $newCalls;
- return new \Elastica\Response( json_encode( [
- 'responses' => array_fill(0, $newCalls,
['total' => 0, 'hits' => []])
- ] ), 200);
- } ) );
-
- $this->setMwGlobals( [
- 'wgCirrusSearchInterwikiSources' => [
- 'foo' => 'foowiki',
- ],
- 'wgCirrusSearchInterwikiLoadTest' => $loadTest,
- ] );
-
- $context = RequestContext::getMain();
- $context->setTitle( Title::newFromText( $titleString ) );
- $engine = new CirrusSearch();
- $connection = $engine->getConnection();
-
- $reflProp = new \ReflectionProperty( $connection, 'client' );
- $reflProp->setAccessible( true );
- $reflProp->setValue( $connection, $client );
-
- $query = $engine->searchText( 'some example' );
- if ( $expectInterwiki ) {
- $this->assertGreaterThan( 1, $calls );
- } else {
- $this->assertEquals( 1, $calls );
- }
- }
-}
--
To view, visit https://gerrit.wikimedia.org/r/325849
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I19a6506ead56ef4469237c4688e06ec5f8c28fce
Gerrit-PatchSet: 2
Gerrit-Project: mediawiki/extensions/CirrusSearch
Gerrit-Branch: master
Gerrit-Owner: EBernhardson <ebernhard...@wikimedia.org>
Gerrit-Reviewer: Cindy-the-browser-test-bot <bernhardsone...@gmail.com>
Gerrit-Reviewer: DCausse <dcau...@wikimedia.org>
Gerrit-Reviewer: Gehel <gleder...@wikimedia.org>
Gerrit-Reviewer: Smalyshev <smalys...@wikimedia.org>
Gerrit-Reviewer: Tjones <tjo...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
