Skip to content

Commit

Permalink
Remove lifecycle from CircuitBreakerService (elastic#118699)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@original-brownbear
original-brownbear authored Dec 14, 2024
1 parent fee6165 commit d695020
Show file tree
Hide file tree
Showing 11 changed files with 340 additions and 393 deletions.
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/common/breaker/PreallocatedCircuitBreakerService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public CircuitBreakerStats stats(String name) {
}

@Override
protected void doClose() {
public void close() {
preallocated.close();
}

Expand Down
12 changes: 1 addition & 11 deletions server/src/main/java/org/elasticsearch/indices/breaker/CircuitBreakerService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,12 @@
package org.elasticsearch.indices.breaker;

import org.elasticsearch.common.breaker.CircuitBreaker;
import org.elasticsearch.common.component.AbstractLifecycleComponent;

/**
* Interface for Circuit Breaker services, which provide breakers to classes
* that load field data.
*/
public abstract class CircuitBreakerService extends AbstractLifecycleComponent {
public abstract class CircuitBreakerService {

protected CircuitBreakerService() {}

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

@Override
protected void doStart() {}

@Override
protected void doStop() {}

@Override
protected void doClose() {}

}
1 change: 0 additions & 1 deletion server/src/main/java/org/elasticsearch/node/NodeConstruction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1459,7 +1459,6 @@ private CircuitBreakerService createCircuitBreakerService(
case "none" -> new NoneCircuitBreakerService();
default -> throw new IllegalArgumentException("Unknown circuit breaker type [" + type + "]");
};
resourcesToClose.add(circuitBreakerService);
modules.bindToInstance(CircuitBreakerService.class, circuitBreakerService);

pluginBreakers.forEach(t -> {
Expand Down
138 changes: 66 additions & 72 deletions ...rc/test/java/org/elasticsearch/common/breaker/PreallocatedCircuitBreakerServiceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,100 +25,94 @@

public class PreallocatedCircuitBreakerServiceTests extends ESTestCase {
public void testUseNotPreallocated() {
try (HierarchyCircuitBreakerService real = real()) {
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, 1024)) {
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
b.addEstimateBytesAndMaybeBreak(100, "test");
b.addWithoutBreaking(-100);
}
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
HierarchyCircuitBreakerService real = real();
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, 1024)) {
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
b.addEstimateBytesAndMaybeBreak(100, "test");
b.addWithoutBreaking(-100);
}
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
}

public void testUseLessThanPreallocated() {
try (HierarchyCircuitBreakerService real = real()) {
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, 1024)) {
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
b.addEstimateBytesAndMaybeBreak(100, "test");
b.addWithoutBreaking(-100);
}
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
HierarchyCircuitBreakerService real = real();
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, 1024)) {
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
b.addEstimateBytesAndMaybeBreak(100, "test");
b.addWithoutBreaking(-100);
}
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
}

public void testCloseIsIdempotent() {
try (HierarchyCircuitBreakerService real = real()) {
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, 1024)) {
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
b.addEstimateBytesAndMaybeBreak(100, "test");
b.addWithoutBreaking(-100);
preallocated.close();
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
} // Closes again which should do nothing
HierarchyCircuitBreakerService real = real();
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, 1024)) {
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
b.addEstimateBytesAndMaybeBreak(100, "test");
b.addWithoutBreaking(-100);
preallocated.close();
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
}
} // Closes again which should do nothing
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
}

public void testUseMoreThanPreallocated() {
try (HierarchyCircuitBreakerService real = real()) {
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, 1024)) {
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
b.addEstimateBytesAndMaybeBreak(2048, "test");
b.addWithoutBreaking(-2048);
}
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
HierarchyCircuitBreakerService real = real();
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, 1024)) {
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
b.addEstimateBytesAndMaybeBreak(2048, "test");
b.addWithoutBreaking(-2048);
}
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
}

public void testPreallocateMoreThanRemains() {
try (HierarchyCircuitBreakerService real = real()) {
long limit = real.getBreaker(CircuitBreaker.REQUEST).getLimit();
Exception e = expectThrows(CircuitBreakingException.class, () -> preallocateRequest(real, limit + 1024));
assertThat(e.getMessage(), startsWith("[request] Data too large, data for [preallocate[test]] would be ["));
}
HierarchyCircuitBreakerService real = real();
long limit = real.getBreaker(CircuitBreaker.REQUEST).getLimit();
Exception e = expectThrows(CircuitBreakingException.class, () -> preallocateRequest(real, limit + 1024));
assertThat(e.getMessage(), startsWith("[request] Data too large, data for [preallocate[test]] would be ["));
}

public void testRandom() {
try (HierarchyCircuitBreakerService real = real()) {
CircuitBreaker realBreaker = real.getBreaker(CircuitBreaker.REQUEST);
long preallocatedBytes = randomLongBetween(1, (long) (realBreaker.getLimit() * .8));
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, preallocatedBytes)) {
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
boolean usedPreallocated = false;
long current = 0;
for (int i = 0; i < 10000; i++) {
if (current >= preallocatedBytes) {
usedPreallocated = true;
}
if (usedPreallocated) {
assertThat(realBreaker.getUsed(), equalTo(current));
} else {
assertThat(realBreaker.getUsed(), equalTo(preallocatedBytes));
}
if (current > 0 && randomBoolean()) {
long delta = randomLongBetween(-Math.min(current, realBreaker.getLimit() / 100), 0);
b.addWithoutBreaking(delta);
current += delta;
continue;
}
long delta = randomLongBetween(0, realBreaker.getLimit() / 100);
if (randomBoolean()) {
b.addWithoutBreaking(delta);
current += delta;
continue;
}
if (current + delta < realBreaker.getLimit()) {
b.addEstimateBytesAndMaybeBreak(delta, "test");
current += delta;
continue;
}
Exception e = expectThrows(CircuitBreakingException.class, () -> b.addEstimateBytesAndMaybeBreak(delta, "test"));
assertThat(e.getMessage(), startsWith("[request] Data too large, data for [test] would be ["));
HierarchyCircuitBreakerService real = real();
CircuitBreaker realBreaker = real.getBreaker(CircuitBreaker.REQUEST);
long preallocatedBytes = randomLongBetween(1, (long) (realBreaker.getLimit() * .8));
try (PreallocatedCircuitBreakerService preallocated = preallocateRequest(real, preallocatedBytes)) {
CircuitBreaker b = preallocated.getBreaker(CircuitBreaker.REQUEST);
boolean usedPreallocated = false;
long current = 0;
for (int i = 0; i < 10000; i++) {
if (current >= preallocatedBytes) {
usedPreallocated = true;
}
if (usedPreallocated) {
assertThat(realBreaker.getUsed(), equalTo(current));
} else {
assertThat(realBreaker.getUsed(), equalTo(preallocatedBytes));
}
if (current > 0 && randomBoolean()) {
long delta = randomLongBetween(-Math.min(current, realBreaker.getLimit() / 100), 0);
b.addWithoutBreaking(delta);
current += delta;
continue;
}
b.addWithoutBreaking(-current);
long delta = randomLongBetween(0, realBreaker.getLimit() / 100);
if (randomBoolean()) {
b.addWithoutBreaking(delta);
current += delta;
continue;
}
if (current + delta < realBreaker.getLimit()) {
b.addEstimateBytesAndMaybeBreak(delta, "test");
current += delta;
continue;
}
Exception e = expectThrows(CircuitBreakingException.class, () -> b.addEstimateBytesAndMaybeBreak(delta, "test"));
assertThat(e.getMessage(), startsWith("[request] Data too large, data for [test] would be ["));
}
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
b.addWithoutBreaking(-current);
}
assertThat(real.getBreaker(CircuitBreaker.REQUEST).getUsed(), equalTo(0L));
}

private HierarchyCircuitBreakerService real() {
Expand Down
69 changes: 33 additions & 36 deletions server/src/test/java/org/elasticsearch/common/util/BigArraysTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -527,49 +527,46 @@ public void testOverSizeUsesMinPageCount() {
*/
public void testPreallocate() {
ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS);
HierarchyCircuitBreakerService realBreakers = new HierarchyCircuitBreakerService(
CircuitBreakerMetrics.NOOP,
Settings.EMPTY,
List.of(),
clusterSettings
);
BigArrays unPreAllocated = new MockBigArrays(new MockPageCacheRecycler(Settings.EMPTY), realBreakers);
long toPreallocate = randomLongBetween(4000, 10000);
CircuitBreaker realBreaker = realBreakers.getBreaker(CircuitBreaker.REQUEST);
assertThat(realBreaker.getUsed(), equalTo(0L));
try (
HierarchyCircuitBreakerService realBreakers = new HierarchyCircuitBreakerService(
CircuitBreakerMetrics.NOOP,
Settings.EMPTY,
List.of(),
clusterSettings
PreallocatedCircuitBreakerService prealloctedBreakerService = new PreallocatedCircuitBreakerService(
realBreakers,
CircuitBreaker.REQUEST,
toPreallocate,
"test"
)
) {
BigArrays unPreAllocated = new MockBigArrays(new MockPageCacheRecycler(Settings.EMPTY), realBreakers);
long toPreallocate = randomLongBetween(4000, 10000);
CircuitBreaker realBreaker = realBreakers.getBreaker(CircuitBreaker.REQUEST);
assertThat(realBreaker.getUsed(), equalTo(0L));
try (
PreallocatedCircuitBreakerService prealloctedBreakerService = new PreallocatedCircuitBreakerService(
realBreakers,
CircuitBreaker.REQUEST,
toPreallocate,
"test"
)
) {
assertThat(realBreaker.getUsed(), equalTo(toPreallocate));
BigArrays preallocated = unPreAllocated.withBreakerService(prealloctedBreakerService);
assertThat(realBreaker.getUsed(), equalTo(toPreallocate));
BigArrays preallocated = unPreAllocated.withBreakerService(prealloctedBreakerService);

// We don't grab any bytes just making a new BigArrays
assertThat(realBreaker.getUsed(), equalTo(toPreallocate));
// We don't grab any bytes just making a new BigArrays
assertThat(realBreaker.getUsed(), equalTo(toPreallocate));

List<BigArray> arrays = new ArrayList<>();
for (int i = 0; i < 30; i++) {
// We're well under the preallocation so grabbing a little array doesn't allocate anything
arrays.add(preallocated.newLongArray(1));
assertThat(realBreaker.getUsed(), equalTo(toPreallocate));
}

// Allocating a large array *does* allocate some bytes
arrays.add(preallocated.newLongArray(1024));
long expectedMin = (PageCacheRecycler.LONG_PAGE_SIZE + arrays.size()) * Long.BYTES;
assertThat(realBreaker.getUsed(), greaterThanOrEqualTo(expectedMin));
// 64 should be enough room for each BigArray object
assertThat(realBreaker.getUsed(), lessThanOrEqualTo(expectedMin + 64 * arrays.size()));
Releasables.close(arrays);
List<BigArray> arrays = new ArrayList<>();
for (int i = 0; i < 30; i++) {
// We're well under the preallocation so grabbing a little array doesn't allocate anything
arrays.add(preallocated.newLongArray(1));
assertThat(realBreaker.getUsed(), equalTo(toPreallocate));
}
assertThat(realBreaker.getUsed(), equalTo(0L));

// Allocating a large array *does* allocate some bytes
arrays.add(preallocated.newLongArray(1024));
long expectedMin = (PageCacheRecycler.LONG_PAGE_SIZE + arrays.size()) * Long.BYTES;
assertThat(realBreaker.getUsed(), greaterThanOrEqualTo(expectedMin));
// 64 should be enough room for each BigArray object
assertThat(realBreaker.getUsed(), lessThanOrEqualTo(expectedMin + 64 * arrays.size()));
Releasables.close(arrays);
}
assertThat(realBreaker.getUsed(), equalTo(0L));
}

private List<BigArraysHelper> bigArrayCreators(final long maxSize, final boolean withBreaking) {
Expand Down
Loading

0 comments on commit d695020

Please sign in to comment.