From 3f94a566ff5005e4c4819706f3032a995dfec97d Mon Sep 17 00:00:00 2001
From: Simon Willnauer <simonw@apache.org>
Date: Mon, 2 Nov 2015 10:15:49 +0100
Subject: [PATCH] Deduplicate cause if already contained in shard failures

If we have a shard failure on SearchPhaseExecutionException
we can deduplicate the original cause and use the more informative
ShardSearchFailure containing the shard ID etc. but we should deduplicate
the actual cause to prevent stack trace duplication.
---
 .../search/SearchPhaseExecutionException.java | 26 ++++++++++++++-----
 ...nsportSearchScrollQueryAndFetchAction.java |  4 +--
 ...sportSearchScrollQueryThenFetchAction.java |  8 +++---
 .../type/TransportSearchTypeAction.java       |  6 +++--
 .../org/elasticsearch/ESExceptionTests.java   | 17 +++++++++++-
 .../index/query/TemplateQueryIT.java          |  2 +-
 6 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/core/src/main/java/org/elasticsearch/action/search/SearchPhaseExecutionException.java b/core/src/main/java/org/elasticsearch/action/search/SearchPhaseExecutionException.java
index 68bcf6d269c..40e8b0730ff 100644
--- a/core/src/main/java/org/elasticsearch/action/search/SearchPhaseExecutionException.java
+++ b/core/src/main/java/org/elasticsearch/action/search/SearchPhaseExecutionException.java
@@ -38,13 +38,11 @@ public class SearchPhaseExecutionException extends ElasticsearchException {
     private final ShardSearchFailure[] shardFailures;
 
     public SearchPhaseExecutionException(String phaseName, String msg, ShardSearchFailure[] shardFailures) {
-        super(msg);
-        this.phaseName = phaseName;
-        this.shardFailures = shardFailures;
+        this(phaseName, msg, null, shardFailures);
     }
 
     public SearchPhaseExecutionException(String phaseName, String msg, Throwable cause, ShardSearchFailure[] shardFailures) {
-        super(msg, cause);
+        super(msg, deduplicateCause(cause, shardFailures));
         this.phaseName = phaseName;
         this.shardFailures = shardFailures;
     }
@@ -63,12 +61,26 @@ public class SearchPhaseExecutionException extends ElasticsearchException {
     public void writeTo(StreamOutput out) throws IOException {
         super.writeTo(out);
         out.writeOptionalString(phaseName);
-        out.writeVInt(shardFailures == null ? 0 : shardFailures.length);
-        if (shardFailures != null) {
+        out.writeVInt(shardFailures.length);
+        for (ShardSearchFailure failure : shardFailures) {
+            failure.writeTo(out);
+        }
+    }
+
+    private static final Throwable deduplicateCause(Throwable cause, ShardSearchFailure[] shardFailures) {
+        if (shardFailures == null) {
+            throw new IllegalArgumentException("shardSearchFailures must not be null");
+        }
+        // if the cause of this exception is also the cause of one of the shard failures we don't add it
+        // to prevent duplication in stack traces rendered to the REST layer
+        if (cause != null) {
             for (ShardSearchFailure failure : shardFailures) {
-                failure.writeTo(out);
+                if (failure.getCause() == cause) {
+                    return null;
+                }
             }
         }
+        return cause;
     }
 
     @Override
diff --git a/core/src/main/java/org/elasticsearch/action/search/type/TransportSearchScrollQueryAndFetchAction.java b/core/src/main/java/org/elasticsearch/action/search/type/TransportSearchScrollQueryAndFetchAction.java
index cd4238ccdea..2a953f9b732 100644
--- a/core/src/main/java/org/elasticsearch/action/search/type/TransportSearchScrollQueryAndFetchAction.java
+++ b/core/src/main/java/org/elasticsearch/action/search/type/TransportSearchScrollQueryAndFetchAction.java
@@ -114,7 +114,7 @@ public class TransportSearchScrollQueryAndFetchAction extends AbstractComponent
 
         public void start() {
             if (scrollId.getContext().length == 0) {
-                listener.onFailure(new SearchPhaseExecutionException("query", "no nodes to search on", null));
+                listener.onFailure(new SearchPhaseExecutionException("query", "no nodes to search on", ShardSearchFailure.EMPTY_ARRAY));
                 return;
             }
 
@@ -175,7 +175,7 @@ public class TransportSearchScrollQueryAndFetchAction extends AbstractComponent
             successfulOps.decrementAndGet();
             if (counter.decrementAndGet() == 0) {
                 if (successfulOps.get() == 0) {
-                    listener.onFailure(new SearchPhaseExecutionException("query_fetch", "all shards failed", buildShardFailures()));
+                    listener.onFailure(new SearchPhaseExecutionException("query_fetch", "all shards failed", t, buildShardFailures()));
                 } else {
                     finishHim();
                 }
diff --git a/core/src/main/java/org/elasticsearch/action/search/type/TransportSearchScrollQueryThenFetchAction.java b/core/src/main/java/org/elasticsearch/action/search/type/TransportSearchScrollQueryThenFetchAction.java
index 85b06ea7860..8f2df714319 100644
--- a/core/src/main/java/org/elasticsearch/action/search/type/TransportSearchScrollQueryThenFetchAction.java
+++ b/core/src/main/java/org/elasticsearch/action/search/type/TransportSearchScrollQueryThenFetchAction.java
@@ -123,7 +123,7 @@ public class TransportSearchScrollQueryThenFetchAction extends AbstractComponent
 
         public void start() {
             if (scrollId.getContext().length == 0) {
-                listener.onFailure(new SearchPhaseExecutionException("query", "no nodes to search on", null));
+                listener.onFailure(new SearchPhaseExecutionException("query", "no nodes to search on", ShardSearchFailure.EMPTY_ARRAY));
                 return;
             }
             final AtomicInteger counter = new AtomicInteger(scrollId.getContext().length);
@@ -143,7 +143,7 @@ public class TransportSearchScrollQueryThenFetchAction extends AbstractComponent
                         try {
                             executeFetchPhase();
                         } catch (Throwable e) {
-                            listener.onFailure(new SearchPhaseExecutionException("query", "Fetch failed", e, null));
+                            listener.onFailure(new SearchPhaseExecutionException("query", "Fetch failed", e, ShardSearchFailure.EMPTY_ARRAY));
                             return;
                         }
                     }
@@ -181,12 +181,12 @@ public class TransportSearchScrollQueryThenFetchAction extends AbstractComponent
             successfulOps.decrementAndGet();
             if (counter.decrementAndGet() == 0) {
                 if (successfulOps.get() == 0) {
-                    listener.onFailure(new SearchPhaseExecutionException("query", "all shards failed", buildShardFailures()));
+                    listener.onFailure(new SearchPhaseExecutionException("query", "all shards failed", t, buildShardFailures()));
                 } else {
                     try {
                         executeFetchPhase();
                     } catch (Throwable e) {
-                        listener.onFailure(new SearchPhaseExecutionException("query", "Fetch failed", e, null));
+                        listener.onFailure(new SearchPhaseExecutionException("query", "Fetch failed", e, ShardSearchFailure.EMPTY_ARRAY));
                     }
                 }
             }
diff --git a/core/src/main/java/org/elasticsearch/action/search/type/TransportSearchTypeAction.java b/core/src/main/java/org/elasticsearch/action/search/type/TransportSearchTypeAction.java
index fa5776387dd..31cd3986d2f 100644
--- a/core/src/main/java/org/elasticsearch/action/search/type/TransportSearchTypeAction.java
+++ b/core/src/main/java/org/elasticsearch/action/search/type/TransportSearchTypeAction.java
@@ -220,17 +220,19 @@ public abstract class TransportSearchTypeAction extends TransportAction<SearchRe
                         logger.trace("{}: Failed to execute [{}]", t, shard, request);
                     }
                 }
+                final ShardSearchFailure[] shardSearchFailures = buildShardFailures();
                 if (successfulOps.get() == 0) {
                     if (logger.isDebugEnabled()) {
                         logger.debug("All shards failed for phase: [{}]", t, firstPhaseName());
                     }
+
                     // no successful ops, raise an exception
-                    raiseEarlyFailure(new SearchPhaseExecutionException(firstPhaseName(), "all shards failed", buildShardFailures()));
+                    raiseEarlyFailure(new SearchPhaseExecutionException(firstPhaseName(), "all shards failed", t, shardSearchFailures));
                 } else {
                     try {
                         innerMoveToSecondPhase();
                     } catch (Throwable e) {
-                        raiseEarlyFailure(new ReduceSearchPhaseException(firstPhaseName(), "", e, buildShardFailures()));
+                        raiseEarlyFailure(new ReduceSearchPhaseException(firstPhaseName(), "", e, shardSearchFailures));
                     }
                 }
             } else {
diff --git a/core/src/test/java/org/elasticsearch/ESExceptionTests.java b/core/src/test/java/org/elasticsearch/ESExceptionTests.java
index 4d81b3488cb..5aeaf9601ef 100644
--- a/core/src/test/java/org/elasticsearch/ESExceptionTests.java
+++ b/core/src/test/java/org/elasticsearch/ESExceptionTests.java
@@ -140,7 +140,7 @@ public class ESExceptionTests extends ESTestCase {
                     new SearchShardTarget("node_1", "foo", 1));
             ShardSearchFailure failure1 = new ShardSearchFailure(new ParsingException(1, 2, "foobar", null),
                     new SearchShardTarget("node_1", "foo", 2));
-            SearchPhaseExecutionException ex = new SearchPhaseExecutionException("search", "all shards failed", new ShardSearchFailure[]{failure, failure1});
+            SearchPhaseExecutionException ex = new SearchPhaseExecutionException("search", "all shards failed", randomBoolean() ? failure1.getCause() : failure.getCause(), new ShardSearchFailure[]{failure, failure1});
             XContentBuilder builder = XContentFactory.jsonBuilder();
             builder.startObject();
             ex.toXContent(builder, PARAMS);
@@ -163,6 +163,21 @@ public class ESExceptionTests extends ESTestCase {
             String expected = "{\"type\":\"search_phase_execution_exception\",\"reason\":\"all shards failed\",\"phase\":\"search\",\"grouped\":true,\"failed_shards\":[{\"shard\":1,\"index\":\"foo\",\"node\":\"node_1\",\"reason\":{\"type\":\"parsing_exception\",\"reason\":\"foobar\",\"line\":1,\"col\":2}},{\"shard\":1,\"index\":\"foo1\",\"node\":\"node_1\",\"reason\":{\"type\":\"query_shard_exception\",\"reason\":\"foobar\",\"index\":\"foo1\"}}]}";
             assertEquals(expected, builder.string());
         }
+        {
+            ShardSearchFailure failure = new ShardSearchFailure(new ParsingException(1, 2, "foobar", null),
+                    new SearchShardTarget("node_1", "foo", 1));
+            ShardSearchFailure failure1 = new ShardSearchFailure(new ParsingException(1, 2, "foobar", null),
+                    new SearchShardTarget("node_1", "foo", 2));
+            NullPointerException nullPointerException = new NullPointerException();
+            SearchPhaseExecutionException ex = new SearchPhaseExecutionException("search", "all shards failed", nullPointerException, new ShardSearchFailure[]{failure, failure1});
+            assertEquals(nullPointerException, ex.getCause());
+            XContentBuilder builder = XContentFactory.jsonBuilder();
+            builder.startObject();
+            ex.toXContent(builder, PARAMS);
+            builder.endObject();
+            String expected = "{\"type\":\"search_phase_execution_exception\",\"reason\":\"all shards failed\",\"phase\":\"search\",\"grouped\":true,\"failed_shards\":[{\"shard\":1,\"index\":\"foo\",\"node\":\"node_1\",\"reason\":{\"type\":\"parsing_exception\",\"reason\":\"foobar\",\"line\":1,\"col\":2}}],\"caused_by\":{\"type\":\"null_pointer_exception\",\"reason\":null}}";
+            assertEquals(expected, builder.string());
+        }
     }
 
     public void testGetRootCause() {
diff --git a/core/src/test/java/org/elasticsearch/index/query/TemplateQueryIT.java b/core/src/test/java/org/elasticsearch/index/query/TemplateQueryIT.java
index ce5eca5d11c..d4816f8d334 100644
--- a/core/src/test/java/org/elasticsearch/index/query/TemplateQueryIT.java
+++ b/core/src/test/java/org/elasticsearch/index/query/TemplateQueryIT.java
@@ -307,7 +307,7 @@ public class TemplateQueryIT extends ESIntegTestCase {
                                     templateParams)).get();
             fail("Expected SearchPhaseExecutionException");
         } catch (SearchPhaseExecutionException e) {
-            assertThat(e.getCause().getMessage(), containsString("Illegal index script format"));
+            assertThat(e.toString(), containsString("Illegal index script format"));
         }
     }