Skip to content

Aggs: Add real memory CB call when building internal aggregators in buckets #116329

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Nov 7, 2024
Merged

Aggs: Add real memory CB call when building internal aggregators in buckets #116329

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
57c0b6e
Add circuit breaker call on buckets aggregator internal aggs building
ivancea Nov 6, 2024
9794d58
Format
ivancea Nov 6, 2024
48f29d9
Moved CB check inside loop
ivancea Nov 6, 2024
9165205
Updated javadocs and called CB before building aggs
ivancea Nov 6, 2024
620d8a8
Merge branch 'main' into bucket-aggs-real-memory-break-internal
elasticmachine Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions server/src/main/java/org/elasticsearch/search/aggregations/bucket/BucketsAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,7 @@ public final void collectBucket(LeafBucketCollector subCollector, int doc, long
grow(bucketOrd + 1);
int docCount = docCountProvider.getDocCount(doc);
if (docCounts.increment(bucketOrd, docCount) == docCount) {
// We call the circuit breaker the time to time in order to give it a chance to check available
// 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
if ((++callCount & 0x3FF) == 0) {
breaker.addEstimateBytesAndMaybeBreak(0, "allocated_buckets");
}
updateCircuitBreaker("allocated_buckets");
}
subCollector.collect(doc, bucketOrd);
}
Expand Down Expand Up @@ -179,6 +174,7 @@ protected final IntFunction<InternalAggregations> buildSubAggsForBuckets(long[]
prepareSubAggs(bucketOrdsToCollect);
InternalAggregation[][] aggregations = new InternalAggregation[subAggregators.length][];
for (int i = 0; i < subAggregators.length; i++) {
updateCircuitBreaker("building_sub_aggregation");
aggregations[i] = subAggregators[i].buildAggregations(bucketOrdsToCollect);
}
return subAggsForBucketFunction(aggregations);
Expand Down Expand Up @@ -415,4 +411,15 @@ protected void preGetSubLeafCollectors(LeafReaderContext ctx) throws IOException
// Set LeafReaderContext to the doc_count provider
docCountProvider.setLeafReaderContext(ctx);
}

/**
* This method calls the circuit breaker from time to time in order to give it a chance to check available
* memory in the parent breaker (Which should be a real memory 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 •
edited
Loading

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.

if ((++callCount & 0x3FF) == 0) {
breaker.addEstimateBytesAndMaybeBreak(0, label);
}
}
}