ES|QL: Integrate circuit breaker into BestBucketsDeferringCollector by hawkhuang-collab · Pull Request #155600 · elastic/elasticsearch

hawkhuang-collab · 2026-07-31T10:35:05Z

BestBucketsDeferringCollector stores PackedLongValues (doc deltas and bucket ordinals) for every collected document with no circuit breaker tracking. Under high-cardinality deferring aggregations (rare-terms, auto-date-histogram, variable-width-histogram), this can exhaust heap (issue #148516: 5.8GB of 8GB observed).

Add a LongConsumer bytesAccounter to the constructor. Callers pass this::addRequestCircuitBreakerBytes from the owning aggregator so bytes flow through requestBytesUsed and are automatically returned when the aggregator closes, even on query failure.

finishLeaf(): charge ramBytesUsed of each committed entry
prepareSelectedBuckets(): return bytes as entry data is freed
rewriteBuckets(): return old entry bytes, charge new entry bytes
collect(): periodic zero-byte check lets the parent real-memory breaker catch in-flight builder growth between finishLeaf calls

elasticsearchmachine · 2026-07-31T10:35:51Z

Hi @hawkhuang-collab, I've created a changelog YAML for you.

github-actions · 2026-07-31T10:36:55Z

🔍 Preview links for changed docs

⏳ Building and deploying preview... View progress

This comment will be updated with preview links when the build is complete.

github-actions · 2026-07-31T10:38:20Z

ℹ️ Important: Docs version tagging

👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version.

We use applies_to tags to mark version-specific features and changes.

Expand for a quick overview

When to use applies_to tags:

✅ At the page level to indicate which products/deployments the content applies to (mandatory)
✅ When features change state (e.g. preview, ga) in a specific version
✅ When availability differs across deployments and environments

What NOT to do:

❌ Don't remove or replace information that applies to an older version
❌ Don't add new information that applies to a specific version without an applies_to tag
❌ Don't forget that applies_to tags can be used at the page, section, and inline level

🤔 Need help?

Check out the cumulative docs guidelines
Reach out in the #docs Slack channel

elasticsearchmachine · 2026-08-04T10:51:58Z

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

BestBucketsDeferringCollector stores PackedLongValues (doc deltas and bucket ordinals) for every collected document with no circuit breaker tracking. Under high-cardinality deferring aggregations (rare-terms, auto-date-histogram, variable-width-histogram), this can exhaust heap (issue elastic#148516: 5.8GB of 8GB observed). Add a LongConsumer bytesAccounter to the constructor. Callers pass this::addRequestCircuitBreakerBytes from the owning aggregator so bytes flow through requestBytesUsed and are automatically returned when the aggregator closes, even on query failure. - finishLeaf(): charge ramBytesUsed of each committed entry - prepareSelectedBuckets(): return bytes as entry data is freed - rewriteBuckets(): return old entry bytes, charge new entry bytes - collect(): periodic zero-byte check lets the parent real-memory breaker catch in-flight builder growth between finishLeaf calls Fixes: elastic#148516

Add three tests that exercise the byte-tracking added in the parent commit: - testCircuitBreakerBytesChargedAndReturnedAfterReplay: verifies that the running balance is positive after collection and reaches zero after prepareSelectedBuckets returns all entry bytes. - testCircuitBreakerBytesAdjustedByRewriteBuckets: verifies that rewriteBuckets returns old entry bytes and recharges for rebuilt entries, with the balance still reaching zero after replay. - testCircuitBreakerTripDuringFinishLeaf: verifies that a CircuitBreakingException thrown by the bytesAccounter propagates correctly out of postCollection.

- Add Objects.requireNonNull for bytesAccounter in the constructor so a missing callback surfaces immediately at construction rather than as a NullPointerException deep inside finishLeaf. - Expand the rewriteBuckets comment to accurately describe how long the old packed structures remain live (the full rebuild loop, not just a brief moment) and document the exception-safety property: a CircuitBreakingException mid-rebuild leaves entries pointing at the old list, fails the query, and lets AggregatorBase.close() reconcile the breaker on the way out.

Replace the AtomicLong-based tests with event-list tests that capture every individual charge and return call: - testCircuitBreakerChargesOneEventPerSegmentAndReleasesSymmetrically: asserts exactly one positive event per segment after collection, and that each subsequent negative event equals the exact negation of its corresponding charge (not just that the final sum is zero). - testCircuitBreakerRewriteBucketsProducesSymmetricEvents: asserts the full 6-event sequence [+A, -A, +A', +B, -A', -B], including that the rebuilt entry A' is smaller than A (all-zero bucket array compresses better than distinct ordinals 0-4).

Cover six boundary conditions not exercised by the existing tests: - empty segment (no collect() calls) produces no CB event - prepareSelectedBuckets with no matching ordinals still returns bytes - rewriteBuckets on empty entries fires no events - rewriteBuckets mapping all ordinals to -1 returns bytes without recharging - two successive rewriteBuckets calls maintain symmetric accounting - zero-byte heartbeat events (>1024 docs) don't affect the net balance

ivancea · 2026-08-05T14:52:08Z

Is this worth a cranky breaker test? Not sure how/if we test with cranky breaker other aggs, but I would give it a look. This feels safe as it uses addRequestCircuitBreakerBytes though

It's worth making sure we have cranky tests, yeah. We wrote the first cranky breaker for aggs, so it should be available. We might even get it by default.

I'll look into this.

Good insight and time for me to search and learn cranky breaker tests! :)

Ok, so this is a design trade off/ design decision, and need your expert suggestion.
In our current implementation, we use LongConsumer, which is more flexible and decoupled and we used in many existing aggregator such as: MapStringTermsAggregator, the downside/ problem is that then we can't use the existing test coverage as it has to use CircuitBreaker directly..

I've added a manual cranky test there for now.

GalLalouche · 2026-08-05T15:08:28Z

                lastDoc = doc;
+                // Periodically give the real-memory parent breaker a chance to check heap
+                // usage for in-flight builder allocations that are not yet committed to entries.
+                if ((++callCount & 0x3FF) == 0) {


Can we avoid the magic bitwise operations? The compiler should be smart enough to figure this out on its own :) and if can't for whatever (verified) reason, this needs a comment explaining what it's doing.

Yeah, make sense, will update it.

Meh. I read this as "This fires every thousand of so calls and that caller cares very much about this line being fast."

My bitwise-fu is very, very weak. I see something like this and I immediately go cross-eyed like Austin Powers trying to understand time-travel. Even if this was warranted, a simple comment like what you just said would suffice.

Thanks for the feedback, I'll keep the bitwise check since this is a hot-path optimization, and I'll add a comment explaining that it's intentionally checking every 1024 calls.

GalLalouche · 2026-08-05T15:11:05Z

+     * aggregator's tracker means bytes flow through {@code requestBytesUsed} and are automatically
+     * returned when the aggregator is closed, even if the query fails before
+     * {@link #prepareSelectedBuckets} is called.
+     */


The javadoc for this this is repeated (i none form or another) multiple times. And either way, is it really this class's responsibility how this is used? Perhaps just giving this a more descriptive name should suffice.

Let me remove the javadoc, I think the name is already self explained enough, and yes, this is the right place and right type as well.

GalLalouche · 2026-08-05T15:12:08Z

     * Sole constructor.
     * @param isGlobal Whether this collector visits all documents (global context)
+     * @param bytesAccounter Callback used to charge and return circuit breaker bytes.
+     *                       Pass {@code aggregator::addRequestCircuitBreakerBytes} so that


This javadoc feels very smelly to me. If you expect this to be a very specific callback, then it should be enforced in more than just a Javadoc comment.

GalLalouche · 2026-08-05T15:13:17Z

+            // Charge the circuit breaker for the packed values we just committed. If this
+            // trips, the exception propagates out of getLeafCollector and the aggregator
+            // framework handles cleanup via AggregatorBase.close().
+            bytesAccounter.accept(entry.ramBytesUsed);


Comment would be redundant if bytesAccounter had a better name, and either way, this feels like a very "The AI wrote this code to explain to me why it chose the solution it did, but there's no reason to keep this for posterity" kind of comment :)

You got it.

GalLalouche · 2026-08-05T15:13:54Z

+        // second loop, entries still points at the old list whose bytes have already
+        // been returned; the resulting CircuitBreakingException propagates to the
+        // caller, failing the query, and AggregatorBase.close() reconciles
+        // requestBytesUsed with the breaker on the way out.


Same thing here: feels like "AI is explaining their solution" more than a comment that should be kept for all time.

Let me update it

GalLalouche · 2026-08-05T15:15:00Z

+                for (int i = 0; i < 5; i++) {
+                    indexWriter.addDocument(new Document());
+                }
+            }


These tests are very long long and duplicate a lot of their setup. Could you please refactor this so it's DRYer and also easier to review?

Yeah, fair point, let me work on it.

GalLalouche

LGTM with a few remaining suggestions. Thanks @hawkhuang-collab!

…-breaker

elasticsearchmachine · 2026-08-14T12:07:51Z

💚 Backport successful

Status	Branch	Result
✅	9.4
✅	9.5

…lastic#155600) * Integrate circuit breaker into BestBucketsDeferringCollector BestBucketsDeferringCollector stores PackedLongValues (doc deltas and bucket ordinals) for every collected document with no circuit breaker tracking. Under high-cardinality deferring aggregations (rare-terms, auto-date-histogram, variable-width-histogram), this can exhaust heap (issue elastic#148516: 5.8GB of 8GB observed). Add a LongConsumer bytesAccounter to the constructor. Callers pass this::addRequestCircuitBreakerBytes from the owning aggregator so bytes flow through requestBytesUsed and are automatically returned when the aggregator closes, even on query failure. - finishLeaf(): charge ramBytesUsed of each committed entry - prepareSelectedBuckets(): return bytes as entry data is freed - rewriteBuckets(): return old entry bytes, charge new entry bytes - collect(): periodic zero-byte check lets the parent real-memory breaker catch in-flight builder growth between finishLeaf calls Fixes: elastic#148516 * Update docs/changelog/155600.yaml * Test circuit breaker accounting in BestBucketsDeferringCollector Add three tests that exercise the byte-tracking added in the parent commit: - testCircuitBreakerBytesChargedAndReturnedAfterReplay: verifies that the running balance is positive after collection and reaches zero after prepareSelectedBuckets returns all entry bytes. - testCircuitBreakerBytesAdjustedByRewriteBuckets: verifies that rewriteBuckets returns old entry bytes and recharges for rebuilt entries, with the balance still reaching zero after replay. - testCircuitBreakerTripDuringFinishLeaf: verifies that a CircuitBreakingException thrown by the bytesAccounter propagates correctly out of postCollection. * Polish BestBucketsDeferringCollector: null-check and comment accuracy - Add Objects.requireNonNull for bytesAccounter in the constructor so a missing callback surfaces immediately at construction rather than as a NullPointerException deep inside finishLeaf. - Expand the rewriteBuckets comment to accurately describe how long the old packed structures remain live (the full rebuild loop, not just a brief moment) and document the exception-safety property: a CircuitBreakingException mid-rebuild leaves entries pointing at the old list, fails the query, and lets AggregatorBase.close() reconcile the breaker on the way out. * [CI] Auto commit changes from spotless * Strengthen circuit breaker tests to verify event symmetry Replace the AtomicLong-based tests with event-list tests that capture every individual charge and return call: - testCircuitBreakerChargesOneEventPerSegmentAndReleasesSymmetrically: asserts exactly one positive event per segment after collection, and that each subsequent negative event equals the exact negation of its corresponding charge (not just that the final sum is zero). - testCircuitBreakerRewriteBucketsProducesSymmetricEvents: asserts the full 6-event sequence [+A, -A, +A', +B, -A', -B], including that the rebuilt entry A' is smaller than A (all-zero bucket array compresses better than distinct ordinals 0-4). * Add edge-case circuit breaker tests for BestBucketsDeferringCollector Cover six boundary conditions not exercised by the existing tests: - empty segment (no collect() calls) produces no CB event - prepareSelectedBuckets with no matching ordinals still returns bytes - rewriteBuckets on empty entries fires no events - rewriteBuckets mapping all ordinals to -1 returns bytes without recharging - two successive rewriteBuckets calls maintain symmetric accounting - zero-byte heartbeat events (>1024 docs) don't affect the net balance * Address review comments on BestBucketsDeferringCollector * Refactor circuit breaker tests to reduce duplication * simplify to inline. * Add cranky circuit breaker test for BestBucketsDeferringCollector * Extract runBreakerTest helper to eliminate index setup boilerplate * [CI] Auto commit changes from spotless * Extract withSearcher helper for custom-accounter tests * Polish cranky test and CollectingBucketCollector * Trim finishLeaf comment to the non-obvious why * Assert byte balance after circuit breaker trips in tests * Remove unused default from withSearcher * Move zero-sum invariant into runBreakerTest and make helpers static * Clarify accept(0) heartbeat protocol in collect loop * remove ghost * Extract heartbeat() method from getLeafCollector --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

…155600) (#156781) * Integrate circuit breaker into BestBucketsDeferringCollector BestBucketsDeferringCollector stores PackedLongValues (doc deltas and bucket ordinals) for every collected document with no circuit breaker tracking. Under high-cardinality deferring aggregations (rare-terms, auto-date-histogram, variable-width-histogram), this can exhaust heap (issue #148516: 5.8GB of 8GB observed). Add a LongConsumer bytesAccounter to the constructor. Callers pass this::addRequestCircuitBreakerBytes from the owning aggregator so bytes flow through requestBytesUsed and are automatically returned when the aggregator closes, even on query failure. - finishLeaf(): charge ramBytesUsed of each committed entry - prepareSelectedBuckets(): return bytes as entry data is freed - rewriteBuckets(): return old entry bytes, charge new entry bytes - collect(): periodic zero-byte check lets the parent real-memory breaker catch in-flight builder growth between finishLeaf calls Fixes: #148516 * Update docs/changelog/155600.yaml * Test circuit breaker accounting in BestBucketsDeferringCollector Add three tests that exercise the byte-tracking added in the parent commit: - testCircuitBreakerBytesChargedAndReturnedAfterReplay: verifies that the running balance is positive after collection and reaches zero after prepareSelectedBuckets returns all entry bytes. - testCircuitBreakerBytesAdjustedByRewriteBuckets: verifies that rewriteBuckets returns old entry bytes and recharges for rebuilt entries, with the balance still reaching zero after replay. - testCircuitBreakerTripDuringFinishLeaf: verifies that a CircuitBreakingException thrown by the bytesAccounter propagates correctly out of postCollection. * Polish BestBucketsDeferringCollector: null-check and comment accuracy - Add Objects.requireNonNull for bytesAccounter in the constructor so a missing callback surfaces immediately at construction rather than as a NullPointerException deep inside finishLeaf. - Expand the rewriteBuckets comment to accurately describe how long the old packed structures remain live (the full rebuild loop, not just a brief moment) and document the exception-safety property: a CircuitBreakingException mid-rebuild leaves entries pointing at the old list, fails the query, and lets AggregatorBase.close() reconcile the breaker on the way out. * [CI] Auto commit changes from spotless * Strengthen circuit breaker tests to verify event symmetry Replace the AtomicLong-based tests with event-list tests that capture every individual charge and return call: - testCircuitBreakerChargesOneEventPerSegmentAndReleasesSymmetrically: asserts exactly one positive event per segment after collection, and that each subsequent negative event equals the exact negation of its corresponding charge (not just that the final sum is zero). - testCircuitBreakerRewriteBucketsProducesSymmetricEvents: asserts the full 6-event sequence [+A, -A, +A', +B, -A', -B], including that the rebuilt entry A' is smaller than A (all-zero bucket array compresses better than distinct ordinals 0-4). * Add edge-case circuit breaker tests for BestBucketsDeferringCollector Cover six boundary conditions not exercised by the existing tests: - empty segment (no collect() calls) produces no CB event - prepareSelectedBuckets with no matching ordinals still returns bytes - rewriteBuckets on empty entries fires no events - rewriteBuckets mapping all ordinals to -1 returns bytes without recharging - two successive rewriteBuckets calls maintain symmetric accounting - zero-byte heartbeat events (>1024 docs) don't affect the net balance * Address review comments on BestBucketsDeferringCollector * Refactor circuit breaker tests to reduce duplication * simplify to inline. * Add cranky circuit breaker test for BestBucketsDeferringCollector * Extract runBreakerTest helper to eliminate index setup boilerplate * [CI] Auto commit changes from spotless * Extract withSearcher helper for custom-accounter tests * Polish cranky test and CollectingBucketCollector * Trim finishLeaf comment to the non-obvious why * Assert byte balance after circuit breaker trips in tests * Remove unused default from withSearcher * Move zero-sum invariant into runBreakerTest and make helpers static * Clarify accept(0) heartbeat protocol in collect loop * remove ghost * Extract heartbeat() method from getLeafCollector --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

…lastic#155600) * Integrate circuit breaker into BestBucketsDeferringCollector BestBucketsDeferringCollector stores PackedLongValues (doc deltas and bucket ordinals) for every collected document with no circuit breaker tracking. Under high-cardinality deferring aggregations (rare-terms, auto-date-histogram, variable-width-histogram), this can exhaust heap (issue elastic#148516: 5.8GB of 8GB observed). Add a LongConsumer bytesAccounter to the constructor. Callers pass this::addRequestCircuitBreakerBytes from the owning aggregator so bytes flow through requestBytesUsed and are automatically returned when the aggregator closes, even on query failure. - finishLeaf(): charge ramBytesUsed of each committed entry - prepareSelectedBuckets(): return bytes as entry data is freed - rewriteBuckets(): return old entry bytes, charge new entry bytes - collect(): periodic zero-byte check lets the parent real-memory breaker catch in-flight builder growth between finishLeaf calls Fixes: elastic#148516 * Update docs/changelog/155600.yaml * Test circuit breaker accounting in BestBucketsDeferringCollector Add three tests that exercise the byte-tracking added in the parent commit: - testCircuitBreakerBytesChargedAndReturnedAfterReplay: verifies that the running balance is positive after collection and reaches zero after prepareSelectedBuckets returns all entry bytes. - testCircuitBreakerBytesAdjustedByRewriteBuckets: verifies that rewriteBuckets returns old entry bytes and recharges for rebuilt entries, with the balance still reaching zero after replay. - testCircuitBreakerTripDuringFinishLeaf: verifies that a CircuitBreakingException thrown by the bytesAccounter propagates correctly out of postCollection. * Polish BestBucketsDeferringCollector: null-check and comment accuracy - Add Objects.requireNonNull for bytesAccounter in the constructor so a missing callback surfaces immediately at construction rather than as a NullPointerException deep inside finishLeaf. - Expand the rewriteBuckets comment to accurately describe how long the old packed structures remain live (the full rebuild loop, not just a brief moment) and document the exception-safety property: a CircuitBreakingException mid-rebuild leaves entries pointing at the old list, fails the query, and lets AggregatorBase.close() reconcile the breaker on the way out. * [CI] Auto commit changes from spotless * Strengthen circuit breaker tests to verify event symmetry Replace the AtomicLong-based tests with event-list tests that capture every individual charge and return call: - testCircuitBreakerChargesOneEventPerSegmentAndReleasesSymmetrically: asserts exactly one positive event per segment after collection, and that each subsequent negative event equals the exact negation of its corresponding charge (not just that the final sum is zero). - testCircuitBreakerRewriteBucketsProducesSymmetricEvents: asserts the full 6-event sequence [+A, -A, +A', +B, -A', -B], including that the rebuilt entry A' is smaller than A (all-zero bucket array compresses better than distinct ordinals 0-4). * Add edge-case circuit breaker tests for BestBucketsDeferringCollector Cover six boundary conditions not exercised by the existing tests: - empty segment (no collect() calls) produces no CB event - prepareSelectedBuckets with no matching ordinals still returns bytes - rewriteBuckets on empty entries fires no events - rewriteBuckets mapping all ordinals to -1 returns bytes without recharging - two successive rewriteBuckets calls maintain symmetric accounting - zero-byte heartbeat events (>1024 docs) don't affect the net balance * Address review comments on BestBucketsDeferringCollector * Refactor circuit breaker tests to reduce duplication * simplify to inline. * Add cranky circuit breaker test for BestBucketsDeferringCollector * Extract runBreakerTest helper to eliminate index setup boilerplate * [CI] Auto commit changes from spotless * Extract withSearcher helper for custom-accounter tests * Polish cranky test and CollectingBucketCollector * Trim finishLeaf comment to the non-obvious why * Assert byte balance after circuit breaker trips in tests * Remove unused default from withSearcher * Move zero-sum invariant into runBreakerTest and make helpers static * Clarify accept(0) heartbeat protocol in collect loop * remove ghost * Extract heartbeat() method from getLeafCollector --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

…155600) (#156780) * Integrate circuit breaker into BestBucketsDeferringCollector BestBucketsDeferringCollector stores PackedLongValues (doc deltas and bucket ordinals) for every collected document with no circuit breaker tracking. Under high-cardinality deferring aggregations (rare-terms, auto-date-histogram, variable-width-histogram), this can exhaust heap (issue #148516: 5.8GB of 8GB observed). Add a LongConsumer bytesAccounter to the constructor. Callers pass this::addRequestCircuitBreakerBytes from the owning aggregator so bytes flow through requestBytesUsed and are automatically returned when the aggregator closes, even on query failure. - finishLeaf(): charge ramBytesUsed of each committed entry - prepareSelectedBuckets(): return bytes as entry data is freed - rewriteBuckets(): return old entry bytes, charge new entry bytes - collect(): periodic zero-byte check lets the parent real-memory breaker catch in-flight builder growth between finishLeaf calls Fixes: #148516 * Update docs/changelog/155600.yaml * Test circuit breaker accounting in BestBucketsDeferringCollector Add three tests that exercise the byte-tracking added in the parent commit: - testCircuitBreakerBytesChargedAndReturnedAfterReplay: verifies that the running balance is positive after collection and reaches zero after prepareSelectedBuckets returns all entry bytes. - testCircuitBreakerBytesAdjustedByRewriteBuckets: verifies that rewriteBuckets returns old entry bytes and recharges for rebuilt entries, with the balance still reaching zero after replay. - testCircuitBreakerTripDuringFinishLeaf: verifies that a CircuitBreakingException thrown by the bytesAccounter propagates correctly out of postCollection. * Polish BestBucketsDeferringCollector: null-check and comment accuracy - Add Objects.requireNonNull for bytesAccounter in the constructor so a missing callback surfaces immediately at construction rather than as a NullPointerException deep inside finishLeaf. - Expand the rewriteBuckets comment to accurately describe how long the old packed structures remain live (the full rebuild loop, not just a brief moment) and document the exception-safety property: a CircuitBreakingException mid-rebuild leaves entries pointing at the old list, fails the query, and lets AggregatorBase.close() reconcile the breaker on the way out. * [CI] Auto commit changes from spotless * Strengthen circuit breaker tests to verify event symmetry Replace the AtomicLong-based tests with event-list tests that capture every individual charge and return call: - testCircuitBreakerChargesOneEventPerSegmentAndReleasesSymmetrically: asserts exactly one positive event per segment after collection, and that each subsequent negative event equals the exact negation of its corresponding charge (not just that the final sum is zero). - testCircuitBreakerRewriteBucketsProducesSymmetricEvents: asserts the full 6-event sequence [+A, -A, +A', +B, -A', -B], including that the rebuilt entry A' is smaller than A (all-zero bucket array compresses better than distinct ordinals 0-4). * Add edge-case circuit breaker tests for BestBucketsDeferringCollector Cover six boundary conditions not exercised by the existing tests: - empty segment (no collect() calls) produces no CB event - prepareSelectedBuckets with no matching ordinals still returns bytes - rewriteBuckets on empty entries fires no events - rewriteBuckets mapping all ordinals to -1 returns bytes without recharging - two successive rewriteBuckets calls maintain symmetric accounting - zero-byte heartbeat events (>1024 docs) don't affect the net balance * Address review comments on BestBucketsDeferringCollector * Refactor circuit breaker tests to reduce duplication * simplify to inline. * Add cranky circuit breaker test for BestBucketsDeferringCollector * Extract runBreakerTest helper to eliminate index setup boilerplate * [CI] Auto commit changes from spotless * Extract withSearcher helper for custom-accounter tests * Polish cranky test and CollectingBucketCollector * Trim finishLeaf comment to the non-obvious why * Assert byte balance after circuit breaker trips in tests * Remove unused default from withSearcher * Move zero-sum invariant into runBreakerTest and make helpers static * Clarify accept(0) heartbeat protocol in collect loop * remove ghost * Extract heartbeat() method from getLeafCollector --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

…lastic#155600) * Integrate circuit breaker into BestBucketsDeferringCollector BestBucketsDeferringCollector stores PackedLongValues (doc deltas and bucket ordinals) for every collected document with no circuit breaker tracking. Under high-cardinality deferring aggregations (rare-terms, auto-date-histogram, variable-width-histogram), this can exhaust heap (issue elastic#148516: 5.8GB of 8GB observed). Add a LongConsumer bytesAccounter to the constructor. Callers pass this::addRequestCircuitBreakerBytes from the owning aggregator so bytes flow through requestBytesUsed and are automatically returned when the aggregator closes, even on query failure. - finishLeaf(): charge ramBytesUsed of each committed entry - prepareSelectedBuckets(): return bytes as entry data is freed - rewriteBuckets(): return old entry bytes, charge new entry bytes - collect(): periodic zero-byte check lets the parent real-memory breaker catch in-flight builder growth between finishLeaf calls Fixes: elastic#148516 * Update docs/changelog/155600.yaml * Test circuit breaker accounting in BestBucketsDeferringCollector Add three tests that exercise the byte-tracking added in the parent commit: - testCircuitBreakerBytesChargedAndReturnedAfterReplay: verifies that the running balance is positive after collection and reaches zero after prepareSelectedBuckets returns all entry bytes. - testCircuitBreakerBytesAdjustedByRewriteBuckets: verifies that rewriteBuckets returns old entry bytes and recharges for rebuilt entries, with the balance still reaching zero after replay. - testCircuitBreakerTripDuringFinishLeaf: verifies that a CircuitBreakingException thrown by the bytesAccounter propagates correctly out of postCollection. * Polish BestBucketsDeferringCollector: null-check and comment accuracy - Add Objects.requireNonNull for bytesAccounter in the constructor so a missing callback surfaces immediately at construction rather than as a NullPointerException deep inside finishLeaf. - Expand the rewriteBuckets comment to accurately describe how long the old packed structures remain live (the full rebuild loop, not just a brief moment) and document the exception-safety property: a CircuitBreakingException mid-rebuild leaves entries pointing at the old list, fails the query, and lets AggregatorBase.close() reconcile the breaker on the way out. * [CI] Auto commit changes from spotless * Strengthen circuit breaker tests to verify event symmetry Replace the AtomicLong-based tests with event-list tests that capture every individual charge and return call: - testCircuitBreakerChargesOneEventPerSegmentAndReleasesSymmetrically: asserts exactly one positive event per segment after collection, and that each subsequent negative event equals the exact negation of its corresponding charge (not just that the final sum is zero). - testCircuitBreakerRewriteBucketsProducesSymmetricEvents: asserts the full 6-event sequence [+A, -A, +A', +B, -A', -B], including that the rebuilt entry A' is smaller than A (all-zero bucket array compresses better than distinct ordinals 0-4). * Add edge-case circuit breaker tests for BestBucketsDeferringCollector Cover six boundary conditions not exercised by the existing tests: - empty segment (no collect() calls) produces no CB event - prepareSelectedBuckets with no matching ordinals still returns bytes - rewriteBuckets on empty entries fires no events - rewriteBuckets mapping all ordinals to -1 returns bytes without recharging - two successive rewriteBuckets calls maintain symmetric accounting - zero-byte heartbeat events (>1024 docs) don't affect the net balance * Address review comments on BestBucketsDeferringCollector * Refactor circuit breaker tests to reduce duplication * simplify to inline. * Add cranky circuit breaker test for BestBucketsDeferringCollector * Extract runBreakerTest helper to eliminate index setup boilerplate * [CI] Auto commit changes from spotless * Extract withSearcher helper for custom-accounter tests * Polish cranky test and CollectingBucketCollector * Trim finishLeaf comment to the non-obvious why * Assert byte balance after circuit breaker trips in tests * Remove unused default from withSearcher * Move zero-sum invariant into runBreakerTest and make helpers static * Clarify accept(0) heartbeat protocol in collect loop * remove ghost * Extract heartbeat() method from getLeafCollector --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

hawkhuang-collab added >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL labels Jul 31, 2026

elasticsearchmachine added the v9.6.0 label Jul 31, 2026

github-actions Bot deployed to docs-preview July 31, 2026 10:37 View deployment

github-actions Bot deployed to docs-preview July 31, 2026 12:38 View deployment

github-actions Bot deployed to docs-preview July 31, 2026 12:47 View deployment

github-actions Bot deployed to docs-preview July 31, 2026 13:58 View deployment

hawkhuang-collab marked this pull request as ready for review August 4, 2026 10:51

hawkhuang-collab requested review from idegtiarenko, ivancea and nik9000 and removed request for idegtiarenko August 4, 2026 15:39

hawkhuang-collab changed the title ~~Integrate circuit breaker into BestBucketsDeferringCollector~~ ES|QL: Integrate circuit breaker into BestBucketsDeferringCollector Aug 4, 2026

hawkhuang-collab and others added 7 commits August 4, 2026 21:26

Update docs/changelog/155600.yaml

5915689

[CI] Auto commit changes from spotless

e0b08a9

hawkhuang-collab requested a review from idegtiarenko August 5, 2026 14:39

ivancea reviewed Aug 5, 2026

View reviewed changes

GalLalouche requested changes Aug 5, 2026

View reviewed changes

Address review comments on BestBucketsDeferringCollector

c7a8156

hawkhuang-collab force-pushed the fix/best-buckets-deferring-collector-circuit-breaker branch from e96dc75 to c7a8156 Compare August 5, 2026 16:39

hawkhuang-collab requested review from GalLalouche and ivancea August 6, 2026 06:56

GalLalouche requested changes Aug 6, 2026

View reviewed changes

hawkhuang-collab added 5 commits August 6, 2026 10:50

Assert byte balance after circuit breaker trips in tests

6996a66

Remove unused default from withSearcher

58e66aa

Move zero-sum invariant into runBreakerTest and make helpers static

b8a3cf2

Clarify accept(0) heartbeat protocol in collect loop

30150ba

remove ghost

f7d802f

hawkhuang-collab requested a review from GalLalouche August 6, 2026 10:21

GalLalouche approved these changes Aug 6, 2026

View reviewed changes

hawkhuang-collab added 2 commits August 6, 2026 13:06

Extract heartbeat() method from getLeafCollector

460f5b4

Merge branch 'main' into fix/best-buckets-deferring-collector-circuit…

0966dda

…-breaker

hawkhuang-collab added v9.5.2 auto-backport Automatically create backport pull requests when merged v9.4.6 labels Aug 7, 2026

hawkhuang-collab added 5 commits August 13, 2026 08:52

Merge branch 'main' into fix/best-buckets-deferring-collector-circuit…

6233354

…-breaker

Merge branch 'main' into fix/best-buckets-deferring-collector-circuit…

13a448f

…-breaker

Merge branch 'main' into fix/best-buckets-deferring-collector-circuit…

09c7d8f

…-breaker

Merge branch 'main' into fix/best-buckets-deferring-collector-circuit…

0783bc9

…-breaker

Merge branch 'main' into fix/best-buckets-deferring-collector-circuit…

84303f0

…-breaker

hawkhuang-collab merged commit 9f03cc8 into elastic:main Aug 14, 2026
43 checks passed

This was referenced Aug 14, 2026

[9.4] ES|QL: Integrate circuit breaker into BestBucketsDeferringCollector (#155600) #156780

Merged

[9.5] ES|QL: Integrate circuit breaker into BestBucketsDeferringCollector (#155600) #156781

Merged

Conversation

hawkhuang-collab commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 31, 2026

Uh oh!

github-actions Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 Preview links for changed docs

Uh oh!

github-actions Bot commented Jul 31, 2026

ℹ️ Important: Docs version tagging

When to use applies_to tags:

What NOT to do:

🤔 Need help?

Uh oh!

elasticsearchmachine commented Aug 4, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hawkhuang-collab Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hawkhuang-collab Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GalLalouche left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hawkhuang-collab commented Jul 31, 2026 •

edited

Loading

github-actions Bot commented Jul 31, 2026 •

edited

Loading

hawkhuang-collab Aug 5, 2026 •

edited

Loading

hawkhuang-collab Aug 5, 2026 •

edited

Loading