Skip to content

Conversation

ivancea
Copy link
Contributor

@ivancea ivancea commented Nov 6, 2024

Related with #88128

This PR pretends to reduce the potential OOMs received when building internal aggregations.

@ivancea ivancea added >non-issue :Analytics/Aggregations Aggregations Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) auto-backport Automatically create backport pull requests when merged v9.0.0 v8.17.0 labels Nov 6, 2024
@ivancea ivancea requested a review from iverase November 6, 2024 13:28
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

* memory in the parent breaker and break the execution if we are running out.
* To achieve that, we are passing 0 as the estimated bytes every 1024 calls
*/
private void updateCircuitBreaker(String label) {
Copy link
Contributor

@iverase iverase Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets add parent in the method name so it is clear the purpose? like checkParentCircuitBreaker?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand what do we mean by "parent" here. We're updating the current CB. Parent of what?
That copied javadoc says parent too, but I'm not sure what it is

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have different circuit breakers with different thresholds. All those circuit breakers are below another breaker that is called the parent. In the past, the parent circuit breaker was a global threshold, so even if all CBs did not hit their individual threshold, summing them up will hit a global threshold (the threshold of the parent) and then the request would trip.

More recently, we introduced the real memory CB which replaced the parent circuit breaker (you can disable the real memory CB and you get the old behaviour). In this case, instead of checking a global threshold, we check the actual memory usage and will trip if we are over a threshold after trying to free some heap,

Therefore adding 0 bytes to the CB only has effect if we check the real memory, otherwise is a NOOP.

@ivancea
Copy link
Contributor Author

ivancea commented Nov 6, 2024

@elasticmachine update branch

@ivancea ivancea merged commit a3eba57 into elastic:main Nov 7, 2024
16 checks passed
ivancea added a commit to ivancea/elasticsearch that referenced this pull request Nov 7, 2024
…uckets (elastic#116329)

Related with elastic#88128

This PR pretends to reduce the potential OOMs received when building internal aggregations.
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

@ivancea ivancea deleted the bucket-aggs-real-memory-break-internal branch November 7, 2024 10:26
elasticsearchmachine pushed a commit that referenced this pull request Nov 7, 2024
…uckets (#116329) (#116393)

Related with #88128

This PR pretends to reduce the potential OOMs received when building internal aggregations.
kderusso pushed a commit to kderusso/elasticsearch that referenced this pull request Nov 7, 2024
…uckets (elastic#116329)

Related with elastic#88128

This PR pretends to reduce the potential OOMs received when building internal aggregations.
jozala pushed a commit that referenced this pull request Nov 13, 2024
…uckets (#116329)

Related with #88128

This PR pretends to reduce the potential OOMs received when building internal aggregations.
alexey-ivanov-es pushed a commit to alexey-ivanov-es/elasticsearch that referenced this pull request Nov 28, 2024
…uckets (elastic#116329)

Related with elastic#88128

This PR pretends to reduce the potential OOMs received when building internal aggregations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/Aggregations Aggregations auto-backport Automatically create backport pull requests when merged >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.17.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants