Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aggregations enhancement - better memory usage estimates to avoid circuit breaking #28220

Closed
markharwood opened this issue Jan 15, 2018 · 3 comments
Labels
:Analytics/Aggregations Aggregations >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)

Comments

@markharwood
Copy link
Contributor

In a recent discuss forum post a user reported circuit breaking exceptions when using a large number of avg aggregations nested under two layers of terms aggregations.
Internally, the estimated memory cost of handling the request was (badly) estimated to be nearly 3GB in size but the actual size should have been much smaller. In part, this is due to a one-size-fits-all cost which is added for every aggregation which is hard-coded to 5kb in the aggregator base class.

For some aggregations this is often an underestimate (e.g. cardinality agg) while for simple metric aggregations like the avg aggregation it is a large over-estimate (which led to the circuit breaker error in the case mentioned above).

The solution requires

  1. A change to AggregatorBase to allow subclasses to override a new "getWeight" method.
  2. Changes to metric aggregations like avg to provide a better estimate of their memory cost.
@markharwood
Copy link
Contributor Author

Follow up - we discussed the particular query that led to this issue and were not convinced it wasn't an abusive query. An estimate of a true final response size was 400mb which led us to consider that the reported over-estimate for the size of the interim working state was in this case not a bad call given the final state produced by the coordinating node would be even larger and (currently) not subjected to a circuit breaker.
It will always be tough to efficiently and accurately estimate sizes (especially when breadth-first and depth-first modes come into play) so perhaps the existing algorithm's over-estimation remains a healthy choice of setting?

@polyfractal
Copy link
Contributor

@elastic/es-search-aggs

@rjernst rjernst added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label May 4, 2020
@polyfractal
Copy link
Contributor

I think this can be closed, now that we are relying heavily on the real-memory circuit breaker in #46751 and #57042

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)
Projects
None yet
Development

No branches or pull requests

3 participants