Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cross Cluster Search: preserve remote status code #30976

Merged
merged 4 commits into from
Jun 1, 2018

Conversation

javanna
Copy link
Member

@javanna javanna commented May 30, 2018

In case an error is returned when calling search_shards on a remote
cluster, which will lead to throwing an exception in the coordinating
node, we should make sure that the status code returned by the
coordinating node is the same as the one returned by the remote
cluster. Up until now a 500 - Internal Server Error was always
returned. This commit changes this behaviour so that for instance if an
index is not found, which causes an 404, a 404 is also returned by the
coordinating node to the client.

Closes #27461

In case an error is returned when calling search_shards on a remote
cluster, which will lead to throwing an exception in the coordinating
 node, we should make sure that the status code returned by the
 coordinating node is the same as the one returned by the remote
 cluster. Up until now a 500 - Internal Server Error was always
 returned. This commit changes this behaviour so that for instance if an
 index is not found, which causes an 404, a 404 is also returned by the
 coordinating node to the client.

 Closes elastic#27461
@javanna javanna added >bug :Search/Search Search-related issues that do not fall into other categories v7.0.0 v6.4.0 v6.3.1 labels May 30, 2018
@javanna javanna requested a review from s1monw May 30, 2018 19:03
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@@ -215,7 +215,7 @@ public void collectSearchShards(IndicesOptions indicesOptions, String preference
ActionListener<Map<String, ClusterSearchShardsResponse>> listener) {
final CountDown responsesCountDown = new CountDown(remoteIndicesByCluster.size());
final Map<String, ClusterSearchShardsResponse> searchShardsResponses = new ConcurrentHashMap<>();
final AtomicReference<TransportException> transportException = new AtomicReference<>();
final AtomicReference<RemoteTransportException> transportException = new AtomicReference<>();
Copy link
Member Author

@javanna javanna May 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RemoteTransportException implements ElasticsearchWrapperException, then unwrapCause will unwrap the inner cause and status() will return the inner status see https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/ElasticsearchException.java#L235

This behaviour is tricky. I think how ElasticsearchException#status works may cause problems when for instance it is thrown in the high-level REST client, I will have to dig deeper on that and probably open a follow-up PR, but I think this change is enough for cross-cluster search.

Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@javanna
Copy link
Member Author

javanna commented May 31, 2018

retest this please

@javanna javanna merged commit 70749e0 into elastic:master Jun 1, 2018
javanna added a commit that referenced this pull request Jun 1, 2018
In case an error is returned when calling search_shards on a remote
cluster, which will lead to throwing an exception in the coordinating
 node, we should make sure that the status code returned by the
 coordinating node is the same as the one returned by the remote
 cluster. Up until now a 500 - Internal Server Error was always
 returned. This commit changes this behaviour so that for instance if an
 index is not found, which causes an 404, a 404 is also returned by the
 coordinating node to the client.

 Closes #27461
dnhatn added a commit that referenced this pull request Jun 2, 2018
* 6.x:
  Adjust BWC version on client features
  Introduce client feature tracking (#31020)
  [DOCS] Make geoshape docs less memory hungry (#31014)
  Fix handling of percent-encoded spaces in Windows batch files (#31034)
  [Docs] Fix a typo in Create Index naming limitation (#30891)
  REST high-level client: add delete ingest pipeline API (#30865)
  Ensure that index_prefixes settings cannot be changed (#30967)
  REST high-level client: add get ingest pipeline API (#30847)
  Cross Cluster Search: preserve remote status code (#30976)
  High-level client: list tasks failure to not lose nodeId (#31001)
  Refactor Sniffer and make it testable (#29638)
  [ML][TEST] Fix bucket count assertion in all tests in ModelPlotsIT (#31026)
  Add an option to split keyword field on whitespace at query time (#30691)
  Allow rollup job creation only if cluster is x-pack ready (#30963)
  Fix interoperability with < 6.3 transport clients (#30971)
  [Tests] Fix alias names in PutIndexTemplateRequestTests (#30960)
  [DOCS] Fixes links (#31011)
  Watcher: Give test a little more time
dnhatn added a commit that referenced this pull request Jun 2, 2018
* master:
  Avoid randomization bug in FeatureAwareTests
  Adjust BWC version on client features
  Add TRACE, CONNECT, and PATCH http methods (#31035)
  Adjust BWC version on client features
  [DOCS] Make geoshape docs less memory hungry (#31014)
  Fix handling of percent-encoded spaces in Windows batch files (#31034)
  [Docs] Fix a typo in Create Index naming limitation (#30891)
  Introduce client feature tracking (#31020)
  Ensure that index_prefixes settings cannot be changed (#30967)
  REST high-level client: add delete ingest pipeline API (#30865)
  [ML][TEST] Fix bucket count assertion in all tests in ModelPlotsIT (#31026)
  Allow rollup job creation only if cluster is x-pack ready (#30963)
  Fix interoperability with < 6.3 transport clients (#30971)
  Add an option to split keyword field on whitespace at query time (#30691)
  [Tests] Fix alias names in PutIndexTemplateRequestTests (#30960)
  REST high-level client: add get ingest pipeline API (#30847)
  Cross Cluster Search: preserve remote status code (#30976)
  High-level client: list tasks failure to not lose nodeId (#31001)
  [DOCS] Fixes links (#31011)
  Watcher: Give test a little more time
  Reuse expiration date of trial licenses (#30950)
  Remove unused query methods from MappedFieldType. (#30987)
  Transport client: Don't validate node in handshake (#30737)
  [DOCS] Clarify not all PKCS12 usable as truststores (#30750)
  HLRest: Allow caller to set per request options (#30490)
  Remove version read/write logic in Verify Response (#30879)
  [DOCS] Update readme for testing x-pack code snippets (#30696)
  Ensure intended key is selected in SamlAuthenticatorTests (#30993)
  Core: Remove RequestBuilder from Action (#30966)
@jpountz
Copy link
Contributor

jpountz commented Jun 13, 2018

@javanna This may be backported now.

javanna added a commit that referenced this pull request Jun 14, 2018
In case an error is returned when calling search_shards on a remote
cluster, which will lead to throwing an exception in the coordinating
 node, we should make sure that the status code returned by the
 coordinating node is the same as the one returned by the remote
 cluster. Up until now a 500 - Internal Server Error was always
 returned. This commit changes this behaviour so that for instance if an
 index is not found, which causes an 404, a 404 is also returned by the
 coordinating node to the client.

 Closes #27461
@javanna
Copy link
Member Author

javanna commented Jun 14, 2018

@jpountz done thanks a lot for the pings ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Search Search-related issues that do not fall into other categories v6.3.1 v6.4.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants