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

CCE when re-throwing "shard not available" exception in TransportShardMultiGetAction #32185

Merged
merged 3 commits into from
Jul 23, 2018

Conversation

itsnotv
Copy link
Contributor

@itsnotv itsnotv commented Jul 19, 2018

ClassCastException can be thrown by callers of TransportActions.isShardNotAvailableException(e) as e is not always an instance of ElasticSearchException

fixes #32173

@itsnotv itsnotv changed the title fixes #32173 CCE when re-throwing "shard not available" exception in TransportShardMultiGetAction #32173 Jul 19, 2018
@itsnotv itsnotv changed the title CCE when re-throwing "shard not available" exception in TransportShardMultiGetAction #32173 CCE when re-throwing "shard not available" exception in TransportShardMultiGetAction Jul 19, 2018
@romseygeek romseygeek added the :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. label Jul 19, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@bleskes
Copy link
Contributor

bleskes commented Jul 19, 2018

Thanks @itsnotv . How about doing it like this:

Index: server/src/main/java/org/elasticsearch/action/get/TransportShardMultiGetAction.java
<+>UTF-8
===================================================================
--- server/src/main/java/org/elasticsearch/action/get/TransportShardMultiGetAction.java	(date 1531906244000)
+++ server/src/main/java/org/elasticsearch/action/get/TransportShardMultiGetAction.java	(date 1531986507000)
@@ -20,7 +20,6 @@
 package org.elasticsearch.action.get;
 
 import org.apache.logging.log4j.message.ParameterizedMessage;
-import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.action.support.ActionFilters;
 import org.elasticsearch.action.support.TransportActions;
 import org.elasticsearch.action.support.single.shard.TransportSingleShardAction;
@@ -90,9 +89,9 @@
                 GetResult getResult = indexShard.getService().get(item.type(), item.id(), item.storedFields(), request.realtime(), item.version(),
                     item.versionType(), item.fetchSourceContext());
                 response.add(request.locations.get(i), new GetResponse(getResult));
-            } catch (Exception e) {
+            } catch (RuntimeException e) {
                 if (TransportActions.isShardNotAvailableException(e)) {
-                    throw (ElasticsearchException) e;
+                    throw e;
                 } else {
                     logger.debug(() -> new ParameterizedMessage("{} failed to execute multi_get for [{}]/[{}]", shardId,
                         item.type(), item.id()), e);

Also note that there are other places in the code that do this (see last comment on #32173 ).

Last - do you mind adapting the issue description to be more complete?

Thanks again for working on this.

@itsnotv
Copy link
Contributor Author

itsnotv commented Jul 19, 2018

@bleskes updated the PR.

@bleskes
Copy link
Contributor

bleskes commented Jul 22, 2018

@elasticmachine test this please

@dnhatn
Copy link
Member

dnhatn commented Jul 22, 2018

[ant:checkstyle] Running Checkstyle 8.10.1 on 2864 files
[ant:checkstyle] [ERROR] /var/lib/jenkins/workspace/elastic+elasticsearch+pull-request/server/src/main/java/org/elasticsearch/action/get/TransportShardMultiGetAction.java:23:8: Unused import - org.elasticsearch.ElasticsearchException. [UnusedImports]
[ant:checkstyle] [ERROR] /var/lib/jenkins/workspace/elastic+elasticsearch+pull-request/server/src/main/java/org/elasticsearch/action/termvectors/TransportShardMultiTermsVectorAction.java:23:8: Unused import - org.elasticsearch.ElasticsearchException. [UnusedImports]

@itsnotv Can you please remove unused imports in TransportShardMultiGetAction.java and TransportShardMultiTermsVectorAction.java? It would be great if you run ./gradlew precommit before pushing :).

@itsnotv
Copy link
Contributor Author

itsnotv commented Jul 22, 2018

@dnhatn done.

$ gradle precommit
BUILD SUCCESSFUL in 1h 6m 58s
2053 actionable tasks: 2053 executed

@dnhatn
Copy link
Member

dnhatn commented Jul 22, 2018

Thanks @itsnotv.

@elasticmachine test this please.

Copy link
Contributor

@bleskes bleskes left a comment

Choose a reason for hiding this comment

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

LGTM. Thx @itsnotv

@bleskes bleskes merged commit 4b3284f into elastic:master Jul 23, 2018
bleskes pushed a commit that referenced this pull request Jul 23, 2018
…dMultiGetAction (#32185)

ClassCastException can be thrown by callers of TransportActions.isShardNotAvailableException(e) as e is not always an instance of ElasticSearchException

fixes #32173
dnhatn added a commit that referenced this pull request Jul 25, 2018
* 6.x:
  Security: revert to old way of merging automata (#32254)
  Fix a test bug in RangeQueryBuilderTests introduced in the field aliases backport.
  Introduce Application Privileges with support for Kibana RBAC (#32309)
  Undo a debugging change that snuck in during the field aliases merge.
  [test] port linux package packaging tests (#31943)
  Painless: Update More Methods to New Naming Scheme (#32305)
  Tribe: Add error with secure settings copied to tribe (#32298)
  Add V_6_3_3 version constant
  Add ERR to ranking evaluation documentation (#32314)
  [DOCS] Added link to 6.3.2 RNs
  [DOCS] Updates 6.3.2 release notes with PRs from ml-cpp repo (#32334)
  [Kerberos] Add Kerberos authentication support (#32263)
  [ML] Extract persistent task methods from MlMetadata (#32319)
  Backport - Add Snapshots Status API to High Level Rest Client (#32295)
  Make release notes ignore the `>test-failure` label. (#31309)
  [DOCS] Adds release highlights for search for 6.4 (#32095)
  Allow Integ Tests to run in a FIPS-140 JVM (#32316)
  Add support for field aliases to 6.x. (#32184)
  Register ERR metric with NamedXContentRegistry (#32320)
  fixes broken build for third-party-tests (#32315) Relates #31918 / Closes infra/issues/6085
  [DOCS] Rollup Caps API incorrectly mentions GET Jobs API (#32280)
  Rest HL client: Add put watch action (#32026) (#32191)
  Add WeightedAvg metric aggregation (#31037)
  Consistent encoder names (#29492)
  Switch monitoring to new style Requests (#32255)
  specify subdirs of lib, bin, modules in package (#32253)
  Rename ranking evaluation `quality_level` to `metric_score` (#32168)
  Add new permission for JDK11 to load JAAS libraries (#32132)
  Switch x-pack:core to new style Requests (#32252)
  Watcher: Store username on watch execution (#31873)
  Silence SSL reload test that fails on JDK 11
  Painless: Clean up add methods in PainlessLookup (#32258)
  CCE when re-throwing "shard not available" exception in TransportShardMultiGetAction (#32185)
  Fail shard if IndexShard#storeStats runs into an IOException (#32241)
  Fix `range` queries on `_type` field for singe type indices (#31756) (#32161)
  AwaitsFix RecoveryIT#testHistoryUUIDIsGenerated
  Add new fields to monitoring template for Beats state (#32085) (#32273)
  [TEST] improve REST high-level client naming conventions check (#32244)
  Check that client methods match API defined in the REST spec (#31825)
dnhatn added a commit that referenced this pull request Jul 25, 2018
* master:
  Security: revert to old way of merging automata (#32254)
  Networking: Fix test leaking buffer (#32296)
  Undo a debugging change that snuck in during the field aliases merge.
  Painless: Update More Methods to New Naming Scheme (#32305)
  [TEST] Fix assumeFalse -> assumeTrue in SSLReloadIntegTests
  Ingest: Support integer and long hex values in convert (#32213)
  Introduce fips_mode setting and associated checks (#32326)
  Add V_6_3_3 version constant
  [DOCS] Removed extraneous callout number.
  Rest HL client: Add put license action (#32214)
  Add ERR to ranking evaluation documentation (#32314)
  Introduce Application Privileges with support for Kibana RBAC (#32309)
  Build: Shadow x-pack:protocol into x-pack:plugin:core (#32240)
  [Kerberos] Add Kerberos authentication support (#32263)
  [ML] Extract persistent task methods from MlMetadata (#32319)
  Add Restore Snapshot High Level REST API
  Register ERR metric with NamedXContentRegistry (#32320)
  fixes broken build for third-party-tests (#32315)
  Allow Integ Tests to run in a FIPS-140 JVM (#31989)
  [DOCS] Rollup Caps API incorrectly mentions GET Jobs API (#32280)
  awaitsfix testRandomClusterStateUpdates
  [TEST] add version skip to weighted_avg tests
  Consistent encoder names (#29492)
  Add WeightedAvg metric aggregation (#31037)
  Switch monitoring to new style Requests (#32255)
  Rename ranking evaluation `quality_level` to `metric_score` (#32168)
  Fix a test bug around nested aggregations and field aliases. (#32287)
  Add new permission for JDK11 to load JAAS libraries (#32132)
  Silence SSL reload test that fails on JDK 11
  [test] package pre-install java check (#32259)
  specify subdirs of lib, bin, modules in package (#32253)
  Switch x-pack:core to new style Requests (#32252)
  awaitsfix SSLConfigurationReloaderTests
  Painless: Clean up add methods in PainlessLookup (#32258)
  Fail shard if IndexShard#storeStats runs into an IOException (#32241)
  AwaitsFix RecoveryIT#testHistoryUUIDIsGenerated
  Remove unnecessary warning supressions (#32250)
  CCE when re-throwing "shard not available" exception in TransportShardMultiGetAction (#32185)
  Add new fields to monitoring template for Beats state (#32085)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. v6.4.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CCE when re-throwing "shard not available" exception in TransportShardMultiGetAction
6 participants