From b22eeb8ec34591274d42f92c48dac4dcd5b13f80 Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Tue, 10 Apr 2018 14:49:21 -0700 Subject: [PATCH 1/9] WIP commit to try calling rewrite on coordinating node during TransportSearchAction --- .../query/TransportValidateQueryAction.java | 28 +++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java index 0513a37e4fe0..fbabe425e299 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java @@ -22,6 +22,11 @@ import org.apache.lucene.search.MatchNoDocsQuery; import org.apache.lucene.search.Query; import org.elasticsearch.action.ActionListener; +import org.elasticsearch.action.OriginalIndices; +import org.elasticsearch.action.search.SearchResponse; +import org.elasticsearch.action.search.SearchShardIterator; +import org.elasticsearch.action.search.SearchTask; +import org.elasticsearch.action.search.TransportSearchAction; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.DefaultShardOperationFailedException; import org.elasticsearch.action.support.broadcast.BroadcastShardOperationFailedException; @@ -30,6 +35,7 @@ import org.elasticsearch.cluster.block.ClusterBlockException; import org.elasticsearch.cluster.block.ClusterBlockLevel; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; +import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.routing.GroupShardsIterator; import org.elasticsearch.cluster.routing.ShardRouting; import org.elasticsearch.cluster.service.ClusterService; @@ -39,21 +45,24 @@ import org.elasticsearch.common.lease.Releasables; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.query.ParsedQuery; +import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.index.query.QueryShardException; +import org.elasticsearch.index.query.Rewriteable; import org.elasticsearch.search.SearchService; +import org.elasticsearch.search.builder.SearchSourceBuilder; import org.elasticsearch.search.internal.AliasFilter; import org.elasticsearch.search.internal.SearchContext; import org.elasticsearch.search.internal.ShardSearchLocalRequest; import org.elasticsearch.tasks.Task; import org.elasticsearch.threadpool.ThreadPool; +import org.elasticsearch.transport.RemoteClusterAware; import org.elasticsearch.transport.TransportService; import java.io.IOException; -import java.util.ArrayList; -import java.util.List; -import java.util.Map; -import java.util.Set; +import java.util.*; import java.util.concurrent.atomic.AtomicReferenceArray; +import java.util.function.BiFunction; +import java.util.function.LongSupplier; public class TransportValidateQueryAction extends TransportBroadcastAction { @@ -71,7 +80,16 @@ public TransportValidateQueryAction(Settings settings, ThreadPool threadPool, Cl @Override protected void doExecute(Task task, ValidateQueryRequest request, ActionListener listener) { request.nowInMillis = System.currentTimeMillis(); - super.doExecute(task, request, listener); + LongSupplier timeProvider = () -> request.nowInMillis; + ActionListener rewriteListener = ActionListener.wrap(source -> { + super.doExecute(task, request, listener); + }, listener::onFailure); + if (request.query() == null) { + rewriteListener.onResponse(request.query()); + } else { + Rewriteable.rewriteAndFetch(request.query(), searchService.getRewriteContext(timeProvider), + rewriteListener); + } } @Override From 8127f2d3f19c3f1f0d2c986cd3902cc67d2a143b Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Wed, 11 Apr 2018 10:46:07 -0700 Subject: [PATCH 2/9] Use re-written query instead of using the original query --- .../indices/validate/query/TransportValidateQueryAction.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java index fbabe425e299..a93cca73dc0e 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java @@ -81,7 +81,8 @@ public TransportValidateQueryAction(Settings settings, ThreadPool threadPool, Cl protected void doExecute(Task task, ValidateQueryRequest request, ActionListener listener) { request.nowInMillis = System.currentTimeMillis(); LongSupplier timeProvider = () -> request.nowInMillis; - ActionListener rewriteListener = ActionListener.wrap(source -> { + ActionListener rewriteListener = ActionListener.wrap(rewrittenQuery -> { + request.query(rewrittenQuery); super.doExecute(task, request, listener); }, listener::onFailure); if (request.query() == null) { From f94a48aad0de328084cf6b355d2af6a235128011 Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Wed, 11 Apr 2018 12:17:02 -0700 Subject: [PATCH 3/9] fix incorrect/unused imports and wildcarding --- .../query/TransportValidateQueryAction.java | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java index a93cca73dc0e..006305fbb7c0 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java @@ -22,11 +22,6 @@ import org.apache.lucene.search.MatchNoDocsQuery; import org.apache.lucene.search.Query; import org.elasticsearch.action.ActionListener; -import org.elasticsearch.action.OriginalIndices; -import org.elasticsearch.action.search.SearchResponse; -import org.elasticsearch.action.search.SearchShardIterator; -import org.elasticsearch.action.search.SearchTask; -import org.elasticsearch.action.search.TransportSearchAction; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.DefaultShardOperationFailedException; import org.elasticsearch.action.support.broadcast.BroadcastShardOperationFailedException; @@ -35,7 +30,6 @@ import org.elasticsearch.cluster.block.ClusterBlockException; import org.elasticsearch.cluster.block.ClusterBlockLevel; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; -import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.routing.GroupShardsIterator; import org.elasticsearch.cluster.routing.ShardRouting; import org.elasticsearch.cluster.service.ClusterService; @@ -45,23 +39,22 @@ import org.elasticsearch.common.lease.Releasables; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.query.ParsedQuery; -import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.index.query.QueryShardException; import org.elasticsearch.index.query.Rewriteable; import org.elasticsearch.search.SearchService; -import org.elasticsearch.search.builder.SearchSourceBuilder; import org.elasticsearch.search.internal.AliasFilter; import org.elasticsearch.search.internal.SearchContext; import org.elasticsearch.search.internal.ShardSearchLocalRequest; import org.elasticsearch.tasks.Task; import org.elasticsearch.threadpool.ThreadPool; -import org.elasticsearch.transport.RemoteClusterAware; import org.elasticsearch.transport.TransportService; import java.io.IOException; -import java.util.*; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Set; import java.util.concurrent.atomic.AtomicReferenceArray; -import java.util.function.BiFunction; import java.util.function.LongSupplier; public class TransportValidateQueryAction extends TransportBroadcastAction { From 3e4b77c6a9dfcd96ae248cfa8e980d550553193f Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Wed, 11 Apr 2018 13:57:21 -0700 Subject: [PATCH 4/9] add error handling for cases where an exception is thrown --- .../query/TransportValidateQueryAction.java | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java index 006305fbb7c0..fd8823c1d074 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java @@ -77,7 +77,27 @@ protected void doExecute(Task task, ValidateQueryRequest request, ActionListener ActionListener rewriteListener = ActionListener.wrap(rewrittenQuery -> { request.query(rewrittenQuery); super.doExecute(task, request, listener); - }, listener::onFailure); + }, ex -> { + List explanations = new ArrayList<>(); + // TODO[PCS]: given that we can have multiple indices, what's the best way to include + // these in a query explanation? + explanations.add(new QueryExplanation("", + QueryExplanation.RANDOM_SHARD, + false, + ex.getMessage(), + // TODO[PCS]: duplicate information is never good, what is right for detail here? + ex.getMessage())); + listener.onResponse( + new ValidateQueryResponse( + false, + explanations, + // TODO[PCS]: is it correct to have 0 for total shards if the request is invalidated before + // it makes it to the shards? I think the answer is probably no? + 0, + 0 , + 0, + null)); + }); if (request.query() == null) { rewriteListener.onResponse(request.query()); } else { From f218da2ad228a50c613792ea04cdc3f6bf21f245 Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Tue, 17 Apr 2018 12:09:58 -0700 Subject: [PATCH 5/9] correct exception handling such that integration tests pass successfully --- .../validate/query/TransportValidateQueryAction.java | 9 +++++++-- .../java/org/elasticsearch/index/query/Rewriteable.java | 4 +++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java index fd8823c1d074..a0bae6fb9829 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java @@ -38,6 +38,7 @@ import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.lease.Releasables; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.index.IndexNotFoundException; import org.elasticsearch.index.query.ParsedQuery; import org.elasticsearch.index.query.QueryShardException; import org.elasticsearch.index.query.Rewriteable; @@ -77,14 +78,18 @@ protected void doExecute(Task task, ValidateQueryRequest request, ActionListener ActionListener rewriteListener = ActionListener.wrap(rewrittenQuery -> { request.query(rewrittenQuery); super.doExecute(task, request, listener); - }, ex -> { + }, + ex -> { + if (ex instanceof IndexNotFoundException) { + listener.onFailure(ex); + } List explanations = new ArrayList<>(); // TODO[PCS]: given that we can have multiple indices, what's the best way to include // these in a query explanation? explanations.add(new QueryExplanation("", QueryExplanation.RANDOM_SHARD, false, - ex.getMessage(), + null, // TODO[PCS]: duplicate information is never good, what is right for detail here? ex.getMessage())); listener.onResponse( diff --git a/server/src/main/java/org/elasticsearch/index/query/Rewriteable.java b/server/src/main/java/org/elasticsearch/index/query/Rewriteable.java index 492130527e8d..5ee1ee4c1f34 100644 --- a/server/src/main/java/org/elasticsearch/index/query/Rewriteable.java +++ b/server/src/main/java/org/elasticsearch/index/query/Rewriteable.java @@ -19,6 +19,8 @@ package org.elasticsearch.index.query; import org.elasticsearch.action.ActionListener; +import org.elasticsearch.common.ParsingException; +import org.elasticsearch.common.xcontent.XContentParseException; import java.io.IOException; import java.util.ArrayList; @@ -111,7 +113,7 @@ static > void rewriteAndFetch(T original, QueryRewriteC } } rewriteResponse.onResponse(builder); - } catch (IOException ex) { + } catch (IOException|XContentParseException|ParsingException ex) { rewriteResponse.onFailure(ex); } } From 4925ba8a1d0ec66e84c3eacb77ce8e774e732a61 Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Tue, 17 Apr 2018 14:50:55 -0700 Subject: [PATCH 6/9] fix additional case covered by IndicesOptionsIntegrationIT. --- .../indices/validate/query/TransportValidateQueryAction.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java index a0bae6fb9829..1fa0495324e9 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java @@ -42,6 +42,7 @@ import org.elasticsearch.index.query.ParsedQuery; import org.elasticsearch.index.query.QueryShardException; import org.elasticsearch.index.query.Rewriteable; +import org.elasticsearch.indices.IndexClosedException; import org.elasticsearch.search.SearchService; import org.elasticsearch.search.internal.AliasFilter; import org.elasticsearch.search.internal.SearchContext; @@ -80,7 +81,8 @@ protected void doExecute(Task task, ValidateQueryRequest request, ActionListener super.doExecute(task, request, listener); }, ex -> { - if (ex instanceof IndexNotFoundException) { + if (ex instanceof IndexNotFoundException || + ex instanceof IndexClosedException) { listener.onFailure(ex); } List explanations = new ArrayList<>(); From 3849ae6e700da19de5daa7430842b29b2871845f Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Thu, 19 Apr 2018 11:39:11 -0700 Subject: [PATCH 7/9] add integration test case that verifies queries are now valid --- .../query/TransportValidateQueryAction.java | 1 - .../validate/SimpleValidateQueryIT.java | 19 +++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java index 1fa0495324e9..4c10993daa95 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java @@ -92,7 +92,6 @@ protected void doExecute(Task task, ValidateQueryRequest request, ActionListener QueryExplanation.RANDOM_SHARD, false, null, - // TODO[PCS]: duplicate information is never good, what is right for detail here? ex.getMessage())); listener.onResponse( new ValidateQueryResponse( diff --git a/server/src/test/java/org/elasticsearch/validate/SimpleValidateQueryIT.java b/server/src/test/java/org/elasticsearch/validate/SimpleValidateQueryIT.java index 3c4666148d8f..34501ba8a1b0 100644 --- a/server/src/test/java/org/elasticsearch/validate/SimpleValidateQueryIT.java +++ b/server/src/test/java/org/elasticsearch/validate/SimpleValidateQueryIT.java @@ -29,6 +29,8 @@ import org.elasticsearch.index.query.MoreLikeThisQueryBuilder; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryBuilders; +import org.elasticsearch.index.query.TermsQueryBuilder; +import org.elasticsearch.indices.TermsLookup; import org.elasticsearch.test.ESIntegTestCase; import org.elasticsearch.test.ESIntegTestCase.ClusterScope; import org.elasticsearch.test.ESIntegTestCase.Scope; @@ -330,4 +332,21 @@ private static void assertExplanations(QueryBuilder queryBuilder, assertThat(response.isValid(), equalTo(true)); } } + + public void testExplainTermsQueryWithLookup() throws Exception { + client().admin().indices().prepareCreate("twitter") + .addMapping("_doc", "user", "type=integer", "followers", "type=integer") + .setSettings(Settings.builder().put(SETTING_NUMBER_OF_SHARDS, 2).put("index.number_of_routing_shards", 2)).get(); + client().prepareIndex("twitter", "_doc", "1") + .setSource("followers", new int[] {1, 2, 3}).get(); + refresh(); + + TermsQueryBuilder termsLookupQuery = QueryBuilders.termsLookupQuery("user", new TermsLookup("twitter", "_doc", "1", "followers")); + ValidateQueryResponse response = client().admin().indices().prepareValidateQuery("twitter") + .setTypes("_doc") + .setQuery(termsLookupQuery) + .setExplain(true) + .execute().actionGet(); + assertThat(response.isValid(), is(true)); + } } From 226da402025e52a5daf517b53dbdf0b17de78686 Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Wed, 25 Apr 2018 21:23:20 -0700 Subject: [PATCH 8/9] add optional value for index --- .../indices/validate/query/QueryExplanation.java | 12 ++++++++++-- .../validate/query/TransportValidateQueryAction.java | 6 +++--- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/QueryExplanation.java b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/QueryExplanation.java index df9c12c95f4c..1438daf29fd1 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/QueryExplanation.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/QueryExplanation.java @@ -75,7 +75,11 @@ public String getExplanation() { @Override public void readFrom(StreamInput in) throws IOException { - index = in.readString(); + if (in.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) { + index = in.readOptionalString(); + } else { + index = in.readString(); + } if (in.getVersion().onOrAfter(Version.V_5_4_0)) { shard = in.readInt(); } else { @@ -88,7 +92,11 @@ public void readFrom(StreamInput in) throws IOException { @Override public void writeTo(StreamOutput out) throws IOException { - out.writeString(index); + if (out.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) { + out.writeOptionalString(index); + } else { + out.writeString(index); + } if (out.getVersion().onOrAfter(Version.V_5_4_0)) { out.writeInt(shard); } diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java index 4c10993daa95..eb9d71e5273b 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java @@ -88,7 +88,7 @@ protected void doExecute(Task task, ValidateQueryRequest request, ActionListener List explanations = new ArrayList<>(); // TODO[PCS]: given that we can have multiple indices, what's the best way to include // these in a query explanation? - explanations.add(new QueryExplanation("", + explanations.add(new QueryExplanation(null, QueryExplanation.RANDOM_SHARD, false, null, @@ -97,8 +97,8 @@ protected void doExecute(Task task, ValidateQueryRequest request, ActionListener new ValidateQueryResponse( false, explanations, - // TODO[PCS]: is it correct to have 0 for total shards if the request is invalidated before - // it makes it to the shards? I think the answer is probably no? + // totalShards is documented as "the total shards this request ran against", + // which is 0 since the failure is happening on the coordinating node. 0, 0 , 0, From 6d5eeffa54188336fb0d95a67353671641683c7f Mon Sep 17 00:00:00 2001 From: Paul Sanwald Date: Thu, 26 Apr 2018 08:53:38 -0700 Subject: [PATCH 9/9] address review comments: catch superclass of XContentParseException --- .../indices/validate/query/TransportValidateQueryAction.java | 2 -- .../main/java/org/elasticsearch/index/query/Rewriteable.java | 3 +-- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java index eb9d71e5273b..5be321734b5d 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java @@ -86,8 +86,6 @@ protected void doExecute(Task task, ValidateQueryRequest request, ActionListener listener.onFailure(ex); } List explanations = new ArrayList<>(); - // TODO[PCS]: given that we can have multiple indices, what's the best way to include - // these in a query explanation? explanations.add(new QueryExplanation(null, QueryExplanation.RANDOM_SHARD, false, diff --git a/server/src/main/java/org/elasticsearch/index/query/Rewriteable.java b/server/src/main/java/org/elasticsearch/index/query/Rewriteable.java index 5ee1ee4c1f34..ba8d6b84d537 100644 --- a/server/src/main/java/org/elasticsearch/index/query/Rewriteable.java +++ b/server/src/main/java/org/elasticsearch/index/query/Rewriteable.java @@ -20,7 +20,6 @@ import org.elasticsearch.action.ActionListener; import org.elasticsearch.common.ParsingException; -import org.elasticsearch.common.xcontent.XContentParseException; import java.io.IOException; import java.util.ArrayList; @@ -113,7 +112,7 @@ static > void rewriteAndFetch(T original, QueryRewriteC } } rewriteResponse.onResponse(builder); - } catch (IOException|XContentParseException|ParsingException ex) { + } catch (IOException|IllegalArgumentException|ParsingException ex) { rewriteResponse.onFailure(ex); } }