Skip to content

Commit 75fb8b1

Browse files
pavan-traceablepavan-traceable
committed
Addressed comments
1 parent 8ca26b7 commit 75fb8b1

File tree

5 files changed

+40
-27
lines changed

5 files changed

+40
-27
lines changed

‎grpc-circuitbreaker-utils/src/main/java/org/hypertrace/circuitbreaker/grpcutils/CircuitBreakerConfigParser.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ public class CircuitBreakerConfigParser {
2828
private static final String SLIDING_WINDOW_TYPE = "slidingWindowType";
2929
public static final String ENABLED = "enabled";
3030
public static final String DEFAULT_THRESHOLDS = "defaultThresholds";
31-
private static final Set<String> NON_THRESHOLD_KEYS = Set.of(ENABLED);
31+
private static final Set<String> NON_THRESHOLD_KEYS = Set.of(ENABLED, DEFAULT_THRESHOLDS);
3232

3333
public static <T> CircuitBreakerConfiguration.CircuitBreakerConfigurationBuilder<T> parseConfig(
3434
Config config) {
@@ -46,10 +46,10 @@ public static <T> CircuitBreakerConfiguration.CircuitBreakerConfigurationBuilder
4646
key -> key, // Circuit breaker key
4747
key -> buildCircuitBreakerThresholds(config.getConfig(key))));
4848

49-
if (!config.hasPath(DEFAULT_THRESHOLDS)) {
50-
builder.defaultThresholds(buildCircuitBreakerDefaultThresholds());
51-
circuitBreakerThresholdsMap.put(DEFAULT_THRESHOLDS, buildCircuitBreakerDefaultThresholds());
52-
}
49+
builder.defaultThresholds(
50+
config.hasPath(DEFAULT_THRESHOLDS)
51+
? buildCircuitBreakerThresholds(config.getConfig(DEFAULT_THRESHOLDS))
52+
: buildCircuitBreakerDefaultThresholds());
5353

5454
builder.circuitBreakerThresholdsMap(circuitBreakerThresholdsMap);
5555
log.debug("Loaded circuit breaker configs: {}", builder);

‎grpc-circuitbreaker-utils/src/main/java/org/hypertrace/circuitbreaker/grpcutils/resilience/ResilienceCircuitBreakerConfigConverter.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package org.hypertrace.circuitbreaker.grpcutils.resilience;
22

33
import io.github.resilience4j.circuitbreaker.CircuitBreakerConfig;
4+
import java.util.List;
45
import java.util.Map;
56
import java.util.stream.Collectors;
67
import org.hypertrace.circuitbreaker.grpcutils.CircuitBreakerThresholds;
@@ -14,6 +15,14 @@ public static Map<String, CircuitBreakerConfig> getCircuitBreakerConfigs(
1415
.collect(Collectors.toMap(Map.Entry::getKey, entry -> convertConfig(entry.getValue())));
1516
}
1617

18+
public static List<String> getDisabledKeys(
19+
Map<String, CircuitBreakerThresholds> configurationMap) {
20+
return configurationMap.entrySet().stream()
21+
.filter(entry -> entry.getValue().isEnabled())
22+
.map(Map.Entry::getKey)
23+
.collect(Collectors.toList());
24+
}
25+
1726
static CircuitBreakerConfig convertConfig(CircuitBreakerThresholds configuration) {
1827
return CircuitBreakerConfig.custom()
1928
.failureRateThreshold(configuration.getFailureRateThreshold())

‎grpc-circuitbreaker-utils/src/main/java/org/hypertrace/circuitbreaker/grpcutils/resilience/ResilienceCircuitBreakerFactory.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,15 +12,16 @@ public static ResilienceCircuitBreakerInterceptor getResilienceCircuitBreakerInt
1212
Map<String, CircuitBreakerConfig> resilienceCircuitBreakerConfigMap =
1313
ResilienceCircuitBreakerConfigConverter.getCircuitBreakerConfigs(
1414
circuitBreakerConfiguration.getCircuitBreakerThresholdsMap());
15-
CircuitBreakerRegistry resilicenceCircuitBreakerRegistry =
15+
CircuitBreakerRegistry resilienceCircuitBreakerRegistry =
1616
new ResilienceCircuitBreakerRegistryProvider(
1717
circuitBreakerConfiguration.getDefaultThresholds())
1818
.getCircuitBreakerRegistry();
1919
ResilienceCircuitBreakerProvider resilienceCircuitBreakerProvider =
2020
new ResilienceCircuitBreakerProvider(
21-
resilicenceCircuitBreakerRegistry,
21+
resilienceCircuitBreakerRegistry,
2222
resilienceCircuitBreakerConfigMap,
23-
circuitBreakerConfiguration.getCircuitBreakerThresholdsMap());
23+
ResilienceCircuitBreakerConfigConverter.getDisabledKeys(
24+
circuitBreakerConfiguration.getCircuitBreakerThresholdsMap()));
2425
return new ResilienceCircuitBreakerInterceptor(
2526
circuitBreakerConfiguration, clock, resilienceCircuitBreakerProvider);
2627
}

‎grpc-circuitbreaker-utils/src/main/java/org/hypertrace/circuitbreaker/grpcutils/resilience/ResilienceCircuitBreakerInterceptor.java‎

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ protected <ReqT, RespT> ClientCall<ReqT, RespT> createInterceptedCall(
4646
MethodDescriptor<ReqT, RespT> method, CallOptions callOptions, Channel next) {
4747
return new ForwardingClientCall.SimpleForwardingClientCall<>(
4848
next.newCall(method, callOptions)) {
49-
Optional<CircuitBreaker> circuitBreaker;
49+
Optional<CircuitBreaker> optionalCircuitBreaker;
5050
String circuitBreakerKey;
5151

5252
@Override
@@ -70,19 +70,21 @@ public void sendMessage(ReqT message) {
7070
}
7171
if (config.getKeyFunction() != null) {
7272
circuitBreakerKey = config.getKeyFunction().apply(RequestContext.CURRENT.get(), message);
73-
circuitBreaker = resilienceCircuitBreakerProvider.getCircuitBreaker(circuitBreakerKey);
73+
optionalCircuitBreaker =
74+
resilienceCircuitBreakerProvider.getCircuitBreaker(circuitBreakerKey);
7475
} else {
7576
log.debug("Circuit breaker will apply to all requests as keyFunction config is not set");
76-
circuitBreaker = resilienceCircuitBreakerProvider.getDefaultCircuitBreaker();
77+
optionalCircuitBreaker = resilienceCircuitBreakerProvider.getDefaultCircuitBreaker();
7778
}
78-
if (circuitBreaker.isEmpty()) {
79+
CircuitBreaker circuitBreaker = optionalCircuitBreaker.orElse(null);
80+
if (circuitBreaker == null) {
7981
super.sendMessage(message);
8082
return;
8183
}
82-
if (!circuitBreaker.get().tryAcquirePermission()) {
83-
logCircuitBreakerRejection(circuitBreakerKey, circuitBreaker.get());
84+
if (!circuitBreaker.tryAcquirePermission()) {
85+
logCircuitBreakerRejection(circuitBreakerKey, circuitBreaker);
8486
String rejectionReason =
85-
circuitBreaker.get().getState() == CircuitBreaker.State.HALF_OPEN
87+
circuitBreaker.getState() == CircuitBreaker.State.HALF_OPEN
8688
? "Circuit Breaker is HALF-OPEN and rejecting excess requests"
8789
: "Circuit Breaker is OPEN and blocking requests";
8890
throw config.getExceptionBuilder().apply(rejectionReason);
@@ -94,21 +96,23 @@ public void sendMessage(ReqT message) {
9496
wrapListenerWithCircuitBreaker(Listener<RespT> responseListener, Instant startTime) {
9597
return new ForwardingClientCallListener.SimpleForwardingClientCallListener<>(
9698
responseListener) {
97-
@SuppressWarnings("OptionalGetWithoutIsPresent")
9899
@Override
99100
public void onClose(Status status, Metadata trailers) {
100101
long duration = Duration.between(startTime, clock.instant()).toNanos();
102+
CircuitBreaker circuitBreaker = optionalCircuitBreaker.orElse(null);
103+
if (circuitBreaker == null) {
104+
super.onClose(status, trailers);
105+
return;
106+
}
101107
if (status.isOk()) {
102-
circuitBreaker.get().onSuccess(duration, TimeUnit.NANOSECONDS);
108+
circuitBreaker.onSuccess(duration, TimeUnit.NANOSECONDS);
103109
} else {
104110
log.debug(
105111
"Circuit Breaker '{}' detected failure. Status: {}, Description: {}",
106-
circuitBreaker.get().getName(),
112+
circuitBreaker.getName(),
107113
status.getCode(),
108114
status.getDescription());
109-
circuitBreaker
110-
.get()
111-
.onError(duration, TimeUnit.NANOSECONDS, status.asRuntimeException());
115+
circuitBreaker.onError(duration, TimeUnit.NANOSECONDS, status.asRuntimeException());
112116
}
113117
super.onClose(status, trailers);
114118
}

‎grpc-circuitbreaker-utils/src/main/java/org/hypertrace/circuitbreaker/grpcutils/resilience/ResilienceCircuitBreakerProvider.java‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,34 +3,33 @@
33
import io.github.resilience4j.circuitbreaker.CircuitBreaker;
44
import io.github.resilience4j.circuitbreaker.CircuitBreakerConfig;
55
import io.github.resilience4j.circuitbreaker.CircuitBreakerRegistry;
6+
import java.util.List;
67
import java.util.Map;
78
import java.util.Optional;
89
import java.util.concurrent.ConcurrentHashMap;
910
import lombok.extern.slf4j.Slf4j;
1011
import org.hypertrace.circuitbreaker.grpcutils.CircuitBreakerConfigParser;
11-
import org.hypertrace.circuitbreaker.grpcutils.CircuitBreakerThresholds;
1212

1313
/** Utility class to provide Resilience4j CircuitBreaker */
1414
@Slf4j
1515
class ResilienceCircuitBreakerProvider {
1616

1717
private final CircuitBreakerRegistry circuitBreakerRegistry;
1818
private final Map<String, CircuitBreakerConfig> circuitBreakerConfigMap;
19-
private final Map<String, CircuitBreakerThresholds> circuitBreakerThresholdsMap;
2019
private final Map<String, CircuitBreaker> circuitBreakerCache = new ConcurrentHashMap<>();
20+
private final List<String> disabledKeys;
2121

2222
public ResilienceCircuitBreakerProvider(
2323
CircuitBreakerRegistry circuitBreakerRegistry,
2424
Map<String, CircuitBreakerConfig> circuitBreakerConfigMap,
25-
Map<String, CircuitBreakerThresholds> circuitBreakerThresholdsMap) {
25+
List<String> disabledKeys) {
2626
this.circuitBreakerRegistry = circuitBreakerRegistry;
2727
this.circuitBreakerConfigMap = circuitBreakerConfigMap;
28-
this.circuitBreakerThresholdsMap = circuitBreakerThresholdsMap;
28+
this.disabledKeys = disabledKeys;
2929
}
3030

3131
public Optional<CircuitBreaker> getCircuitBreaker(String circuitBreakerKey) {
32-
if (circuitBreakerThresholdsMap.containsKey(circuitBreakerKey)
33-
&& !circuitBreakerThresholdsMap.get(circuitBreakerKey).isEnabled()) {
32+
if (disabledKeys.contains(circuitBreakerKey)) {
3433
return Optional.empty();
3534
}
3635
return Optional.of(

0 commit comments

Comments
 (0)