From c70c44005088d4bf0eb7fc5d6f8660c25958a785 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 30 Jun 2017 17:50:20 -0400 Subject: Adjust status on bad allocation explain requests When a user requests a cluster allocation explain in a situation where it does not make sense (for example, there are no unassigned shards), we should consider this a bad request instead of a server error. Yet, today by throwing an illegal state exception, these are treated as server errors. This commit adjusts these so that they throw illegal argument exceptions and are treated as bad requests. Relates #25503 --- .../TransportClusterAllocationExplainAction.java | 9 +++++---- .../allocation/ClusterAllocationExplainActionTests.java | 4 ++-- .../test/cluster.allocation_explain/10_basic.yml | 15 ++++++++++----- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/action/admin/cluster/allocation/TransportClusterAllocationExplainAction.java b/core/src/main/java/org/elasticsearch/action/admin/cluster/allocation/TransportClusterAllocationExplainAction.java index 4d4796aaf3..77d4b24d8c 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/cluster/allocation/TransportClusterAllocationExplainAction.java +++ b/core/src/main/java/org/elasticsearch/action/admin/cluster/allocation/TransportClusterAllocationExplainAction.java @@ -139,7 +139,7 @@ public class TransportClusterAllocationExplainAction foundShard = ui.next(); } if (foundShard == null) { - throw new IllegalStateException("unable to find any unassigned shards to explain [" + request + "]"); + throw new IllegalArgumentException("unable to find any unassigned shards to explain [" + request + "]"); } } else { String index = request.getIndex(); @@ -151,7 +151,8 @@ public class TransportClusterAllocationExplainAction DiscoveryNode primaryNode = allocation.nodes().resolveNode(request.getCurrentNode()); // the primary is assigned to a node other than the node specified in the request if (primaryNode.getId().equals(foundShard.currentNodeId()) == false) { - throw new IllegalStateException("unable to find primary shard assigned to node [" + request.getCurrentNode() + "]"); + throw new IllegalArgumentException( + "unable to find primary shard assigned to node [" + request.getCurrentNode() + "]"); } } } else { @@ -168,7 +169,7 @@ public class TransportClusterAllocationExplainAction } } if (foundShard == null) { - throw new IllegalStateException("unable to find a replica shard assigned to node [" + + throw new IllegalArgumentException("unable to find a replica shard assigned to node [" + request.getCurrentNode() + "]"); } } else { @@ -193,7 +194,7 @@ public class TransportClusterAllocationExplainAction } if (foundShard == null) { - throw new IllegalStateException("unable to find any shards to explain [" + request + "] in the routing table"); + throw new IllegalArgumentException("unable to find any shards to explain [" + request + "] in the routing table"); } return foundShard; } diff --git a/core/src/test/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainActionTests.java b/core/src/test/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainActionTests.java index 0f8f71a3af..f9785df649 100644 --- a/core/src/test/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainActionTests.java +++ b/core/src/test/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainActionTests.java @@ -108,7 +108,7 @@ public class ClusterAllocationExplainActionTests extends ESTestCase { final ClusterState allStartedClusterState = ClusterStateCreationUtils.state("idx", randomBoolean(), ShardRoutingState.STARTED, ShardRoutingState.STARTED); final ClusterAllocationExplainRequest anyUnassignedShardsRequest = new ClusterAllocationExplainRequest(); - expectThrows(IllegalStateException.class, () -> + expectThrows(IllegalArgumentException.class, () -> findShardToExplain(anyUnassignedShardsRequest, routingAllocation(allStartedClusterState))); } @@ -161,7 +161,7 @@ public class ClusterAllocationExplainActionTests extends ESTestCase { } } final ClusterAllocationExplainRequest failingRequest = new ClusterAllocationExplainRequest("idx", 0, primary, explainNode); - expectThrows(IllegalStateException.class, () -> findShardToExplain(failingRequest, allocation)); + expectThrows(IllegalArgumentException.class, () -> findShardToExplain(failingRequest, allocation)); } private static RoutingAllocation routingAllocation(ClusterState clusterState) { diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.allocation_explain/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.allocation_explain/10_basic.yml index 6ec8651fe8..5e1bc42ec4 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.allocation_explain/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.allocation_explain/10_basic.yml @@ -1,14 +1,19 @@ ---- -"cluster shard allocation explanation test": +"bad cluster shard allocation explanation request": - skip: - version: " - 5.1.99" - reason: explain API response output was changed starting in 5.2.0 + version: " - 5.99.99" + reason: response status on bad request was changed starting in 5.6.0 - do: # there aren't any unassigned shards to explain - catch: /illegal_state_exception/ + catch: /illegal_argument_exception/ cluster.allocation_explain: {} +--- +"cluster shard allocation explanation test": + - skip: + version: " - 5.1.99" + reason: explain API response output was changed starting in 5.2.0 + - do: indices.create: index: test -- cgit v1.2.3