Skip to content

Commit cabf886

Browse files
nik9000nik9000
committed
Remove return from breakers (elastic#66943)
This removes the return values from `CircuitBreaker`'s `addEstimateBytesAndMaybeBreak` and `addWithoutBreaking` because they are not supported by the new pre-allocating `CircuitBreaker` and they aren't used for anything other than tests.
1 parent e6012ed commit cabf886

File tree

12 files changed

+40
-61
lines changed

12 files changed

+40
-61
lines changed

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

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -82,10 +82,9 @@ public void circuitBreak(String fieldName, long bytesNeeded) {
8282
* memory limit is set to 0. Will never trip the breaker if the limit is
8383
* set < 0, but can still be used to aggregate estimations.
8484
* @param bytes number of bytes to add to the breaker
85-
* @return number of "used" bytes so far
8685
*/
8786
@Override
88-
public double addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
87+
public void addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
8988
final LimitAndOverhead limitAndOverhead = this.limitAndOverhead;
9089
final long memoryBytesLimit = limitAndOverhead.limit;
9190
final double overheadConstant = limitAndOverhead.overhead;
@@ -115,7 +114,6 @@ public double addEstimateBytesAndMaybeBreak(long bytes, String label) throws Cir
115114
throw e;
116115
}
117116
assert newUsed >= 0 : "Used bytes: [" + newUsed + "] must be >= 0";
118-
return newUsed;
119117
}
120118

121119
private long noLimit(long bytes, String label) {
@@ -163,14 +161,12 @@ memoryBytesLimit, new ByteSizeValue(memoryBytesLimit),
163161
* has been exceeded.
164162
*
165163
* @param bytes number of bytes to add to the breaker
166-
* @return number of "used" bytes so far
167164
*/
168165
@Override
169-
public long addWithoutBreaking(long bytes) {
166+
public void addWithoutBreaking(long bytes) {
170167
long u = used.addAndGet(bytes);
171168
logger.trace(() -> new ParameterizedMessage("[{}] Adjusted breaker by [{}] bytes, now [{}]", this.name, bytes, u));
172169
assert u >= 0 : "Used bytes: [" + u + "] must be >= 0";
173-
return u;
174170
}
175171

176172
/**

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -98,17 +98,17 @@ enum Durability {
9898
void circuitBreak(String fieldName, long bytesNeeded);
9999

100100
/**
101-
* add bytes to the breaker and maybe trip
101+
* Add bytes to the breaker and trip if the that puts breaker over the limit.
102102
* @param bytes number of bytes to add
103-
* @param label string label describing the bytes being added
104-
* @return the number of "used" bytes for the circuit breaker
103+
* @param label thing requesting the bytes being added that is included in
104+
* the exception if the breaker is tripped
105105
*/
106-
double addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException;
106+
void addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException;
107107

108108
/**
109-
* Adjust the circuit breaker without tripping
109+
* Add bytes to the circuit breaker without tripping.
110110
*/
111-
long addWithoutBreaking(long bytes);
111+
void addWithoutBreaking(long bytes);
112112

113113
/**
114114
* @return the currently used bytes the breaker is tracking

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,13 +38,11 @@ public void circuitBreak(String fieldName, long bytesNeeded) {
3838
}
3939

4040
@Override
41-
public double addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
42-
return 0;
41+
public void addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
4342
}
4443

4544
@Override
46-
public long addWithoutBreaking(long bytes) {
47-
return 0;
45+
public void addWithoutBreaking(long bytes) {
4846
}
4947

5048
@Override

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

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -115,45 +115,45 @@ public void circuitBreak(String fieldName, long bytesNeeded) {
115115
}
116116

117117
@Override
118-
public double addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
118+
public void addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
119119
if (closed) {
120120
throw new IllegalStateException("already closed");
121121
}
122122
if (preallocationUsed == preallocated) {
123123
// Preallocation buffer was full before this request
124-
return next.addEstimateBytesAndMaybeBreak(bytes, label);
124+
next.addEstimateBytesAndMaybeBreak(bytes, label);
125+
return;
125126
}
126127
long newUsed = preallocationUsed + bytes;
127128
if (newUsed > preallocated) {
128129
// This request filled up the buffer
129130
preallocationUsed = preallocated;
130-
return next.addEstimateBytesAndMaybeBreak(newUsed - preallocated, label);
131+
next.addEstimateBytesAndMaybeBreak(newUsed - preallocated, label);
132+
return;
131133
}
132134
// This is the fast case. No volatile reads or writes here, ma!
133135
preallocationUsed = newUsed;
134-
// We return garbage here but callers never use the result for anything interesting
135-
return 0;
136136
}
137137

138138
@Override
139-
public long addWithoutBreaking(long bytes) {
139+
public void addWithoutBreaking(long bytes) {
140140
if (closed) {
141141
throw new IllegalStateException("already closed");
142142
}
143143
if (preallocationUsed == preallocated) {
144144
// Preallocation buffer was full before this request
145-
return next.addWithoutBreaking(bytes);
145+
next.addWithoutBreaking(bytes);
146+
return;
146147
}
147148
long newUsed = preallocationUsed + bytes;
148149
if (newUsed > preallocated) {
149150
// This request filled up the buffer
150151
preallocationUsed = preallocated;
151-
return next.addWithoutBreaking(newUsed - preallocated);
152+
next.addWithoutBreaking(newUsed - preallocated);
153+
return;
152154
}
153155
// This is the fast case. No volatile reads or writes here, ma!
154156
preallocationUsed = newUsed;
155-
// We return garbage here but callers never use the result for anything interesting
156-
return 0;
157157
}
158158

159159
@Override

‎server/src/test/java/org/elasticsearch/action/search/SearchPhaseControllerTests.java

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1045,19 +1045,17 @@ private static class AssertingCircuitBreaker extends NoopCircuitBreaker {
10451045
}
10461046

10471047
@Override
1048-
public double addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
1048+
public void addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
10491049
assert bytes >= 0;
10501050
if (shouldBreak.get()) {
10511051
throw new CircuitBreakingException(label, getDurability());
10521052
}
10531053
allocated += bytes;
1054-
return allocated;
10551054
}
10561055

10571056
@Override
1058-
public long addWithoutBreaking(long bytes) {
1057+
public void addWithoutBreaking(long bytes) {
10591058
allocated += bytes;
1060-
return allocated;
10611059
}
10621060
}
10631061
}

‎server/src/test/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerServiceTests.java

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -212,15 +212,12 @@ public void testBorrowingSiblingBreakerMemory() {
212212
assertEquals(new ByteSizeValue(150, ByteSizeUnit.MB).getBytes(), requestCircuitBreaker.getLimit());
213213
assertEquals(new ByteSizeValue(150, ByteSizeUnit.MB).getBytes(), fieldDataCircuitBreaker.getLimit());
214214

215-
double fieldDataUsedBytes = fieldDataCircuitBreaker
216-
.addEstimateBytesAndMaybeBreak(new ByteSizeValue(50, ByteSizeUnit.MB).getBytes(), "should not break");
217-
assertEquals(new ByteSizeValue(50, ByteSizeUnit.MB).getBytes(), fieldDataUsedBytes, 0.0);
218-
double requestUsedBytes = requestCircuitBreaker.addEstimateBytesAndMaybeBreak(new ByteSizeValue(50, ByteSizeUnit.MB).getBytes(),
219-
"should not break");
220-
assertEquals(new ByteSizeValue(50, ByteSizeUnit.MB).getBytes(), requestUsedBytes, 0.0);
221-
requestUsedBytes = requestCircuitBreaker.addEstimateBytesAndMaybeBreak(new ByteSizeValue(50, ByteSizeUnit.MB).getBytes(),
222-
"should not break");
223-
assertEquals(new ByteSizeValue(100, ByteSizeUnit.MB).getBytes(), requestUsedBytes, 0.0);
215+
fieldDataCircuitBreaker.addEstimateBytesAndMaybeBreak(new ByteSizeValue(50, ByteSizeUnit.MB).getBytes(), "should not break");
216+
assertEquals(new ByteSizeValue(50, ByteSizeUnit.MB).getBytes(), fieldDataCircuitBreaker.getUsed(), 0.0);
217+
requestCircuitBreaker.addEstimateBytesAndMaybeBreak(new ByteSizeValue(50, ByteSizeUnit.MB).getBytes(), "should not break");
218+
assertEquals(new ByteSizeValue(50, ByteSizeUnit.MB).getBytes(), requestCircuitBreaker.getUsed(), 0.0);
219+
requestCircuitBreaker.addEstimateBytesAndMaybeBreak(new ByteSizeValue(50, ByteSizeUnit.MB).getBytes(), "should not break");
220+
assertEquals(new ByteSizeValue(100, ByteSizeUnit.MB).getBytes(), requestCircuitBreaker.getUsed(), 0.0);
224221
CircuitBreakingException exception = expectThrows(CircuitBreakingException.class, () -> requestCircuitBreaker
225222
.addEstimateBytesAndMaybeBreak(new ByteSizeValue(50, ByteSizeUnit.MB).getBytes(), "should break"));
226223
assertThat(exception.getMessage(), containsString("[parent] Data too large, data for [should break] would be"));

‎server/src/test/java/org/elasticsearch/search/aggregations/metrics/HyperLogLogPlusPlusSparseTests.java

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -94,18 +94,16 @@ public void testCircuitBreakerOnConstruction() {
9494
when(breakerService.getBreaker(CircuitBreaker.REQUEST)).thenReturn(new NoopCircuitBreaker(CircuitBreaker.REQUEST) {
9595
private int countDown = whenToBreak;
9696
@Override
97-
public double addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
97+
public void addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
9898
if (countDown-- == 0) {
9999
throw new CircuitBreakingException("test error", bytes, Long.MAX_VALUE, Durability.TRANSIENT);
100100
}
101101
total.addAndGet(bytes);
102-
return total.get();
103102
}
104103

105104
@Override
106-
public long addWithoutBreaking(long bytes) {
105+
public void addWithoutBreaking(long bytes) {
107106
total.addAndGet(bytes);
108-
return total.get();
109107
}
110108
});
111109
BigArrays bigArrays = new BigArrays(null, breakerService, CircuitBreaker.REQUEST).withCircuitBreaking();

‎server/src/test/java/org/elasticsearch/search/aggregations/metrics/HyperLogLogPlusPlusTests.java

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -147,18 +147,16 @@ public void testCircuitBreakerOnConstruction() {
147147
when(breakerService.getBreaker(CircuitBreaker.REQUEST)).thenReturn(new NoopCircuitBreaker(CircuitBreaker.REQUEST) {
148148
private int countDown = whenToBreak;
149149
@Override
150-
public double addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
150+
public void addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
151151
if (countDown-- == 0) {
152152
throw new CircuitBreakingException("test error", bytes, Long.MAX_VALUE, Durability.TRANSIENT);
153153
}
154154
total.addAndGet(bytes);
155-
return total.get();
156155
}
157156

158157
@Override
159-
public long addWithoutBreaking(long bytes) {
158+
public void addWithoutBreaking(long bytes) {
160159
total.addAndGet(bytes);
161-
return total.get();
162160
}
163161
});
164162
BigArrays bigArrays = new BigArrays(null, breakerService, CircuitBreaker.REQUEST).withCircuitBreaking();

‎server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregatorTests.java

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -210,17 +210,15 @@ public void mockBreaker() {
210210
private long total = 0;
211211

212212
@Override
213-
public double addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
213+
public void addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
214214
logger.debug("Used {} grabbing {} for {}", total, bytes, label);
215215
total += bytes;
216-
return total;
217216
}
218217

219218
@Override
220-
public long addWithoutBreaking(long bytes) {
219+
public void addWithoutBreaking(long bytes) {
221220
logger.debug("Used {} grabbing {}", total, bytes);
222221
total += bytes;
223-
return total;
224222
}
225223

226224
@Override

‎test/framework/src/main/java/org/elasticsearch/common/breaker/TestCircuitBreaker.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,10 @@ public TestCircuitBreaker() {
3030
}
3131

3232
@Override
33-
public double addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
33+
public void addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
3434
if (shouldBreak.get()) {
3535
throw new CircuitBreakingException("broken", getDurability());
3636
}
37-
return 0;
3837
}
3938

4039
public void startBreaking() {

‎test/framework/src/main/java/org/elasticsearch/common/util/MockBigArrays.java

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -638,17 +638,16 @@ public LimitedBreaker(String name, ByteSizeValue max) {
638638
}
639639

640640
@Override
641-
public double addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
641+
public void addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
642642
long total = used.addAndGet(bytes);
643643
if (total > max.getBytes()) {
644644
throw new CircuitBreakingException("test error", bytes, max.getBytes(), Durability.TRANSIENT);
645645
}
646-
return total;
647646
}
648647

649648
@Override
650-
public long addWithoutBreaking(long bytes) {
651-
return used.addAndGet(bytes);
649+
public void addWithoutBreaking(long bytes) {
650+
used.addAndGet(bytes);
652651
}
653652
}
654653
}

‎x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/inference/loadingservice/ModelLoadingServiceTests.java

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -654,22 +654,20 @@ public void circuitBreak(String fieldName, long bytesNeeded) {
654654
}
655655

656656
@Override
657-
public double addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
657+
public void addEstimateBytesAndMaybeBreak(long bytes, String label) throws CircuitBreakingException {
658658
synchronized (this) {
659659
if (bytes + currentBytes >= maxBytes) {
660660
trippedCount++;
661661
circuitBreak(label, bytes);
662662
}
663663
currentBytes += bytes;
664-
return currentBytes;
665664
}
666665
}
667666

668667
@Override
669-
public long addWithoutBreaking(long bytes) {
668+
public void addWithoutBreaking(long bytes) {
670669
synchronized (this) {
671670
currentBytes += bytes;
672-
return currentBytes;
673671
}
674672
}
675673

0 commit comments

Comments
 (0)