Request-level circuit breaker support on coordinating nodes #62223

jimczi · 2020-09-10T13:29:35Z

This commit allows coordinating node to account the memory used to perform partial and final reduce of
aggregations in the request circuit breaker. The search coordinator adds the memory that it used to save
and reduce the results of shard aggregations in the request circuit breaker. Before any partial or final
reduce, the memory needed to reduce the aggregations is estimated and a CircuitBreakingException is thrown
if exceeds the maximum memory allowed in this breaker.
This size is estimated as roughly 1.5 times the size of the serialized aggregations that need to be reduced.
This estimation can be completely off for some aggregations but it is corrected with the real size after
the reduce completes.
If the reduce is successful, we update the circuit breaker to remove the size of the source aggregations
and replace the estimation with the serialized size of the newly reduced result.

As a follow up we could trigger partial reduces based on the memory accounted in the circuit breaker instead
of relying on a static number of shard responses. A simpler follow up that could be done in the mean time is
to reduce the default batch reduce size of blocking
search request to a more sane number.

Closes #37182

This commit allows coordinating node to account the memory used to perform partial and final reduce of aggregations in the request circuit breaker. The search coordinator adds the memory that it used to save and reduce the results of shard aggregations in the request circuit breaker. Before any partial or final reduce, the memory needed to reduce the aggregations is estimated and a CircuitBreakingException} is thrown if exceeds the maximum memory allowed in this breaker. This size is estimated as roughly 1.5 times the size of the serialized aggregations that need to be reduced. This estimation can be completely off for some aggregations but it is corrected with the real size after the reduce completes. If the reduce is successful, we update the circuit breaker to remove the size of the source aggregations and replace the estimation with the serialized size of the newly reduced result. As a follow up we could trigger partial reduces based on the memory accounted in the circuit breaker instead of relying on a static number of shard responses. A simpler follow up that could be done in the mean time is to [reduce the default batch reduce size](elastic#51857) of blocking search request to a more sane number. Closes elastic#37182

elasticmachine · 2020-09-10T13:29:37Z

Pinging @elastic/es-core-infra (:Core/Infra/Circuit Breakers)

elasticmachine · 2020-09-10T13:29:37Z

Pinging @elastic/es-analytics-geo (:Analytics/Geo)

jimczi · 2020-09-10T13:31:33Z

server/src/test/java/org/elasticsearch/action/search/TransportSearchActionSingleNodeTests.java

@@ -1,177 +0,0 @@
-/*
- * Licensed to Elasticsearch under one or more contributor


Note for reviewer: I moved these tests in https://github.com/elastic/elasticsearch/pull/62223/files#diff-0721d29fdc234c0f88a9019057ea55bd

it is a bit of a shame that these go from single node tests to full blown IT tests, what is the reasoning behind this choice?

I regrouped the search action tests in a single IT class. I agree that these tests may not require the full IT but they are grouped with other tests that require it so I thought that it makes sense to move them here.

nik9000

This looks right to me. I'll have to go over it more closely before 👍 it and I think I'd like to wait a day just to have fresh eyes on it.

This commit removes the serialization of partial reduce in order to speed up the merges when the batch reduce size is smaller than the number of shards in the request. The estimation of the size of partial reduce is still based on the binary size (serialized form) but we keep the full java object and estimate the size with a counting stream output. Finally this change adds a benchmark for the reduce of nested terms aggs. This benchmark was used to optimize the code in this PR.

jimczi · 2020-09-15T19:26:16Z

We discussed offline with @nik9000 and I pushed some changes to speed up the partial merge. First of all I removed the serialization of the partial reduce and replace it with an estimation of the size based on a noop-serialization (just counting the bytes). That resulted in much better performance for large cardinality aggregations and allows to estimate more precisely the memory used by the final reduce. I added the benchmark that I used in the PR to be able to replay the numbers.
For instance a terms/terms agg with a topN size of 100, 512 shards and an overall cardinality of 10,000 gives the following numbers on my machine:

Benchmark                        (bufferSize)  (cardinalityFactor)  (numShards)  (topNSize)  Mode  Cnt      Score      Error  Units
TermsReduceBenchmark.reduceAggs           512                  100         512         100  avgt    7   1756,207 ±   15,345  ms/op
TermsReduceBenchmark.reduceAggs            32                  100          512         100  avgt    7  27112,587 ± 13811,555  ms/op
TermsReduceBenchmark.reduceAggs            32                  100          512         100  avgt    7  10336,829 ± 2762,412  ms/op

The first result is the time it takes to reduce with a batch reduce size of 512, the second result is when we serialize the results of partial aggs with a batch reduce size of 32 and the last one is when we don't serialize partial results. As you can see the speedups are significant.

nik9000

I left some small things but LGTM.

It's a shame about reserializing being slow.

server/src/internalClusterTest/java/org/elasticsearch/action/search/TransportSearchIT.java

server/src/main/java/org/elasticsearch/action/search/QueryPhaseResultConsumer.java

server/src/main/java/org/elasticsearch/search/aggregations/InternalAggregations.java

server/src/test/java/org/elasticsearch/action/search/SearchPhaseControllerTests.java

… master.

javanna

left a couple of small questions

x-pack/plugin/async-search/src/main/java/org/elasticsearch/xpack/search/AsyncSearchTask.java

javanna · 2020-09-23T10:05:50Z

...k/plugin/async-search/src/test/java/org/elasticsearch/xpack/search/AsyncSearchTaskTests.java

-        assertThat(response.get().getFailure().getCause(), instanceOf(IllegalArgumentException.class));
-        assertEquals("Unknown NamedWriteable category [" + InternalAggregation.class.getName() + "]",
-            response.get().getFailure().getCause().getMessage());
-    }


is this test no longer relevant?

it cannot work anymore since we don't need to serialize the aggs. I think it's ok since we have other tests that check that exception thrown during a partial/final reduce are handled correctly.

I see, you mean the condition that the test had to trigger the failure, which was around serialization?

javanna · 2020-09-23T10:08:13Z

server/src/test/java/org/elasticsearch/action/search/TransportSearchActionSingleNodeTests.java

@@ -1,177 +0,0 @@
-/*
- * Licensed to Elasticsearch under one or more contributor


it is a bit of a shame that these go from single node tests to full blown IT tests, what is the reasoning behind this choice?

nik9000 · 2020-09-23T12:06:50Z

Sorry not to comment publicly about the serialization change! It makes me sad not to serialize but I see the reasoning.

jimczi · 2020-09-24T06:37:56Z

@elasticmachine run elasticsearch-ci/2

Ensures that the test always run with a memory circuit breaker. Relates #62223

jimczi added >enhancement release highlight :Analytics/Geo Indexing, search aggregations of geo points and shapes :Core/Infra/Circuit Breakers Track estimates of memory consumption to prevent overload v8.0.0 v7.10.0 labels Sep 10, 2020

jimczi requested review from nik9000 and javanna September 10, 2020 13:29

elasticmachine added Team:Core/Infra Meta label for core/infra team Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Sep 10, 2020

jimczi commented Sep 10, 2020

View reviewed changes

jimczi added 4 commits September 10, 2020 15:32

style

2a719fa

Merge branch 'master' into enhancements/reduce_aggs_circuit_breaker

74671a3

style

62decd3

fix unit test

e883199

nik9000 reviewed Sep 14, 2020

View reviewed changes

jimczi added 4 commits September 15, 2020 09:49

Merge branch 'master' into enhancements/reduce_aggs_circuit_breaker

021f89c

add a seed for the aggs benchmark

8c19f48

fix ut

24e708f

jimczi added 2 commits September 15, 2020 22:02

Merge branch 'master' into enhancements/reduce_aggs_circuit_breaker

4ae09cb

adapt new code after master merge

599f01d

nik9000 approved these changes Sep 17, 2020

View reviewed changes

jimczi added 3 commits September 17, 2020 17:57

Merge branch 'master' into enhancements/reduce_aggs_circuit_breaker

0a78ac8

address review feedback

7178dab

asserting doesn't work with tests, revert. Fix tests after merge with…

8f69584

… master.

javanna reviewed Sep 23, 2020

View reviewed changes

javanna approved these changes Sep 23, 2020

View reviewed changes

jimczi added 2 commits September 23, 2020 17:33

Merge branch 'master' into enhancements/reduce_aggs_circuit_breaker

ae90508

fix compilation after merge with master

400e77a

jimczi merged commit fbed2a1 into elastic:master Sep 24, 2020

jimczi deleted the enhancements/reduce_aggs_circuit_breaker branch September 24, 2020 12:02

jimczi added a commit that referenced this pull request Sep 24, 2020

Fix TransportSearchIT#testCircuitBreakerReduceFail

9231981

Ensures that the test always run with a memory circuit breaker. Relates #62223

jimczi mentioned this pull request Sep 24, 2020

Request-level circuit breaker support on coordinating nodes #62884

Merged

$@polyfractal$ polyfractal mentioned this pull request Jan 13, 2021

Add circuit breaker for memory used by a request's calculations #67476

Closed

maosuhan mentioned this pull request Feb 22, 2021

Add partial reduce nodes for reducing intermediate aggregation results #56748

Open

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

stefnestor mentioned this pull request Apr 13, 2022

[Question/FR] Index Pressure as Circuit Breaker #85853

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Request-level circuit breaker support on coordinating nodes #62223

Request-level circuit breaker support on coordinating nodes #62223

jimczi commented Sep 10, 2020 •

edited by pugnascotia

Loading

elasticmachine commented Sep 10, 2020

elasticmachine commented Sep 10, 2020

jimczi Sep 10, 2020

javanna Sep 23, 2020

jimczi Sep 23, 2020

nik9000 left a comment

jimczi commented Sep 15, 2020

nik9000 left a comment

javanna left a comment

javanna Sep 23, 2020

jimczi Sep 23, 2020

javanna Sep 23, 2020

jimczi Sep 24, 2020

javanna Sep 23, 2020

nik9000 commented Sep 23, 2020

jimczi commented Sep 24, 2020

		@@ -1,177 +0,0 @@
		/*
		* Licensed to Elasticsearch under one or more contributor

Request-level circuit breaker support on coordinating nodes #62223

Request-level circuit breaker support on coordinating nodes #62223

Conversation

jimczi commented Sep 10, 2020 • edited by pugnascotia Loading

elasticmachine commented Sep 10, 2020

elasticmachine commented Sep 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 left a comment

Choose a reason for hiding this comment

jimczi commented Sep 15, 2020

nik9000 left a 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

nik9000 commented Sep 23, 2020

jimczi commented Sep 24, 2020

jimczi commented Sep 10, 2020 •

edited by pugnascotia

Loading