diff options
author | Chris Earle <pickypg@users.noreply.github.com> | 2017-06-02 10:01:42 -0400 |
---|---|---|
committer | GitHub <noreply@github.com> | 2017-06-02 10:01:42 -0400 |
commit | 6464add55138c7e4be324b335ff1859356dc507b (patch) | |
tree | c493083ac65737e6c55d48670782774f36c09d26 /core | |
parent | 5f3ed99c7114e3747991d6d77d341299fd24658c (diff) |
Always Accumulate Transport Exceptions (#25017)
This removes the `accumulateExceptions()` method (and its usage) from `TransportNodesAction` and `TransportTasksAction`, forcing both transport actions to always accumulate exceptions.
Without this change, some transport actions, like `TransportNodesStatsAction` would respond in very unexpected ways by returning no response due to some failure, but instead of returning an
error the response would simply be empty: no response and no error.
This results in a very trappy response structure where users can check for an error, then attempt to blindly use the response when no error is returned.
Diffstat (limited to 'core')
16 files changed, 4 insertions, 85 deletions
diff --git a/core/src/main/java/org/elasticsearch/action/admin/cluster/node/hotthreads/TransportNodesHotThreadsAction.java b/core/src/main/java/org/elasticsearch/action/admin/cluster/node/hotthreads/TransportNodesHotThreadsAction.java index da45a3e402..7b43d1c259 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/cluster/node/hotthreads/TransportNodesHotThreadsAction.java +++ b/core/src/main/java/org/elasticsearch/action/admin/cluster/node/hotthreads/TransportNodesHotThreadsAction.java @@ -24,7 +24,6 @@ import org.elasticsearch.action.FailedNodeException; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.nodes.BaseNodeRequest; import org.elasticsearch.action.support.nodes.TransportNodesAction; -import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.inject.Inject; @@ -82,11 +81,6 @@ public class TransportNodesHotThreadsAction extends TransportNodesAction<NodesHo } } - @Override - protected boolean accumulateExceptions() { - return false; - } - public static class NodeRequest extends BaseNodeRequest { NodesHotThreadsRequest request; diff --git a/core/src/main/java/org/elasticsearch/action/admin/cluster/node/info/TransportNodesInfoAction.java b/core/src/main/java/org/elasticsearch/action/admin/cluster/node/info/TransportNodesInfoAction.java index 7d80b84d5d..afe535601f 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/cluster/node/info/TransportNodesInfoAction.java +++ b/core/src/main/java/org/elasticsearch/action/admin/cluster/node/info/TransportNodesInfoAction.java @@ -76,11 +76,6 @@ public class TransportNodesInfoAction extends TransportNodesAction<NodesInfoRequ request.transport(), request.http(), request.plugins(), request.ingest(), request.indices()); } - @Override - protected boolean accumulateExceptions() { - return false; - } - public static class NodeInfoRequest extends BaseNodeRequest { NodesInfoRequest request; diff --git a/core/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/TransportNodesStatsAction.java b/core/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/TransportNodesStatsAction.java index e4034582f9..56c98ed7db 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/TransportNodesStatsAction.java +++ b/core/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/TransportNodesStatsAction.java @@ -76,11 +76,6 @@ public class TransportNodesStatsAction extends TransportNodesAction<NodesStatsRe request.ingest()); } - @Override - protected boolean accumulateExceptions() { - return false; - } - public static class NodeStatsRequest extends BaseNodeRequest { NodesStatsRequest request; diff --git a/core/src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/cancel/TransportCancelTasksAction.java b/core/src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/cancel/TransportCancelTasksAction.java index 50786d3676..aca1be7adf 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/cancel/TransportCancelTasksAction.java +++ b/core/src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/cancel/TransportCancelTasksAction.java @@ -167,12 +167,6 @@ public class TransportCancelTasksAction extends TransportTasksAction<Cancellable } } - - @Override - protected boolean accumulateExceptions() { - return true; - } - private void setBanOnNodes(String reason, CancellableTask task, DiscoveryNodes nodes, ActionListener<Void> listener) { sendSetBanRequest(nodes, BanParentTaskRequest.createSetBanParentTaskRequest(new TaskId(clusterService.localNode().getId(), task.getId()), reason), diff --git a/core/src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/list/TransportListTasksAction.java b/core/src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/list/TransportListTasksAction.java index 889628e373..eb8a6ad4ca 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/list/TransportListTasksAction.java +++ b/core/src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/list/TransportListTasksAction.java @@ -90,8 +90,4 @@ public class TransportListTasksAction extends TransportTasksAction<Task, ListTas super.processTasks(request, operation); } - @Override - protected boolean accumulateExceptions() { - return true; - } } diff --git a/core/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/TransportNodesSnapshotsStatus.java b/core/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/TransportNodesSnapshotsStatus.java index 71a709f0b5..872793f6ef 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/TransportNodesSnapshotsStatus.java +++ b/core/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/TransportNodesSnapshotsStatus.java @@ -122,11 +122,6 @@ public class TransportNodesSnapshotsStatus extends TransportNodesAction<Transpor } } - @Override - protected boolean accumulateExceptions() { - return true; - } - public static class Request extends BaseNodesRequest<Request> { private Snapshot[] snapshots; diff --git a/core/src/main/java/org/elasticsearch/action/admin/cluster/stats/TransportClusterStatsAction.java b/core/src/main/java/org/elasticsearch/action/admin/cluster/stats/TransportClusterStatsAction.java index d77bc59925..57eeb2d5eb 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/cluster/stats/TransportClusterStatsAction.java +++ b/core/src/main/java/org/elasticsearch/action/admin/cluster/stats/TransportClusterStatsAction.java @@ -118,11 +118,6 @@ public class TransportClusterStatsAction extends TransportNodesAction<ClusterSta } - @Override - protected boolean accumulateExceptions() { - return false; - } - public static class ClusterStatsNodeRequest extends BaseNodeRequest { ClusterStatsRequest request; diff --git a/core/src/main/java/org/elasticsearch/action/support/nodes/TransportNodesAction.java b/core/src/main/java/org/elasticsearch/action/support/nodes/TransportNodesAction.java index d8010f4381..4583e47bc1 100644 --- a/core/src/main/java/org/elasticsearch/action/support/nodes/TransportNodesAction.java +++ b/core/src/main/java/org/elasticsearch/action/support/nodes/TransportNodesAction.java @@ -106,16 +106,11 @@ public abstract class TransportNodesAction<NodesRequest extends BaseNodesRequest final List<NodeResponse> responses = new ArrayList<>(); final List<FailedNodeException> failures = new ArrayList<>(); - final boolean accumulateExceptions = accumulateExceptions(); for (int i = 0; i < nodesResponses.length(); ++i) { Object response = nodesResponses.get(i); if (response instanceof FailedNodeException) { - if (accumulateExceptions) { - failures.add((FailedNodeException)response); - } else { - logger.warn("not accumulating exceptions, excluding exception from response", (FailedNodeException)response); - } + failures.add((FailedNodeException)response); } else { responses.add(nodeResponseClass.cast(response)); } @@ -145,8 +140,6 @@ public abstract class TransportNodesAction<NodesRequest extends BaseNodesRequest return nodeOperation(request); } - protected abstract boolean accumulateExceptions(); - /** * resolve node ids to concrete nodes of the incoming request **/ diff --git a/core/src/main/java/org/elasticsearch/action/support/tasks/TransportTasksAction.java b/core/src/main/java/org/elasticsearch/action/support/tasks/TransportTasksAction.java index 86d158784c..35b2b41dfd 100644 --- a/core/src/main/java/org/elasticsearch/action/support/tasks/TransportTasksAction.java +++ b/core/src/main/java/org/elasticsearch/action/support/tasks/TransportTasksAction.java @@ -226,8 +226,6 @@ public abstract class TransportTasksAction< return false; } - protected abstract boolean accumulateExceptions(); - private class AsyncAction { private final TasksRequest request; @@ -321,9 +319,9 @@ public abstract class TransportTasksAction< (org.apache.logging.log4j.util.Supplier<?>) () -> new ParameterizedMessage("failed to execute on node [{}]", nodeId), t); } - if (accumulateExceptions()) { - responses.set(idx, new FailedNodeException(nodeId, "Failed node [" + nodeId + "]", t)); - } + + responses.set(idx, new FailedNodeException(nodeId, "Failed node [" + nodeId + "]", t)); + if (counter.incrementAndGet() == responses.length()) { finishHim(); } diff --git a/core/src/main/java/org/elasticsearch/gateway/TransportNodesListGatewayMetaState.java b/core/src/main/java/org/elasticsearch/gateway/TransportNodesListGatewayMetaState.java index 13c317c53e..4247ec131a 100644 --- a/core/src/main/java/org/elasticsearch/gateway/TransportNodesListGatewayMetaState.java +++ b/core/src/main/java/org/elasticsearch/gateway/TransportNodesListGatewayMetaState.java @@ -100,11 +100,6 @@ public class TransportNodesListGatewayMetaState extends TransportNodesAction<Tra } } - @Override - protected boolean accumulateExceptions() { - return true; - } - public static class Request extends BaseNodesRequest<Request> { public Request() { diff --git a/core/src/main/java/org/elasticsearch/gateway/TransportNodesListGatewayStartedShards.java b/core/src/main/java/org/elasticsearch/gateway/TransportNodesListGatewayStartedShards.java index f2b046acd9..11df875d4d 100644 --- a/core/src/main/java/org/elasticsearch/gateway/TransportNodesListGatewayStartedShards.java +++ b/core/src/main/java/org/elasticsearch/gateway/TransportNodesListGatewayStartedShards.java @@ -172,11 +172,6 @@ public class TransportNodesListGatewayStartedShards extends } } - @Override - protected boolean accumulateExceptions() { - return true; - } - public static class Request extends BaseNodesRequest<Request> { private ShardId shardId; diff --git a/core/src/main/java/org/elasticsearch/indices/store/TransportNodesListShardStoreMetaData.java b/core/src/main/java/org/elasticsearch/indices/store/TransportNodesListShardStoreMetaData.java index 84d3354ba4..404a19b0ab 100644 --- a/core/src/main/java/org/elasticsearch/indices/store/TransportNodesListShardStoreMetaData.java +++ b/core/src/main/java/org/elasticsearch/indices/store/TransportNodesListShardStoreMetaData.java @@ -159,11 +159,6 @@ public class TransportNodesListShardStoreMetaData extends TransportNodesAction<T } } - @Override - protected boolean accumulateExceptions() { - return true; - } - public static class StoreFilesMetaData implements Iterable<StoreFileMetaData>, Streamable { private ShardId shardId; Store.MetadataSnapshot metadataSnapshot; diff --git a/core/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/TaskManagerTestCase.java b/core/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/TaskManagerTestCase.java index 0cece76425..fdd5091485 100644 --- a/core/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/TaskManagerTestCase.java +++ b/core/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/TaskManagerTestCase.java @@ -164,10 +164,6 @@ public abstract class TaskManagerTestCase extends ESTestCase { @Override protected abstract NodeResponse nodeOperation(NodeRequest request); - @Override - protected boolean accumulateExceptions() { - return true; - } } public static class TestNode implements Releasable { diff --git a/core/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/TestTaskPlugin.java b/core/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/TestTaskPlugin.java index b4ba0354ed..ec981442b5 100644 --- a/core/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/TestTaskPlugin.java +++ b/core/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/TestTaskPlugin.java @@ -313,10 +313,6 @@ public class TestTaskPlugin extends Plugin implements ActionPlugin { throw new UnsupportedOperationException("the task parameter is required"); } - @Override - protected boolean accumulateExceptions() { - return true; - } } public static class TestTaskAction extends Action<NodesRequest, NodesResponse, NodesRequestBuilder> { @@ -453,10 +449,6 @@ public class TestTaskPlugin extends Plugin implements ActionPlugin { listener.onResponse(new UnblockTestTaskResponse()); } - @Override - protected boolean accumulateExceptions() { - return true; - } } public static class UnblockTestTasksAction extends Action<UnblockTestTasksRequest, UnblockTestTasksResponse, diff --git a/core/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/TransportTasksActionTests.java b/core/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/TransportTasksActionTests.java index 4e624164fa..3f9792e32f 100644 --- a/core/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/TransportTasksActionTests.java +++ b/core/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/TransportTasksActionTests.java @@ -270,10 +270,6 @@ public class TransportTasksActionTests extends TaskManagerTestCase { return new TestTaskResponse(in); } - @Override - protected boolean accumulateExceptions() { - return true; - } } private ActionFuture<NodesResponse> startBlockingTestNodesAction(CountDownLatch checkLatch) throws InterruptedException { diff --git a/core/src/test/java/org/elasticsearch/action/support/nodes/TransportNodesActionTests.java b/core/src/test/java/org/elasticsearch/action/support/nodes/TransportNodesActionTests.java index c3ca62616f..7d471f77f8 100644 --- a/core/src/test/java/org/elasticsearch/action/support/nodes/TransportNodesActionTests.java +++ b/core/src/test/java/org/elasticsearch/action/support/nodes/TransportNodesActionTests.java @@ -57,7 +57,6 @@ import java.util.function.Supplier; import static org.elasticsearch.test.ClusterServiceUtils.createClusterService; import static org.elasticsearch.test.ClusterServiceUtils.setState; -import static org.mockito.Mockito.mock; public class TransportNodesActionTests extends ESTestCase { @@ -275,10 +274,6 @@ public class TransportNodesActionTests extends ESTestCase { return new TestNodeResponse(); } - @Override - protected boolean accumulateExceptions() { - return false; - } } private static class DataNodesOnlyTransportNodesAction |