Expose `batched_reduce_size` via `_search` #23288

s1monw · 2017-02-21T12:35:25Z

In #23253 we added the ability to incrementally reduce search results.
This change exposes the parameter to control the batch size and therefore
the memory consumption of a large search request.

In elastic#23253 we added an the ability to incrementally reduce search results. This change exposes the parameter to control the batch since and therefore the memory consumption of a large search request.

clintongormley

LGTM, but it's missing the doc changes.

clintongormley · 2017-02-21T12:36:57Z

rest-api-spec/src/main/resources/rest-api-spec/api/search.json

+        },
+        "batched_reduce_size" : {
+          "type" : "number",
+          "description" : "The number of shard results that should be reduced at once on the coordinating node. This value should be used as a protection mechanism to reduce the memory overhead per search request if the potential number of shards in the request can be large."


add

"default" : 512

s1monw · 2017-02-21T13:32:12Z

LGTM, but it's missing the doc changes.

I am not sure what you mean? I didn't add a documentation for this since it's so specialized. I want to expose it once we remove the softlimit?

nik9000

I think it'd be useful to return the number of reduction phases that we did so we can add a test that asserts that we did the right number. Just to make sure we didn't drop setting the parameter on the floor. I'm fine with you doing that in a followup or doing it myself since I'm the one that wants it.

nik9000 · 2017-02-21T13:34:56Z

test/framework/src/main/java/org/elasticsearch/test/client/RandomizingClient.java

@@ -60,7 +60,7 @@ public RandomizingClient(Client client, Random random) {

    @Override
    public SearchRequestBuilder prepareSearch(String... indices) {
-        return in.prepareSearch(indices).setSearchType(defaultSearchType).setPreference(defaultPreference).setReduceUpTo(reduceUpTo);
+        return in.prepareSearch(indices).setSearchType(defaultSearchType).setPreference(defaultPreference).setBatchedReduceSize(reduceUpTo);


Do you want to rename reduceUpTo here as well?

s1monw · 2017-02-21T13:39:55Z

I think it'd be useful to return the number of reduction phases that we did so we can add a test that asserts that we did the right number. Just to make sure we didn't drop setting the parameter on the floor. I'm fine with you doing that in a followup or doing it myself since I'm the one that wants it.

I don't think we should clutter the API with such an internal optimization. What do you expect from it?

nik9000 · 2017-02-21T13:45:09Z

What do you expect from it?

Just to know if the parameter had any effect at all. Right now you can't tell.

s1monw · 2017-02-21T13:47:52Z

Just to know if the parameter had any effect at all. Right now you can't tell.

this argument is odd. We are testing it throughout the stack with unittests and we know it's passed to the SearchRequest since there is the exception coming from. We can't pass any pointers back just for the integration test sake?

nik9000 · 2017-02-21T13:59:31Z

Once we've removed the limit we can test with a really large reduce. We'll want to do that anyway. I like returning the reduction count as well because it is simpler to debug if it fails and because it'll give us more information if the huge reduce fails and the reduction count test doesn't. I'm ok with not doing the test.

s1monw · 2017-02-21T14:05:14Z

Once we've removed the limit we can test with a really large reduce. We'll want to do that anyway. I like returning the reduction count as well because it is simpler to debug if it fails and because it'll give us more information if the huge reduce fails and the reduction count test doesn't. I'm ok with not doing the test.

we have a whole bunch of tests for this that were added in the original PR

…e phase

s1monw · 2017-02-21T15:56:35Z

I spoke to @nik9000 and I start to agree we should have this response parameter especially for debugging purposes of this feature at the users end. I added new commits.

nik9000 · 2017-02-21T15:58:05Z

core/src/main/java/org/elasticsearch/action/search/SearchResponse.java

@@ -179,13 +179,6 @@ public void scrollId(String scrollId) {
        return internalResponse.profile();
    }

-    static final class Fields {


clintongormley · 2017-02-21T17:00:43Z

I am not sure what you mean? I didn't add a documentation for this since it's so specialized. I want to expose it once we remove the softlimit?

OK

The assertion that if there are buffered aggs at least one incremental reduce phase should have happened doens't hold if there are shard failure. This commit removes this assertion. Relates to #23288

Both PRs below have been backported to 5.4 such that we can enable BWC tests of this feature as well as remove version dependend serialization for search request / responses. Relates to elastic#23288 Relates to elastic#23253

In #23253 we added an the ability to incrementally reduce search results. This change exposes the parameter to control the batch since and therefore the memory consumption of a large search request.

The assertion that if there are buffered aggs at least one incremental reduce phase should have happened doens't hold if there are shard failure. This commit removes this assertion. Relates to #23288

Both PRs below have been backported to 5.4 such that we can enable BWC tests of this feature as well as remove version dependend serialization for search request / responses. Relates to #23288 Relates to #23253

Mpdreamz · 2017-05-02T08:49:15Z

core/src/main/java/org/elasticsearch/action/search/SearchResponse.java

+            builder.field("terminated_early", isTerminatedEarly());
+        }
+        if (getNumReducePhases() != 1) {
+            builder.field("num_reduce_phases", getNumReducePhases());


Relates to #23288

* Updated api gen to 5.4 and added a way to patch specification files through special *.patch.json companion files. Due to pending discusion on elastic/elasticsearch@e579629 :q! * updated x-pack spec to 5.4 * add codegen part for xpack info related APIs * Added support for Field Caps API * add support for RemoteInfo API and adds cross cluster support to IndexName * added support for SourceExists() * add skipversion, eventhough this API existed it was undocumented prior to 5.4 * expose word delimiter graph token filter as per elastic/elasticsearch#23327 * spaces=>tabs * expose num_reduce_phases as per elastic/elasticsearch#23288 * implemented XPackInfo() started on XPackUsage() * added response structure for XPackUsage() * change license date from DateTime to DateTimeOffset' * implement PR feedback on #2743 * remove explicit folder includes in csproj files

Conflicts: src/Nest/Search/Search/SearchResponse.cs

Expose batched_reduce_size via _search

8831034

In elastic#23253 we added an the ability to incrementally reduce search results. This change exposes the parameter to control the batch since and therefore the memory consumption of a large search request.

s1monw added :Search/Search Search-related issues that do not fall into other categories >enhancement review v5.4.0 v6.0.0-alpha1 labels Feb 21, 2017

s1monw requested a review from clintongormley February 21, 2017 12:35

s1monw mentioned this pull request Feb 21, 2017

First step towards incremental reduction of query responses #23253

Merged

clintongormley reviewed Feb 21, 2017

View reviewed changes

add default to spec

f155394

nik9000 approved these changes Feb 21, 2017

View reviewed changes

rename variable

5cef908

s1monw added 3 commits February 21, 2017 16:43

add number of reduce phases to json response if there is more than on…

6ad0296

…e phase

remove useless class

af28113

remove line len suppression

c757276

nik9000 approved these changes Feb 21, 2017

View reviewed changes

s1monw added 3 commits February 21, 2017 17:01

fix line len:

62b1744

Merge branch 'master' into expose_batched_reduce_size

3a81046

Merge branch 'master' into expose_batched_reduce_size

5ab5f49

clintongormley approved these changes Feb 21, 2017

View reviewed changes

s1monw merged commit ce625eb into elastic:master Feb 21, 2017

s1monw deleted the expose_batched_reduce_size branch February 21, 2017 17:37

s1monw mentioned this pull request Feb 22, 2017

Remove BWC layer for number of reduce phases #23303

Merged

Mpdreamz reviewed May 2, 2017

View reviewed changes

Mpdreamz added a commit to elastic/elasticsearch-net that referenced this pull request May 2, 2017

expose num_reduce_phases as per elastic/elasticsearch#23288

b697af5

Mpdreamz mentioned this pull request May 2, 2017

Feature/num reduce phases elastic/elasticsearch-net#2748

Merged

clintongormley added a commit that referenced this pull request May 2, 2017

Added docs for batched_reduce_size

83e204d

Relates to #23288

clintongormley added a commit that referenced this pull request May 2, 2017

Added docs for batched_reduce_size

609172a

Relates to #23288

clintongormley added a commit that referenced this pull request May 2, 2017

Added docs for batched_reduce_size

582b3c0

Relates to #23288

Mpdreamz added a commit to elastic/elasticsearch-net that referenced this pull request May 4, 2017

expose num_reduce_phases as per elastic/elasticsearch#23288

60649f3

Mpdreamz added a commit to elastic/elasticsearch-net that referenced this pull request May 4, 2017

expose num_reduce_phases as per elastic/elasticsearch#23288

8dc8e6d

Mpdreamz added a commit to elastic/elasticsearch-net that referenced this pull request May 4, 2017

expose num_reduce_phases as per elastic/elasticsearch#23288

379c81b

Conflicts: src/Nest/Search/Search/SearchResponse.cs

awelburn pushed a commit to Artesian/elasticsearch-net that referenced this pull request Nov 6, 2017

expose num_reduce_phases as per elastic/elasticsearch#23288

920b3f1

Conflicts: src/Nest/Search/Search/SearchResponse.cs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose `batched_reduce_size` via `_search` #23288

Expose `batched_reduce_size` via `_search` #23288

s1monw commented Feb 21, 2017 •

edited by clintongormley

Loading

clintongormley left a comment

clintongormley Feb 21, 2017

s1monw commented Feb 21, 2017

nik9000 left a comment

nik9000 Feb 21, 2017

s1monw commented Feb 21, 2017

nik9000 commented Feb 21, 2017

s1monw commented Feb 21, 2017

nik9000 commented Feb 21, 2017

s1monw commented Feb 21, 2017

s1monw commented Feb 21, 2017

nik9000 Feb 21, 2017

clintongormley commented Feb 21, 2017

Mpdreamz May 2, 2017

Expose batched_reduce_size via _search #23288

Expose batched_reduce_size via _search #23288

Conversation

s1monw commented Feb 21, 2017 • edited by clintongormley Loading

clintongormley left a comment

Choose a reason for hiding this comment

clintongormley Feb 21, 2017

Choose a reason for hiding this comment

s1monw commented Feb 21, 2017

nik9000 left a comment

Choose a reason for hiding this comment

nik9000 Feb 21, 2017

Choose a reason for hiding this comment

s1monw commented Feb 21, 2017

nik9000 commented Feb 21, 2017

s1monw commented Feb 21, 2017

nik9000 commented Feb 21, 2017

s1monw commented Feb 21, 2017

s1monw commented Feb 21, 2017

nik9000 Feb 21, 2017

Choose a reason for hiding this comment

clintongormley commented Feb 21, 2017

Mpdreamz May 2, 2017

Choose a reason for hiding this comment

Expose `batched_reduce_size` via `_search` #23288

Expose `batched_reduce_size` via `_search` #23288

s1monw commented Feb 21, 2017 •

edited by clintongormley

Loading