[MediaWiki-commits] [Gerrit] mediawiki...CirrusSearch[master]: Improve handling of 5xx responses to elasticsearch requests

2017-11-07 Thread jenkins-bot (Code Review) 
jenkins-bot has submitted this change and it was merged. ( 
https://gerrit.wikimedia.org/r/386741 )

Change subject: Improve handling of 5xx responses to elasticsearch requests
..


Improve handling of 5xx responses to elasticsearch requests

The Elastica\Response::hasError() method is almost never the one you
want to check, on its own at least, to see if a request failed. This
only takes into account if the returned json document contains an
'error' key. Other errors, such as a timeout from a proxy in front
of elasticsearch which does not return a json document will not
trigger hasError.

Reviewed current uses of hasError and added checks to Response::isOk()
in appropriate places. Also filed upstream bug at
https://github.com/ruflin/Elastica/issues/1396 to discuss if this
is desired behaviour.

Change-Id: I8677ceb9dcbc7dde455894b60b5e5851f309a74b
---
M includes/DataSender.php
M includes/Elastica/ReindexRequest.php
M includes/Maintenance/ConfigUtils.php
M includes/Maintenance/IndexCreator.php
M includes/Maintenance/Reindexer.php
5 files changed, 18 insertions(+), 21 deletions(-)

Approvals:
  Smalyshev: Looks good to me, but someone else must approve
  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/includes/DataSender.php b/includes/DataSender.php
index a539d5f..1989dc3 100644
--- a/includes/DataSender.php
+++ b/includes/DataSender.php
@@ -416,24 +416,14 @@
) {
$justDocumentMissing = true;
foreach ( $exception->getResponseSet()->getBulkResponses() as 
$bulkResponse ) {
-   if ( !$bulkResponse->hasError() ) {
+   if ( $bulkResponse->isOK() && 
!$bulkResponse->hasError() ) {
continue;
}
 
$error = $bulkResponse->getFullError();
-   if ( is_string( $error ) ) {
-   // es 1.7 cluster
-   $message = $bulkResponse->getError();
-   if ( false === strpos( $message, 
'DocumentMissingException' ) ) {
-   $justDocumentMissing = false;
-   continue;
-   }
-   } else {
-   // es 2.x cluster
-   if ( $error['type'] !== 
'document_missing_exception' ) {
-   $justDocumentMissing = false;
-   continue;
-   }
+   if ( isset( $error['type'] ) && $error['type'] !== 
'document_missing_exception' ) {
+   $justDocumentMissing = false;
+   continue;
}
 
if ( $logCallback ) {
diff --git a/includes/Elastica/ReindexRequest.php 
b/includes/Elastica/ReindexRequest.php
index 9fc3610..826c8f4 100644
--- a/includes/Elastica/ReindexRequest.php
+++ b/includes/Elastica/ReindexRequest.php
@@ -112,7 +112,7 @@
$query['slices'] = $this->slices;
$response = $this->client->request( '_reindex', Request::POST, 
$this->toArray(), $query );
 
-   if ( !$response->isOK() ) {
+   if ( !$response->isOk() ) {
throw new \Exception( $response->hasError()
? 'Failed reindex request: ' . 
$response->getErrorMessage()
: 'Unknown reindex failure: ' . 
$response->getStatus()
diff --git a/includes/Maintenance/ConfigUtils.php 
b/includes/Maintenance/ConfigUtils.php
index 4faf484..9ce60ce 100644
--- a/includes/Maintenance/ConfigUtils.php
+++ b/includes/Maintenance/ConfigUtils.php
@@ -107,7 +107,7 @@
public function getAllIndicesByType( $typeName ) {
$found = null;
$response = $this->client->request( $typeName . '*' );
-   if ( $response->isOK() ) {
+   if ( $response->isOK() && !$response->hasError() ) {
$found = array_keys( $response->getData() );
} else {
$this->error( "Cannot fetch index names for $typeName: "
@@ -228,7 +228,8 @@
public function getIndexHealth( $indexName ) {
$path = "_cluster/health/$indexName";
$response = $this->client->request( $path );
-   if ( $response->hasError() ) {
+   $http2xx = $response->getStatus() >= 200 && 
$response->getStatus() < 300;
+   if ( !$http2xx || $response->hasError() ) {
throw new \Exception( "Error while fetching index 
health status: ". $response->getError() );
}
return $response->getData();
@@

[MediaWiki-commits] [Gerrit] mediawiki...CirrusSearch[master]: Improve handling of 5xx responses to elasticsearch requests

2017-10-26 Thread EBernhardson (Code Review) 
EBernhardson has uploaded a new change for review. ( 
https://gerrit.wikimedia.org/r/386741 )

Change subject: Improve handling of 5xx responses to elasticsearch requests
..

Improve handling of 5xx responses to elasticsearch requests

The Elastica\Response::hasError() method is almost never the one you
want to check, on its own at least, to see if a request failed. This
only takes into account if the returned json document contains an
'error' key. Other errors, such as a timeout from a proxy in front
of elasticsearch which does not return a json document will not
trigger hasError.

Reviewed current uses of hasError and added checks to Response::isOk()
in appropriate places. Also filed upstream bug at
https://github.com/ruflin/Elastica/issues/1396 to discuss if this
is desired behaviour.

Change-Id: I8677ceb9dcbc7dde455894b60b5e5851f309a74b
---
M includes/DataSender.php
M includes/Elastica/ReindexRequest.php
M includes/Maintenance/ConfigUtils.php
M includes/Maintenance/IndexCreator.php
M includes/Maintenance/Reindexer.php
5 files changed, 17 insertions(+), 21 deletions(-)


  git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/CirrusSearch 
refs/changes/41/386741/1

diff --git a/includes/DataSender.php b/includes/DataSender.php
index a539d5f..1989dc3 100644
--- a/includes/DataSender.php
+++ b/includes/DataSender.php
@@ -416,24 +416,14 @@
) {
$justDocumentMissing = true;
foreach ( $exception->getResponseSet()->getBulkResponses() as 
$bulkResponse ) {
-   if ( !$bulkResponse->hasError() ) {
+   if ( $bulkResponse->isOK() && 
!$bulkResponse->hasError() ) {
continue;
}
 
$error = $bulkResponse->getFullError();
-   if ( is_string( $error ) ) {
-   // es 1.7 cluster
-   $message = $bulkResponse->getError();
-   if ( false === strpos( $message, 
'DocumentMissingException' ) ) {
-   $justDocumentMissing = false;
-   continue;
-   }
-   } else {
-   // es 2.x cluster
-   if ( $error['type'] !== 
'document_missing_exception' ) {
-   $justDocumentMissing = false;
-   continue;
-   }
+   if ( isset( $error['type'] ) && $error['type'] !== 
'document_missing_exception' ) {
+   $justDocumentMissing = false;
+   continue;
}
 
if ( $logCallback ) {
diff --git a/includes/Elastica/ReindexRequest.php 
b/includes/Elastica/ReindexRequest.php
index 9fc3610..826c8f4 100644
--- a/includes/Elastica/ReindexRequest.php
+++ b/includes/Elastica/ReindexRequest.php
@@ -112,7 +112,7 @@
$query['slices'] = $this->slices;
$response = $this->client->request( '_reindex', Request::POST, 
$this->toArray(), $query );
 
-   if ( !$response->isOK() ) {
+   if ( !$response->isOk() ) {
throw new \Exception( $response->hasError()
? 'Failed reindex request: ' . 
$response->getErrorMessage()
: 'Unknown reindex failure: ' . 
$response->getStatus()
diff --git a/includes/Maintenance/ConfigUtils.php 
b/includes/Maintenance/ConfigUtils.php
index 4faf484..d2e10e2 100644
--- a/includes/Maintenance/ConfigUtils.php
+++ b/includes/Maintenance/ConfigUtils.php
@@ -107,7 +107,7 @@
public function getAllIndicesByType( $typeName ) {
$found = null;
$response = $this->client->request( $typeName . '*' );
-   if ( $response->isOK() ) {
+   if ( $response->isOK() && !$response->hasError() ) {
$found = array_keys( $response->getData() );
} else {
$this->error( "Cannot fetch index names for $typeName: "
@@ -228,7 +228,7 @@
public function getIndexHealth( $indexName ) {
$path = "_cluster/health/$indexName";
$response = $this->client->request( $path );
-   if ( $response->hasError() ) {
+   if ( !$response->isOk() || $response->hasError() ) {
throw new \Exception( "Error while fetching index 
health status: ". $response->getError() );
}
return $response->getData();
@@ -273,7 +273,7 @@
}
 
$response = $this->client->request( $indexName . '/_aliases' );
-   if ( $response->isOK() ) {
+   if ( $response->isOK()