From eb9f4531bd736aef0f859571492dfd0b43a700c2 Mon Sep 17 00:00:00 2001
From: jimczi <jim.ferenczi@elastic.co>
Date: Tue, 4 Feb 2020 21:03:17 +0100
Subject: [PATCH] Remove the query builder serialization from
 QueryShardException message

QueryBuilders that throw exceptions on shards when building the Lucene query
returns the full serialization of the query builder in the exception message.
For large queries that fails to execute due to the max boolean clause, this means
that we keep a reference of these big messages for every shard that participate
in the request. In order to limit the memory needed to hold these query shard
exceptions in the coordinating node, this change removes the query builder
serialization from the shard exception. The query is known by the user so
there should be no need to repeat it on every shard exception. We could also
omit the entire stack trace for known bad request exception but it would deserve
a separate issue/pr.

Closes #51843
Closes #48910
---
 .../index/query/QueryShardContext.java        |  4 +-
 .../index/query/QueryShardContextTests.java   | 40 +++++++++++++++++++
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/server/src/main/java/org/elasticsearch/index/query/QueryShardContext.java b/server/src/main/java/org/elasticsearch/index/query/QueryShardContext.java
index 12b2da120f63e..c5dcfe5588ca6 100644
--- a/server/src/main/java/org/elasticsearch/index/query/QueryShardContext.java
+++ b/server/src/main/java/org/elasticsearch/index/query/QueryShardContext.java
@@ -309,10 +309,10 @@ private ParsedQuery toQuery(QueryBuilder queryBuilder, CheckedFunction<QueryBuil
         try {
             QueryBuilder rewriteQuery = Rewriteable.rewrite(queryBuilder, this, true);
             return new ParsedQuery(filterOrQuery.apply(rewriteQuery), copyNamedQueries());
-        } catch(QueryShardException | ParsingException e ) {
+        } catch(QueryShardException | ParsingException e) {
             throw e;
         } catch(Exception e) {
-            throw new QueryShardException(this, "failed to create query: {}", e, queryBuilder);
+            throw new QueryShardException(this, "failed to create query: {}", e, e.getMessage());
         } finally {
             reset();
         }
diff --git a/server/src/test/java/org/elasticsearch/index/query/QueryShardContextTests.java b/server/src/test/java/org/elasticsearch/index/query/QueryShardContextTests.java
index 0f9ccfa86f1aa..8e57ad50bd1cd 100644
--- a/server/src/test/java/org/elasticsearch/index/query/QueryShardContextTests.java
+++ b/server/src/test/java/org/elasticsearch/index/query/QueryShardContextTests.java
@@ -18,12 +18,15 @@
  */
 package org.elasticsearch.index.query;
 
+import org.apache.lucene.search.Query;
 import org.elasticsearch.Version;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
 import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
+import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.util.BigArrays;
 import org.elasticsearch.common.xcontent.NamedXContentRegistry;
+import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.index.IndexSettings;
 import org.elasticsearch.index.fielddata.IndexFieldData;
 import org.elasticsearch.index.fielddata.plain.AbstractAtomicOrdinalsFieldData;
@@ -73,6 +76,43 @@ public void testFailIfFieldMappingNotFound() {
         assertThat(result.name(), equalTo("name"));
     }
 
+    public void testToQueryFails() {
+        QueryShardContext context = createQueryShardContext(IndexMetaData.INDEX_UUID_NA_VALUE, null);
+        Exception exc = expectThrows(Exception.class,
+            () -> context.toQuery(new AbstractQueryBuilder() {
+                @Override
+                public String getWriteableName() {
+                    return null;
+                }
+
+                @Override
+                protected void doWriteTo(StreamOutput out) throws IOException {
+
+                }
+
+                @Override
+                protected void doXContent(XContentBuilder builder, Params params) throws IOException {
+
+                }
+
+                @Override
+                protected Query doToQuery(QueryShardContext context) throws IOException {
+                    throw new RuntimeException("boom");
+                }
+
+                @Override
+                protected boolean doEquals(AbstractQueryBuilder other) {
+                    return false;
+                }
+
+                @Override
+                protected int doHashCode() {
+                    return 0;
+                }
+            }));
+        assertThat(exc.getMessage(), equalTo("failed to create query: boom"));
+    }
+
     public void testClusterAlias() throws IOException {
         final String clusterAlias = randomBoolean() ? null : "remote_cluster";
         QueryShardContext context = createQueryShardContext(IndexMetaData.INDEX_UUID_NA_VALUE, clusterAlias);