Skip to content

Commit d695020

Browse files
original-brownbearoriginal-brownbear
authored
Remove lifecycle from CircuitBreakerService (#118699)
CircuitBreakerService and all its implementations has no lifecycle so we don't need to extend AbstractLifecycleComponent here. Mainly motivated by wanting to have a singleton CircuitBreakerService for some optimizations in query execution, but also a worthwhile cleanup in isolation I believe.
1 parent fee6165 commit d695020

File tree

11 files changed

+340
-393
lines changed

11 files changed

+340
-393
lines changed

‎server/src/main/java/org/elasticsearch/common/breaker/PreallocatedCircuitBreakerService.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ public CircuitBreakerStats stats(String name) {
5858
}
5959

6060
@Override
61-
protected void doClose() {
61+
public void close() {
6262
preallocated.close();
6363
}
6464

‎server/src/main/java/org/elasticsearch/indices/breaker/CircuitBreakerService.java‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,12 @@
1010
package org.elasticsearch.indices.breaker;
1111

1212
import org.elasticsearch.common.breaker.CircuitBreaker;
13-
import org.elasticsearch.common.component.AbstractLifecycleComponent;
1413

1514
/**
1615
* Interface for Circuit Breaker services, which provide breakers to classes
1716
* that load field data.
1817
*/
19-
public abstract class CircuitBreakerService extends AbstractLifecycleComponent {
18+
public abstract class CircuitBreakerService {
2019

2120
protected CircuitBreakerService() {}
2221

@@ -35,13 +34,4 @@ protected CircuitBreakerService() {}
3534
*/
3635
public abstract CircuitBreakerStats stats(String name);
3736

38-
@Override
39-
protected void doStart() {}
40-
41-
@Override
42-
protected void doStop() {}
43-
44-
@Override
45-
protected void doClose() {}
46-
4737
}

‎server/src/main/java/org/elasticsearch/node/NodeConstruction.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1459,7 +1459,6 @@ private CircuitBreakerService createCircuitBreakerService(
14591459
case "none" -> new NoneCircuitBreakerService();
14601460
default -> throw new IllegalArgumentException("Unknown circuit breaker type [" + type + "]");
14611461
};
1462-
resourcesToClose.add(circuitBreakerService);
14631462
modules.bindToInstance(CircuitBreakerService.class, circuitBreakerService);
14641463

14651464
pluginBreakers.forEach(t -> {

‎server/src/test/java/org/elasticsearch/common/breaker/PreallocatedCircuitBreakerServiceTests.java‎

Lines changed: 66 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -25,100 +25,94 @@
2525

2626
public class PreallocatedCircuitBreakerServiceTests extends ESTestCase {
2727
public void testUseNotPreallocated() {
28-
try (HierarchyCircuitBreakerService real = real()) {
29-
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, 1024)) {
30-
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
31-
b.addEstimateBytesAndMaybeBreak(100, "test");
32-
b.addWithoutBreaking(-100);
33-
}
34-
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
28+
HierarchyCircuitBreakerService real = real();
29+
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, 1024)) {
30+
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
31+
b.addEstimateBytesAndMaybeBreak(100, "test");
32+
b.addWithoutBreaking(-100);
3533
}
34+
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
3635
}
3736

3837
public void testUseLessThanPreallocated() {
39-
try (HierarchyCircuitBreakerService real = real()) {
40-
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, 1024)) {
41-
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
42-
b.addEstimateBytesAndMaybeBreak(100, "test");
43-
b.addWithoutBreaking(-100);
44-
}
45-
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
38+
HierarchyCircuitBreakerService real = real();
39+
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, 1024)) {
40+
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
41+
b.addEstimateBytesAndMaybeBreak(100, "test");
42+
b.addWithoutBreaking(-100);
4643
}
44+
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
4745
}
4846

4947
public void testCloseIsIdempotent() {
50-
try (HierarchyCircuitBreakerService real = real()) {
51-
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, 1024)) {
52-
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
53-
b.addEstimateBytesAndMaybeBreak(100, "test");
54-
b.addWithoutBreaking(-100);
55-
preallocated.close();
56-
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
57-
} // Closes again which should do nothing
48+
HierarchyCircuitBreakerService real = real();
49+
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, 1024)) {
50+
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
51+
b.addEstimateBytesAndMaybeBreak(100, "test");
52+
b.addWithoutBreaking(-100);
53+
preallocated.close();
5854
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
59-
}
55+
} // Closes again which should do nothing
56+
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
6057
}
6158

6259
public void testUseMoreThanPreallocated() {
63-
try (HierarchyCircuitBreakerService real = real()) {
64-
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, 1024)) {
65-
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
66-
b.addEstimateBytesAndMaybeBreak(2048, "test");
67-
b.addWithoutBreaking(-2048);
68-
}
69-
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
60+
HierarchyCircuitBreakerService real = real();
61+
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, 1024)) {
62+
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
63+
b.addEstimateBytesAndMaybeBreak(2048, "test");
64+
b.addWithoutBreaking(-2048);
7065
}
66+
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
7167
}
7268

7369
public void testPreallocateMoreThanRemains() {
74-
try (HierarchyCircuitBreakerService real = real()) {
75-
long limit = real.getBreaker(CircuitBreaker.REQUEST).getLimit();
76-
Exception e = expectThrows(CircuitBreakingException.class, () -> preallocateRequest(real, limit + 1024));
77-
assertThat(e.getMessage(), startsWith("[request] Data too large, data for [preallocate[test]] would be ["));
78-
}
70+
HierarchyCircuitBreakerService real = real();
71+
long limit = real.getBreaker(CircuitBreaker.REQUEST).getLimit();
72+
Exception e = expectThrows(CircuitBreakingException.class, () -> preallocateRequest(real, limit + 1024));
73+
assertThat(e.getMessage(), startsWith("[request] Data too large, data for [preallocate[test]] would be ["));
7974
}
8075

8176
public void testRandom() {
82-
try (HierarchyCircuitBreakerService real = real()) {
83-
CircuitBreaker realBreaker = real.getBreaker(CircuitBreaker.REQUEST);
84-
long preallocatedBytes = randomLongBetween(1, (long) (realBreaker.getLimit() * .8));
85-
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, preallocatedBytes)) {
86-
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
87-
boolean usedPreallocated = false;
88-
long current = 0;
89-
for (int i = 0; i < 10000; i++) {
90-
if (current >= preallocatedBytes) {
91-
usedPreallocated = true;
92-
}
93-
if (usedPreallocated) {
94-
assertThat(realBreaker.getUsed(), equalTo(current));
95-
} else {
96-
assertThat(realBreaker.getUsed(), equalTo(preallocatedBytes));
97-
}
98-
if (current > 0 && randomBoolean()) {
99-
long delta = randomLongBetween(-Math.min(current, realBreaker.getLimit() / 100), 0);
100-
b.addWithoutBreaking(delta);
101-
current += delta;
102-
continue;
103-
}
104-
long delta = randomLongBetween(0, realBreaker.getLimit() / 100);
105-
if (randomBoolean()) {
106-
b.addWithoutBreaking(delta);
107-
current += delta;
108-
continue;
109-
}
110-
if (current + delta < realBreaker.getLimit()) {
111-
b.addEstimateBytesAndMaybeBreak(delta, "test");
112-
current += delta;
113-
continue;
114-
}
115-
Exception e = expectThrows(CircuitBreakingException.class, () -> b.addEstimateBytesAndMaybeBreak(delta, "test"));
116-
assertThat(e.getMessage(), startsWith("[request] Data too large, data for [test] would be ["));
77+
HierarchyCircuitBreakerService real = real();
78+
CircuitBreaker realBreaker = real.getBreaker(CircuitBreaker.REQUEST);
79+
long preallocatedBytes = randomLongBetween(1, (long) (realBreaker.getLimit() * .8));
80+
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, preallocatedBytes)) {
81+
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
82+
boolean usedPreallocated = false;
83+
long current = 0;
84+
for (int i = 0; i < 10000; i++) {
85+
if (current >= preallocatedBytes) {
86+
usedPreallocated = true;
87+
}
88+
if (usedPreallocated) {
89+
assertThat(realBreaker.getUsed(), equalTo(current));
90+
} else {
91+
assertThat(realBreaker.getUsed(), equalTo(preallocatedBytes));
92+
}
93+
if (current > 0 && randomBoolean()) {
94+
long delta = randomLongBetween(-Math.min(current, realBreaker.getLimit() / 100), 0);
95+
b.addWithoutBreaking(delta);
96+
current += delta;
97+
continue;
11798
}
118-
b.addWithoutBreaking(-current);
99+
long delta = randomLongBetween(0, realBreaker.getLimit() / 100);
100+
if (randomBoolean()) {
101+
b.addWithoutBreaking(delta);
102+
current += delta;
103+
continue;
104+
}
105+
if (current + delta < realBreaker.getLimit()) {
106+
b.addEstimateBytesAndMaybeBreak(delta, "test");
107+
current += delta;
108+
continue;
109+
}
110+
Exception e = expectThrows(CircuitBreakingException.class, () -> b.addEstimateBytesAndMaybeBreak(delta, "test"));
111+
assertThat(e.getMessage(), startsWith("[request] Data too large, data for [test] would be ["));
119112
}
120-
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
113+
b.addWithoutBreaking(-current);
121114
}
115+
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
122116
}
123117

124118
private HierarchyCircuitBreakerService real() {

‎server/src/test/java/org/elasticsearch/common/util/BigArraysTests.java‎

Lines changed: 33 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -527,49 +527,46 @@ public void testOverSizeUsesMinPageCount() {
527527
*/
528528
public void testPreallocate() {
529529
ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS);
530+
HierarchyCircuitBreakerService realBreakers = new HierarchyCircuitBreakerService(
531+
CircuitBreakerMetrics.NOOP,
532+
Settings.EMPTY,
533+
List.of(),
534+
clusterSettings
535+
);
536+
BigArrays unPreAllocated = new MockBigArrays(new MockPageCacheRecycler(Settings.EMPTY), realBreakers);
537+
long toPreallocate = randomLongBetween(4000, 10000);
538+
CircuitBreaker realBreaker = realBreakers.getBreaker(CircuitBreaker.REQUEST);
539+
assertThat(realBreaker.getUsed(), equalTo(0L));
530540
try (
531-
HierarchyCircuitBreakerService realBreakers = new HierarchyCircuitBreakerService(
532-
CircuitBreakerMetrics.NOOP,
533-
Settings.EMPTY,
534-
List.of(),
535-
clusterSettings
541+
PreallocatedCircuitBreakerService prealloctedBreakerService = new PreallocatedCircuitBreakerService(
542+
realBreakers,
543+
CircuitBreaker.REQUEST,
544+
toPreallocate,
545+
"test"
536546
)
537547
) {
538-
BigArrays unPreAllocated = new MockBigArrays(new MockPageCacheRecycler(Settings.EMPTY), realBreakers);
539-
long toPreallocate = randomLongBetween(4000, 10000);
540-
CircuitBreaker realBreaker = realBreakers.getBreaker(CircuitBreaker.REQUEST);
541-
assertThat(realBreaker.getUsed(), equalTo(0L));
542-
try (
543-
PreallocatedCircuitBreakerService prealloctedBreakerService = new PreallocatedCircuitBreakerService(
544-
realBreakers,
545-
CircuitBreaker.REQUEST,
546-
toPreallocate,
547-
"test"
548-
)
549-
) {
550-
assertThat(realBreaker.getUsed(), equalTo(toPreallocate));
551-
BigArrays preallocated = unPreAllocated.withBreakerService(prealloctedBreakerService);
548+
assertThat(realBreaker.getUsed(), equalTo(toPreallocate));
549+
BigArrays preallocated = unPreAllocated.withBreakerService(prealloctedBreakerService);
552550

553-
// We don't grab any bytes just making a new BigArrays
554-
assertThat(realBreaker.getUsed(), equalTo(toPreallocate));
551+
// We don't grab any bytes just making a new BigArrays
552+
assertThat(realBreaker.getUsed(), equalTo(toPreallocate));
555553

556-
List<BigArray> arrays = new ArrayList<>();
557-
for (int i = 0; i < 30; i++) {
558-
// We're well under the preallocation so grabbing a little array doesn't allocate anything
559-
arrays.add(preallocated.newLongArray(1));
560-
assertThat(realBreaker.getUsed(), equalTo(toPreallocate));
561-
}
562-
563-
// Allocating a large array *does* allocate some bytes
564-
arrays.add(preallocated.newLongArray(1024));
565-
long expectedMin = (PageCacheRecycler.LONG_PAGE_SIZE + arrays.size()) * Long.BYTES;
566-
assertThat(realBreaker.getUsed(), greaterThanOrEqualTo(expectedMin));
567-
// 64 should be enough room for each BigArray object
568-
assertThat(realBreaker.getUsed(), lessThanOrEqualTo(expectedMin + 64 * arrays.size()));
569-
Releasables.close(arrays);
554+
List<BigArray> arrays = new ArrayList<>();
555+
for (int i = 0; i < 30; i++) {
556+
// We're well under the preallocation so grabbing a little array doesn't allocate anything
557+
arrays.add(preallocated.newLongArray(1));
558+
assertThat(realBreaker.getUsed(), equalTo(toPreallocate));
570559
}
571-
assertThat(realBreaker.getUsed(), equalTo(0L));
560+
561+
// Allocating a large array *does* allocate some bytes
562+
arrays.add(preallocated.newLongArray(1024));
563+
long expectedMin = (PageCacheRecycler.LONG_PAGE_SIZE + arrays.size()) * Long.BYTES;
564+
assertThat(realBreaker.getUsed(), greaterThanOrEqualTo(expectedMin));
565+
// 64 should be enough room for each BigArray object
566+
assertThat(realBreaker.getUsed(), lessThanOrEqualTo(expectedMin + 64 * arrays.size()));
567+
Releasables.close(arrays);
572568
}
569+
assertThat(realBreaker.getUsed(), equalTo(0L));
573570
}
574571

575572
private List<BigArraysHelper> bigArrayCreators(final long maxSize, final boolean withBreaking) {

0 commit comments

Comments
 (0)