From ea750de39f2f1ae312515d0770cb6487787a72db Mon Sep 17 00:00:00 2001 From: javanna Date: Fri, 23 Oct 2015 18:53:02 +0200 Subject: [PATCH] Explain api: move query parsing to the coordinating node Similarly to what we did with the search api, we can now also move query parsing on the coordinating node for the explain api. Given that the explain api is a single shard operation (compared to search which is instead a broadcast operation), this doesn't change a lot in how the api works internally. The main benefit is that we can simplify the java api by requiring a structured query object to be provided rather than a bytes array that will get parsed on the data node. Previously if you specified a QueryBuilder it would be serialized in json format and would get reparsed on the data node, while now it doesn't go through parsing anymore (as expected), given that after the query-refactoring we are able to properly stream queries natively. Closes #14270 --- .../action/explain/ExplainRequest.java | 27 ++++----- .../action/explain/ExplainRequestBuilder.java | 38 +------------ .../explain/TransportExplainAction.java | 2 +- .../action/explain/RestExplainAction.java | 31 ++++------ .../messy/tests/IndicesRequestTests.java | 56 ++++--------------- 5 files changed, 33 insertions(+), 121 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/action/explain/ExplainRequest.java b/core/src/main/java/org/elasticsearch/action/explain/ExplainRequest.java index 2b796b08f9c..08c188ae998 100644 --- a/core/src/main/java/org/elasticsearch/action/explain/ExplainRequest.java +++ b/core/src/main/java/org/elasticsearch/action/explain/ExplainRequest.java @@ -21,13 +21,11 @@ package org.elasticsearch.action.explain; import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.ValidateActions; -import org.elasticsearch.action.support.QuerySourceBuilder; import org.elasticsearch.action.support.single.shard.SingleShardRequest; -import org.elasticsearch.client.Requests; import org.elasticsearch.common.Strings; -import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.search.fetch.source.FetchSourceContext; import java.io.IOException; @@ -41,7 +39,7 @@ public class ExplainRequest extends SingleShardRequest { private String id; private String routing; private String preference; - private BytesReference source; + private QueryBuilder query; private String[] fields; private FetchSourceContext fetchSourceContext; @@ -102,17 +100,12 @@ public class ExplainRequest extends SingleShardRequest { return this; } - public BytesReference source() { - return source; + public QueryBuilder query() { + return query; } - public ExplainRequest source(QuerySourceBuilder sourceBuilder) { - this.source = sourceBuilder.buildAsBytes(Requests.CONTENT_TYPE); - return this; - } - - public ExplainRequest source(BytesReference source) { - this.source = source; + public ExplainRequest query(QueryBuilder query) { + this.query = query; return this; } @@ -159,8 +152,8 @@ public class ExplainRequest extends SingleShardRequest { if (id == null) { validationException = ValidateActions.addValidationError("id is missing", validationException); } - if (source == null) { - validationException = ValidateActions.addValidationError("source is missing", validationException); + if (query == null) { + validationException = ValidateActions.addValidationError("query is missing", validationException); } return validationException; } @@ -172,7 +165,7 @@ public class ExplainRequest extends SingleShardRequest { id = in.readString(); routing = in.readOptionalString(); preference = in.readOptionalString(); - source = in.readBytesReference(); + query = in.readQuery(); filteringAlias = in.readStringArray(); if (in.readBoolean()) { fields = in.readStringArray(); @@ -189,7 +182,7 @@ public class ExplainRequest extends SingleShardRequest { out.writeString(id); out.writeOptionalString(routing); out.writeOptionalString(preference); - out.writeBytesReference(source); + out.writeQuery(query); out.writeStringArray(filteringAlias); if (fields != null) { out.writeBoolean(true); diff --git a/core/src/main/java/org/elasticsearch/action/explain/ExplainRequestBuilder.java b/core/src/main/java/org/elasticsearch/action/explain/ExplainRequestBuilder.java index f78b0ea2e6d..2910736031f 100644 --- a/core/src/main/java/org/elasticsearch/action/explain/ExplainRequestBuilder.java +++ b/core/src/main/java/org/elasticsearch/action/explain/ExplainRequestBuilder.java @@ -19,12 +19,10 @@ package org.elasticsearch.action.explain; -import org.elasticsearch.action.support.QuerySourceBuilder; import org.elasticsearch.action.support.single.shard.SingleShardOperationRequestBuilder; import org.elasticsearch.client.ElasticsearchClient; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.Strings; -import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.search.fetch.source.FetchSourceContext; @@ -33,8 +31,6 @@ import org.elasticsearch.search.fetch.source.FetchSourceContext; */ public class ExplainRequestBuilder extends SingleShardOperationRequestBuilder { - private QuerySourceBuilder sourceBuilder; - ExplainRequestBuilder(ElasticsearchClient client, ExplainAction action) { super(client, action, new ExplainRequest()); } @@ -87,15 +83,7 @@ public class ExplainRequestBuilder extends SingleShardOperationRequestBuilder query = RestActions.urlParamsToQueryBuilder(request); + explainRequest.query(query); } String sField = request.param("fields"); diff --git a/plugins/lang-groovy/src/test/java/org/elasticsearch/messy/tests/IndicesRequestTests.java b/plugins/lang-groovy/src/test/java/org/elasticsearch/messy/tests/IndicesRequestTests.java index 15c77960ead..4291f00bf1a 100644 --- a/plugins/lang-groovy/src/test/java/org/elasticsearch/messy/tests/IndicesRequestTests.java +++ b/plugins/lang-groovy/src/test/java/org/elasticsearch/messy/tests/IndicesRequestTests.java @@ -76,7 +76,6 @@ import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.action.search.SearchType; import org.elasticsearch.action.suggest.SuggestAction; import org.elasticsearch.action.suggest.SuggestRequest; -import org.elasticsearch.action.support.QuerySourceBuilder; import org.elasticsearch.action.termvectors.MultiTermVectorsAction; import org.elasticsearch.action.termvectors.MultiTermVectorsRequest; import org.elasticsearch.action.termvectors.TermVectorsAction; @@ -96,33 +95,16 @@ import org.elasticsearch.test.ESIntegTestCase; import org.elasticsearch.test.ESIntegTestCase.ClusterScope; import org.elasticsearch.test.ESIntegTestCase.Scope; import org.elasticsearch.threadpool.ThreadPool; -import org.elasticsearch.transport.Transport; -import org.elasticsearch.transport.TransportChannel; -import org.elasticsearch.transport.TransportModule; -import org.elasticsearch.transport.TransportRequest; -import org.elasticsearch.transport.TransportRequestHandler; -import org.elasticsearch.transport.TransportService; +import org.elasticsearch.transport.*; import org.junit.After; import org.junit.Before; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; -import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; +import java.util.*; import java.util.function.Supplier; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures; -import static org.hamcrest.Matchers.emptyIterable; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.greaterThan; -import static org.hamcrest.Matchers.hasItem; -import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.*; @ClusterScope(scope = Scope.SUITE, numClientNodes = 1, minNumDataNodes = 2) public class IndicesRequestTests extends ESIntegTestCase { @@ -307,7 +289,7 @@ public class IndicesRequestTests extends ESIntegTestCase { String explainShardAction = ExplainAction.NAME + "[s]"; interceptTransportActions(explainShardAction); - ExplainRequest explainRequest = new ExplainRequest(randomIndexOrAlias(), "type", "id").source(new QuerySourceBuilder().setQuery(QueryBuilders.matchAllQuery())); + ExplainRequest explainRequest = new ExplainRequest(randomIndexOrAlias(), "type", "id").query(QueryBuilders.matchAllQuery()); internalCluster().clientNodeClient().explain(explainRequest).actionGet(); clearInterceptedActions(); @@ -684,24 +666,6 @@ public class IndicesRequestTests extends ESIntegTestCase { } } } - - private static void assertSameIndicesOptionalRequests(String[] indices, String... actions) { - assertSameIndices(indices, true, actions); - } - - private static void assertSameIndices(String[] indices, boolean optional, String... actions) { - for (String action : actions) { - List requests = consumeTransportRequests(action); - if (!optional) { - assertThat("no internal requests intercepted for action [" + action + "]", requests.size(), greaterThan(0)); - } - for (TransportRequest internalRequest : requests) { - assertThat(internalRequest, instanceOf(IndicesRequest.class)); - assertThat(internalRequest.getClass().getName(), ((IndicesRequest)internalRequest).indices(), equalTo(indices)); - } - } - } - private static void assertIndicesSubset(List indices, String... actions) { //indices returned by each bulk shard request need to be a subset of the original indices for (String action : actions) { @@ -820,26 +784,26 @@ public class IndicesRequestTests extends ESIntegTestCase { @Override public void registerRequestHandler(String action, Supplier request, String executor, boolean forceExecution, TransportRequestHandler handler) { - super.registerRequestHandler(action, request, executor, forceExecution, new InterceptingRequestHandler(action, handler)); + super.registerRequestHandler(action, request, executor, forceExecution, new InterceptingRequestHandler<>(action, handler)); } @Override public void registerRequestHandler(String action, Supplier requestFactory, String executor, TransportRequestHandler handler) { - super.registerRequestHandler(action, requestFactory, executor, new InterceptingRequestHandler(action, handler)); + super.registerRequestHandler(action, requestFactory, executor, new InterceptingRequestHandler<>(action, handler)); } - private class InterceptingRequestHandler implements TransportRequestHandler { + private class InterceptingRequestHandler implements TransportRequestHandler { - private final TransportRequestHandler requestHandler; + private final TransportRequestHandler requestHandler; private final String action; - InterceptingRequestHandler(String action, TransportRequestHandler requestHandler) { + InterceptingRequestHandler(String action, TransportRequestHandler requestHandler) { this.requestHandler = requestHandler; this.action = action; } @Override - public void messageReceived(TransportRequest request, TransportChannel channel) throws Exception { + public void messageReceived(T request, TransportChannel channel) throws Exception { synchronized (InterceptingTransportService.this) { if (actions.contains(action)) { List requestList = requests.get(action);