Skip to content

Commit 378fef8

Browse files
authored
ESQL: Fix a breaker bug (#136105)
Fixes a case where we release memory we never acquired, which breaker the circuit breaker accounting. Closes #135224 Closes #135260
1 parent 7cf0f6c commit 378fef8

File tree

3 files changed

+14
-9
lines changed

3 files changed

+14
-9
lines changed

docs/changelog/136105.yaml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
pr: 136105
2+
summary: Fix a breaker bug
3+
area: ES|QL
4+
type: bug
5+
issues:
6+
- 135224
7+
- 135260

muted-tests.yml

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -525,12 +525,6 @@ tests:
525525
- class: org.elasticsearch.xpack.test.rest.XPackRestIT
526526
method: test {p0=analytics/nested_top_metrics_sort/terms order by top metrics numeric not null integer values}
527527
issue: https://github.com/elastic/elasticsearch/issues/135162
528-
- class: org.elasticsearch.compute.operator.topn.TopNOperatorTests
529-
method: testSimpleWithCranky
530-
issue: https://github.com/elastic/elasticsearch/issues/135224
531-
- class: org.elasticsearch.compute.operator.topn.TopNOperatorTests
532-
method: testSimpleCircuitBreaking
533-
issue: https://github.com/elastic/elasticsearch/issues/135260
534528
- class: org.elasticsearch.xpack.esql.qa.single_node.PushQueriesIT
535529
method: testEqualityAndOther {SEMANTIC_TEXT_WITH_KEYWORD}
536530
issue: https://github.com/elastic/elasticsearch/issues/135333

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/TopNOperator.java

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,10 +84,10 @@ static final class Row implements Accountable, Releasable {
8484
RefCounted shardRefCounter;
8585

8686
Row(CircuitBreaker breaker, List<SortOrder> sortOrders, int preAllocatedKeysSize, int preAllocatedValueSize) {
87+
breaker.addEstimateBytesAndMaybeBreak(SHALLOW_SIZE, "topn");
8788
this.breaker = breaker;
8889
boolean success = false;
8990
try {
90-
breaker.addEstimateBytesAndMaybeBreak(SHALLOW_SIZE, "topn");
9191
keys = new BreakingBytesRefBuilder(breaker, "topn", preAllocatedKeysSize);
9292
values = new BreakingBytesRefBuilder(breaker, "topn", preAllocatedValueSize);
9393
bytesOrder = new BytesOrder(sortOrders, breaker, "topn");
@@ -684,8 +684,12 @@ public long ramBytesUsed() {
684684

685685
@Override
686686
public void close() {
687-
Releasables.close(Releasables.wrap(this), () -> breaker.addWithoutBreaking(-Queue.sizeOf(topCount)));
688-
687+
Releasables.close(
688+
// Release all entries in the topn
689+
Releasables.wrap(this),
690+
// Release the array itself
691+
() -> breaker.addWithoutBreaking(-Queue.sizeOf(topCount))
692+
);
689693
}
690694

691695
public static long sizeOf(int topCount) {

0 commit comments

Comments
 (0)