Ensure query resources are fetched asynchronously during rewrite #25791

s1monw · 2017-07-19T13:13:11Z

The QueryRewriteContext used to provide a client object that can
be used to fetch geo-shapes, terms or documents for percolation. Unfortunately
all client calls used to be blocking calls which can have significant impact on the
rewrite phase since it occupies an entire search thread until the resource is
received. In the case that the index the resource is fetched from isn't on the local
node this can have significant impact on query throughput.

Note: this doesn't fix MLT since it fetches stuff in doQuery which is a different beast. Yet, it is a huge step in the right direction

The `QueryRewriteContext` used to provide a client object that can be used to fetch geo-shapes, terms or documents for percolation. Unfortunately all client calls used to be blocking calls which can have significant impact on the rewrite phase since it occupies an entire search thread until the resource is received. In the case that the index the resource is fetched from isn't on the local node this can have significant impact on query throughput.

jpountz · 2017-07-19T15:59:34Z

core/src/main/java/org/elasticsearch/index/query/Rewriteable.java

@@ -27,6 +27,8 @@
 */
 public interface Rewriteable<T> {

+    int MAX_REVIEW_ROUNDS = 16;


s/REWIEW/REWRITE/ ?

jpountz

This is not the part of the code base that I'm most familiar with so I could easily miss something, but it looks good to me.

jimczi

For some query builders (geo and terms) we could avoid fetching the same data for each shard requests but that's another story.
LGTM for the asynchronous fetch and the light rewrite for canMatch

colings86

LGTM2

martijnvg

Left 2 comments. LGTM

martijnvg · 2017-07-20T08:42:44Z

modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java

    }

    @Override
    protected void doWriteTo(StreamOutput out) throws IOException {
+        if (documentSupplier != null) {
+            throw new IllegalStateException("document supplier must be non-null");


document supplier must be null instead?

correct ++ I will fix and add a test

martijnvg · 2017-07-20T08:54:39Z

core/src/main/java/org/elasticsearch/index/query/Rewriteable.java

+
+    /**
+     * Rewrites the given {@link Rewriteable} into its primitive form. Rewriteables that for instance fetch resources from remote hosts or
+     * can simplify / optimize itself should do their heavy lifting during {@link #rewrite(QueryRewriteContext)}. This method


Do you mean {@link #rewriteAndFetch(QueryRewriteContext)} instead of {@link #rewrite(QueryRewriteContext)}? Because normal rewrite doesn't fetch any resource?

… once.

javanna

left a few minors, LGTM though

javanna · 2017-07-20T07:41:21Z

core/src/main/java/org/elasticsearch/index/query/QueryRewriteContext.java

+        return asyncActions.isEmpty() == false;
+    }
+
+    public void executeAsyncActions(ActionListener listener) {


ActionListener<?> ?

javanna · 2017-07-20T07:47:09Z

core/src/main/java/org/elasticsearch/index/query/QueryRewriteContext.java

+                    }
+                }
+            };
+            ArrayList<BiConsumer<Client, ActionListener<?>>> biConsumers = new ArrayList<>(asyncActions);


nit: List on the left side instead of ArrayList

javanna · 2017-07-20T07:48:04Z

core/src/main/java/org/elasticsearch/index/query/QueryRewriteContext.java

+        if (asyncActions.isEmpty()) {
+            listener.onResponse(null);
+        } else {
+            CountDown done = new CountDown(asyncActions.size());


nit: maybe call it countDown instead of done?

javanna · 2017-07-20T08:03:05Z

core/src/main/java/org/elasticsearch/index/query/QueryRewriteContext.java

+            listener.onResponse(null);
+        } else {
+            CountDown done = new CountDown(asyncActions.size());
+            ActionListener internalListener = new ActionListener() {


ActionListener<?> here too?

javanna · 2017-07-20T09:52:14Z

core/src/main/java/org/elasticsearch/index/query/GeoShapeQueryBuilder.java

+            return supplier.get() == null ? this : new GeoShapeQueryBuilder(this.fieldName, supplier.get()).relation(relation).strategy
+                (strategy);
+        } else if (this.shape == null) {
+            AtomicReference<ShapeBuilder> supplier = new AtomicReference<>();


I wonder if AtomicReference is needed here given that we write from a single thread. It's just a visibility problem hence SetOnce would be a good fit, which has also the advantage of checking that we do set it only once (thanks for the suggestion!)

I did that already thanks for the idea

javanna · 2017-07-20T09:54:04Z

core/src/main/java/org/elasticsearch/search/SearchService.java

+     * The action listener is guaranteed to be executed on the search thread-pool
+     */
+    private void rewriteShardRequest(ShardSearchRequest request, ActionListener<ShardSearchRequest> listener) {
+        Rewriteable.rewriteAndFetch(request.getRewriteable(), indicesService.getRewriteContext(request::nowInMillis),


would be nice to address the unchecked warning on this line. To be honest I tried that myself and I didn't succeed :)

I dont see that

This change rewrites search requests on the coordinating node before we send requests to the individual shards. This will reduce the rewrite load and object creation for each rewrite on the executing nodes and will fetch resources only once instead of N times once per shard for queries like `terms` query with index lookups. (among percolator and geo-shape) Relates to #25791

s1monw added :Search/Search Search-related issues that do not fall into other categories das awesome >enhancement review v6.0.0 labels Jul 19, 2017

s1monw requested review from colings86 and javanna July 19, 2017 13:13

s1monw added 6 commits July 19, 2017 15:20

fix javadocs

ceea610

Cut over more rewriteables

939222a

fix lines

fe5435a

ensure we fail fast once we have unexpected async fetches

3fd130e

remove default method

31aa7e9

add protection against runnaway rewrites

52736a1

jpountz reviewed Jul 19, 2017

View reviewed changes

jpountz approved these changes Jul 19, 2017

View reviewed changes

s1monw added 3 commits July 19, 2017 20:27

don't ignore argument

bb1d9c2

fix spelling

fd16b9b

fix percolator mapper test

36f233f

s1monw requested a review from jimczi July 19, 2017 19:19

s1monw added 2 commits July 19, 2017 22:11

Add rewrite tests for rescore and highlighter

68c2908

Merge branch 'master' into fetch_query_builder_docs_async

4faa7d2

jimczi approved these changes Jul 20, 2017

View reviewed changes

colings86 approved these changes Jul 20, 2017

View reviewed changes

martijnvg approved these changes Jul 20, 2017

View reviewed changes

s1monw added 3 commits July 20, 2017 11:36

apply feedback

ebc9ec7

add javadocs

c080c70

use SetOnce instead of AtomicReference to ensure stuff is only passed…

aa0579c

… once.

javanna approved these changes Jul 20, 2017

View reviewed changes

fix javadocs

d75def8

s1monw added 4 commits July 20, 2017 13:42

apply feedback

601d831

Merge branch 'master' into fetch_query_builder_docs_async

82ee01f

add test for Rewritable

e008b7e

streamline list rewrites and add test

cd5ef94

s1monw merged commit 5e629cf into elastic:master Jul 20, 2017

s1monw mentioned this pull request Jul 20, 2017

Rewrite search requests on the coordinating nodes #25814

Merged

s1monw deleted the fetch_query_builder_docs_async branch July 20, 2017 15:23

clintongormley added v6.0.0-beta1 and removed v6.0.0 labels Jul 25, 2017

s1monw mentioned this pull request Sep 5, 2017

Let search phases override max concurrent requests #26484

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure query resources are fetched asynchronously during rewrite #25791

Ensure query resources are fetched asynchronously during rewrite #25791

s1monw commented Jul 19, 2017

jpountz Jul 19, 2017

jpountz left a comment

jimczi left a comment

colings86 left a comment

martijnvg left a comment

martijnvg Jul 20, 2017

s1monw Jul 20, 2017

martijnvg Jul 20, 2017

javanna left a comment

javanna Jul 20, 2017

javanna Jul 20, 2017

javanna Jul 20, 2017

javanna Jul 20, 2017

javanna Jul 20, 2017

s1monw Jul 20, 2017

javanna Jul 20, 2017

s1monw Jul 20, 2017

Ensure query resources are fetched asynchronously during rewrite #25791

Ensure query resources are fetched asynchronously during rewrite #25791

Conversation

s1monw commented Jul 19, 2017

Choose a reason for hiding this comment

jpountz left a comment

Choose a reason for hiding this comment

jimczi left a comment

Choose a reason for hiding this comment

colings86 left a comment

Choose a reason for hiding this comment

martijnvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment