Skip to content

Commit 31f07cc

Browse files
authored
[6.3.0] Implement failure circuit breaker (#18541)
* feat: Implement failure circuit breaker Copy of #18120: I accidentally closed #18120 during rebase and doesn't have permission to reopen. We have noticed that any problems with the remote cache have a detrimental effect on build times. On investigation we found that the interface for the circuit breaker was left unimplemented. To address this issue, implemented a failure circuit breaker, which includes three new Bazel flags: 1) experimental_circuitbreaker_strategy, 2) experimental_remote_failure_threshold, and 3) experimental_emote_failure_window. In this implementation, I have implemented failure strategy for circuit breaker and used failure count to trip the circuit. Reasoning behind using failure count instead of failure rate : To measure failure rate I also need the success count. While both the failure and success count need to be an AtomicInteger as both will be modified concurrently by multiple threads. Even though getAndIncrement is very light weight operation, at very high request it might contribute to latency. Reasoning behind using failure circuit breaker : A new instance of Retrier.CircuitBreaker is created for each build. Therefore, if the circuit breaker trips during a build, the remote cache will be disabled for that build. However, it will be enabled again for the next build as a new instance of Retrier.CircuitBreaker will be created. If needed in the future we may add cool down strategy also. e.g. failure_and_cool_down_startegy. closes #18136 Closes #18359. PiperOrigin-RevId: 536349954 Change-Id: I5e1c57d4ad0ce07ddc4808bf1f327bc5df6ce704 * remove target included in cherry-pick by mistake
1 parent 28ebcdc commit 31f07cc

File tree

17 files changed

+578
-176
lines changed

17 files changed

+578
-176
lines changed

src/main/java/com/google/devtools/build/lib/remote/BUILD

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ package(
77
filegroup(
88
name = "srcs",
99
srcs = glob(["*"]) + [
10+
"//src/main/java/com/google/devtools/build/lib/remote/circuitbreaker:srcs",
1011
"//src/main/java/com/google/devtools/build/lib/remote/common:srcs",
1112
"//src/main/java/com/google/devtools/build/lib/remote/downloader:srcs",
1213
"//src/main/java/com/google/devtools/build/lib/remote/disk:srcs",
@@ -84,6 +85,7 @@ java_library(
8485
"//src/main/java/com/google/devtools/build/lib/exec/local",
8586
"//src/main/java/com/google/devtools/build/lib/packages/semantics",
8687
"//src/main/java/com/google/devtools/build/lib/profiler",
88+
"//src/main/java/com/google/devtools/build/lib/remote/circuitbreaker",
8789
"//src/main/java/com/google/devtools/build/lib/remote/common",
8890
"//src/main/java/com/google/devtools/build/lib/remote/common:cache_not_found_exception",
8991
"//src/main/java/com/google/devtools/build/lib/remote/disk",

src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -495,4 +495,8 @@ ListenableFuture<Void> uploadChunker(
495495
MoreExecutors.directExecutor());
496496
return f;
497497
}
498+
499+
Retrier getRetrier() {
500+
return this.retrier;
501+
}
498502
}

src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutor.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,4 +243,8 @@ public void close() {
243243
}
244244
channel.release();
245245
}
246+
247+
RemoteRetrier getRetrier() {
248+
return this.retrier;
249+
}
246250
}

src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@
6262
import com.google.devtools.build.lib.exec.SpawnStrategyRegistry;
6363
import com.google.devtools.build.lib.remote.RemoteServerCapabilities.ServerCapabilitiesRequirement;
6464
import com.google.devtools.build.lib.remote.ToplevelArtifactsDownloader.PathToMetadataConverter;
65+
import com.google.devtools.build.lib.remote.circuitbreaker.CircuitBreakerFactory;
6566
import com.google.devtools.build.lib.remote.common.RemoteCacheClient;
6667
import com.google.devtools.build.lib.remote.common.RemoteExecutionClient;
6768
import com.google.devtools.build.lib.remote.downloader.GrpcRemoteDownloader;
@@ -475,12 +476,11 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
475476
GoogleAuthUtils.newCallCredentialsProvider(credentials);
476477
CallCredentials callCredentials = callCredentialsProvider.getCallCredentials();
477478

479+
Retrier.CircuitBreaker circuitBreaker =
480+
CircuitBreakerFactory.createCircuitBreaker(remoteOptions);
478481
RemoteRetrier retrier =
479482
new RemoteRetrier(
480-
remoteOptions,
481-
RemoteRetrier.RETRIABLE_GRPC_ERRORS,
482-
retryScheduler,
483-
Retrier.ALLOW_ALL_CALLS);
483+
remoteOptions, RemoteRetrier.RETRIABLE_GRPC_ERRORS, retryScheduler, circuitBreaker);
484484

485485
// We only check required capabilities for a given endpoint.
486486
//
@@ -598,7 +598,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
598598
remoteOptions,
599599
RemoteRetrier.RETRIABLE_GRPC_ERRORS, // Handle NOT_FOUND internally
600600
retryScheduler,
601-
Retrier.ALLOW_ALL_CALLS);
601+
circuitBreaker);
602602
remoteExecutor =
603603
new ExperimentalGrpcRemoteExecutor(
604604
remoteOptions, execChannel.retain(), callCredentialsProvider, execRetrier);
@@ -608,7 +608,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
608608
remoteOptions,
609609
RemoteRetrier.RETRIABLE_GRPC_EXEC_ERRORS,
610610
retryScheduler,
611-
Retrier.ALLOW_ALL_CALLS);
611+
circuitBreaker);
612612
remoteExecutor =
613613
new GrpcRemoteExecutor(execChannel.retain(), callCredentialsProvider, execRetrier);
614614
}

src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
import com.google.devtools.build.lib.profiler.SilentCloseable;
5959
import com.google.devtools.build.lib.remote.RemoteExecutionService.RemoteActionResult;
6060
import com.google.devtools.build.lib.remote.RemoteExecutionService.ServerLogs;
61+
import com.google.devtools.build.lib.remote.circuitbreaker.CircuitBreakerFactory;
6162
import com.google.devtools.build.lib.remote.common.BulkTransferException;
6263
import com.google.devtools.build.lib.remote.common.OperationObserver;
6364
import com.google.devtools.build.lib.remote.options.RemoteOptions;
@@ -660,6 +661,8 @@ private void report(Event evt) {
660661
private static RemoteRetrier createExecuteRetrier(
661662
RemoteOptions options, ListeningScheduledExecutorService retryService) {
662663
return new ExecuteRetrier(
663-
options.remoteMaxRetryAttempts, retryService, Retrier.ALLOW_ALL_CALLS);
664+
options.remoteMaxRetryAttempts,
665+
retryService,
666+
CircuitBreakerFactory.createCircuitBreaker(options));
664667
}
665668
}

src/main/java/com/google/devtools/build/lib/remote/Retrier.java

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ enum State {
100100
State state();
101101

102102
/** Called after an execution failed. */
103-
void recordFailure();
103+
void recordFailure(Exception e);
104104

105105
/** Called after an execution succeeded. */
106106
void recordSuccess();
@@ -130,7 +130,7 @@ public State state() {
130130
}
131131

132132
@Override
133-
public void recordFailure() {}
133+
public void recordFailure(Exception e) {}
134134

135135
@Override
136136
public void recordSuccess() {}
@@ -245,7 +245,7 @@ public <T> T execute(Callable<T> call, Backoff backoff) throws Exception {
245245
circuitBreaker.recordSuccess();
246246
return r;
247247
} catch (Exception e) {
248-
circuitBreaker.recordFailure();
248+
circuitBreaker.recordFailure(e);
249249
Throwables.throwIfInstanceOf(e, InterruptedException.class);
250250
if (State.TRIAL_CALL.equals(circuitState)) {
251251
throw e;
@@ -272,19 +272,35 @@ public <T> ListenableFuture<T> executeAsync(AsyncCallable<T> call) {
272272
* backoff.
273273
*/
274274
public <T> ListenableFuture<T> executeAsync(AsyncCallable<T> call, Backoff backoff) {
275+
final State circuitState = circuitBreaker.state();
276+
if (State.REJECT_CALLS.equals(circuitState)) {
277+
return Futures.immediateFailedFuture(new CircuitBreakerException());
278+
}
275279
try {
280+
ListenableFuture<T> future =
281+
Futures.transformAsync(
282+
call.call(),
283+
(f) -> {
284+
circuitBreaker.recordSuccess();
285+
return Futures.immediateFuture(f);
286+
},
287+
MoreExecutors.directExecutor());
276288
return Futures.catchingAsync(
277-
call.call(),
289+
future,
278290
Exception.class,
279-
t -> onExecuteAsyncFailure(t, call, backoff),
291+
t -> onExecuteAsyncFailure(t, call, backoff, circuitState),
280292
MoreExecutors.directExecutor());
281293
} catch (Exception e) {
282-
return onExecuteAsyncFailure(e, call, backoff);
294+
return onExecuteAsyncFailure(e, call, backoff, circuitState);
283295
}
284296
}
285297

286298
private <T> ListenableFuture<T> onExecuteAsyncFailure(
287-
Exception t, AsyncCallable<T> call, Backoff backoff) {
299+
Exception t, AsyncCallable<T> call, Backoff backoff, State circuitState) {
300+
circuitBreaker.recordFailure(t);
301+
if (circuitState.equals(State.TRIAL_CALL)) {
302+
return Futures.immediateFailedFuture(t);
303+
}
288304
if (isRetriable(t)) {
289305
long waitMillis = backoff.nextDelayMillis(t);
290306
if (waitMillis >= 0) {
@@ -310,4 +326,8 @@ public Backoff newBackoff() {
310326
public boolean isRetriable(Exception e) {
311327
return shouldRetry.test(e);
312328
}
329+
330+
CircuitBreaker getCircuitBreaker() {
331+
return this.circuitBreaker;
332+
}
313333
}
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
load("@rules_java//java:defs.bzl", "java_library")
2+
3+
package(
4+
default_applicable_licenses = ["//:license"],
5+
default_visibility = ["//src:__subpackages__"],
6+
)
7+
8+
filegroup(
9+
name = "srcs",
10+
srcs = glob(["*"]),
11+
visibility = ["//src:__subpackages__"],
12+
)
13+
14+
java_library(
15+
name = "circuitbreaker",
16+
srcs = glob(["*.java"]),
17+
deps = [
18+
"//src/main/java/com/google/devtools/build/lib/remote:Retrier",
19+
"//src/main/java/com/google/devtools/build/lib/remote/common:cache_not_found_exception",
20+
"//src/main/java/com/google/devtools/build/lib/remote/options",
21+
"//third_party:guava",
22+
],
23+
)
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
// Copyright 2023 The Bazel Authors. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
package com.google.devtools.build.lib.remote.circuitbreaker;
15+
16+
import com.google.common.collect.ImmutableSet;
17+
import com.google.devtools.build.lib.remote.Retrier;
18+
import com.google.devtools.build.lib.remote.common.CacheNotFoundException;
19+
import com.google.devtools.build.lib.remote.options.RemoteOptions;
20+
21+
/** Factory for {@link Retrier.CircuitBreaker} */
22+
public class CircuitBreakerFactory {
23+
24+
public static final ImmutableSet<Class<? extends Exception>> DEFAULT_IGNORED_ERRORS =
25+
ImmutableSet.of(CacheNotFoundException.class);
26+
27+
private CircuitBreakerFactory() {}
28+
29+
/**
30+
* Creates the instance of the {@link Retrier.CircuitBreaker} as per the strategy defined in
31+
* {@link RemoteOptions}. In case of undefined strategy defaults to {@link
32+
* Retrier.ALLOW_ALL_CALLS} implementation.
33+
*
34+
* @param remoteOptions The configuration for the CircuitBreaker implementation.
35+
* @return an instance of CircuitBreaker.
36+
*/
37+
public static Retrier.CircuitBreaker createCircuitBreaker(final RemoteOptions remoteOptions) {
38+
if (remoteOptions.circuitBreakerStrategy == RemoteOptions.CircuitBreakerStrategy.FAILURE) {
39+
return new FailureCircuitBreaker(
40+
remoteOptions.remoteFailureThreshold,
41+
(int) remoteOptions.remoteFailureWindowInterval.toMillis());
42+
}
43+
return Retrier.ALLOW_ALL_CALLS;
44+
}
45+
}
Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
// Copyright 2023 The Bazel Authors. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
package com.google.devtools.build.lib.remote.circuitbreaker;
15+
16+
import com.google.common.collect.ImmutableSet;
17+
import com.google.devtools.build.lib.remote.Retrier;
18+
import java.util.concurrent.Executors;
19+
import java.util.concurrent.ScheduledExecutorService;
20+
import java.util.concurrent.TimeUnit;
21+
import java.util.concurrent.atomic.AtomicInteger;
22+
23+
/**
24+
* The {@link FailureCircuitBreaker} implementation of the {@link Retrier.CircuitBreaker} prevents
25+
* further calls to a remote cache once the number of failures within a given window exceeds a
26+
* specified threshold for a build. In the context of Bazel, a new instance of {@link
27+
* Retrier.CircuitBreaker} is created for each build. Therefore, if the circuit breaker trips during
28+
* a build, the remote cache will be disabled for that build. However, it will be enabled again for
29+
* the next build as a new instance of {@link Retrier.CircuitBreaker} will be created.
30+
*/
31+
public class FailureCircuitBreaker implements Retrier.CircuitBreaker {
32+
33+
private State state;
34+
private final AtomicInteger failures;
35+
private final int failureThreshold;
36+
private final int slidingWindowSize;
37+
private final ScheduledExecutorService scheduledExecutor;
38+
private final ImmutableSet<Class<? extends Exception>> ignoredErrors;
39+
40+
/**
41+
* Creates a {@link FailureCircuitBreaker}.
42+
*
43+
* @param failureThreshold is used to set the number of failures required to trip the circuit
44+
* breaker in given time window.
45+
* @param slidingWindowSize the size of the sliding window in milliseconds to calculate the number
46+
* of failures.
47+
*/
48+
public FailureCircuitBreaker(int failureThreshold, int slidingWindowSize) {
49+
this.failureThreshold = failureThreshold;
50+
this.failures = new AtomicInteger(0);
51+
this.slidingWindowSize = slidingWindowSize;
52+
this.state = State.ACCEPT_CALLS;
53+
this.scheduledExecutor =
54+
slidingWindowSize > 0 ? Executors.newSingleThreadScheduledExecutor() : null;
55+
this.ignoredErrors = CircuitBreakerFactory.DEFAULT_IGNORED_ERRORS;
56+
}
57+
58+
@Override
59+
public State state() {
60+
return this.state;
61+
}
62+
63+
@Override
64+
public void recordFailure(Exception e) {
65+
if (!ignoredErrors.contains(e.getClass())) {
66+
int failureCount = failures.incrementAndGet();
67+
if (slidingWindowSize > 0) {
68+
var unused =
69+
scheduledExecutor.schedule(
70+
failures::decrementAndGet, slidingWindowSize, TimeUnit.MILLISECONDS);
71+
}
72+
// Since the state can only be changed to the open state, synchronization is not required.
73+
if (failureCount > this.failureThreshold) {
74+
this.state = State.REJECT_CALLS;
75+
}
76+
}
77+
}
78+
79+
@Override
80+
public void recordSuccess() {
81+
// do nothing, implement if we need to set threshold on failure rate instead of count.
82+
}
83+
}

src/main/java/com/google/devtools/build/lib/remote/options/CommonRemoteOptions.java

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,16 @@
1313
// limitations under the License.
1414
package com.google.devtools.build.lib.remote.options;
1515

16+
import com.google.devtools.common.options.Converter;
17+
import com.google.devtools.common.options.Converters;
1618
import com.google.devtools.common.options.Option;
1719
import com.google.devtools.common.options.OptionDocumentationCategory;
1820
import com.google.devtools.common.options.OptionEffectTag;
1921
import com.google.devtools.common.options.OptionsBase;
22+
import com.google.devtools.common.options.OptionsParsingException;
23+
import java.time.Duration;
2024
import java.util.List;
25+
import java.util.regex.Pattern;
2126

2227
/** Options for remote execution and distributed caching that shared between Bazel and Blaze. */
2328
public class CommonRemoteOptions extends OptionsBase {
@@ -33,4 +38,23 @@ public class CommonRemoteOptions extends OptionsBase {
3338
+ " the client to request certain artifacts that might be needed locally (e.g. IDE"
3439
+ " support)")
3540
public List<String> remoteDownloadRegex;
41+
42+
/** Returns the specified duration. Assumes seconds if unitless. */
43+
public static class RemoteDurationConverter extends Converter.Contextless<Duration> {
44+
45+
private static final Pattern UNITLESS_REGEX = Pattern.compile("^[0-9]+$");
46+
47+
@Override
48+
public Duration convert(String input) throws OptionsParsingException {
49+
if (UNITLESS_REGEX.matcher(input).matches()) {
50+
input += "s";
51+
}
52+
return new Converters.DurationConverter().convert(input, /* conversionContext= */ null);
53+
}
54+
55+
@Override
56+
public String getTypeDescription() {
57+
return "An immutable length of time.";
58+
}
59+
}
3660
}

0 commit comments

Comments
 (0)