From 6464add55138c7e4be324b335ff1859356dc507b Mon Sep 17 00:00:00 2001
From: Chris Earle <pickypg@users.noreply.github.com>
Date: Fri, 2 Jun 2017 10:01:42 -0400
Subject: [PATCH] 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.
---
 .../node/hotthreads/TransportNodesHotThreadsAction.java  | 6 ------
 .../cluster/node/info/TransportNodesInfoAction.java      | 5 -----
 .../cluster/node/stats/TransportNodesStatsAction.java    | 5 -----
 .../node/tasks/cancel/TransportCancelTasksAction.java    | 6 ------
 .../node/tasks/list/TransportListTasksAction.java        | 4 ----
 .../snapshots/status/TransportNodesSnapshotsStatus.java  | 5 -----
 .../admin/cluster/stats/TransportClusterStatsAction.java | 5 -----
 .../action/support/nodes/TransportNodesAction.java       | 9 +--------
 .../action/support/tasks/TransportTasksAction.java       | 8 +++-----
 .../gateway/TransportNodesListGatewayMetaState.java      | 5 -----
 .../gateway/TransportNodesListGatewayStartedShards.java  | 5 -----
 .../store/TransportNodesListShardStoreMetaData.java      | 5 -----
 .../admin/cluster/node/tasks/TaskManagerTestCase.java    | 4 ----
 .../action/admin/cluster/node/tasks/TestTaskPlugin.java  | 8 --------
 .../cluster/node/tasks/TransportTasksActionTests.java    | 4 ----
 .../action/support/nodes/TransportNodesActionTests.java  | 5 -----
 .../index/reindex/TransportRethrottleAction.java         | 4 ----
 17 files changed, 4 insertions(+), 89 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 da45a3e4027..7b43d1c259b 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 7d80b84d5d2..afe535601fc 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 e4034582f96..56c98ed7db0 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 50786d36766..aca1be7adff 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 889628e373a..eb8a6ad4ca5 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 71a709f0b5b..872793f6ef2 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 d77bc599258..57eeb2d5eb4 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 d8010f4381f..4583e47bc1d 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 86d158784c0..35b2b41dfda 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 13c317c53e9..4247ec131a3 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 f2b046acd97..11df875d4dd 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 84d3354ba4e..404a19b0ab3 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 0cece76425d..fdd5091485b 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 b4ba0354ed7..ec981442b57 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 4e624164fa0..3f9792e32f4 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 c3ca62616fd..7d471f77f83 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
diff --git a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/TransportRethrottleAction.java b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/TransportRethrottleAction.java
index 0901e5ade31..bcfb2813474 100644
--- a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/TransportRethrottleAction.java
+++ b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/TransportRethrottleAction.java
@@ -87,8 +87,4 @@ public class TransportRethrottleAction extends TransportTasksAction<BulkByScroll
         return new ListTasksResponse(tasks, taskOperationFailures, failedNodeExceptions);
     }
 
-    @Override
-    protected boolean accumulateExceptions() {
-        return true;
-    }
 }