Skip to content

Commit 275c854

Browse files
pavan-traceablepavan-traceable
committed
Addressed comments
1 parent 878a8cf commit 275c854

File tree

9 files changed

+89
-68
lines changed

9 files changed

+89
-68
lines changed

‎grpc-circuitbreaker-utils/build.gradle.kts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ dependencies {
1414
implementation("org.slf4j:slf4j-api:1.7.36")
1515
implementation("io.github.resilience4j:resilience4j-circuitbreaker:1.7.1")
1616
implementation("com.typesafe:config:1.4.2")
17-
implementation("com.google.inject:guice:7.0.0")
1817

1918
annotationProcessor("org.projectlombok:lombok:1.18.24")
2019
compileOnly("org.projectlombok:lombok:1.18.24")

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

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,18 @@
11
package org.hypertrace.circuitbreaker.grpcutils;
22

33
import com.typesafe.config.Config;
4+
import java.time.Duration;
45
import java.util.Map;
6+
import java.util.Set;
57
import java.util.stream.Collectors;
68
import lombok.extern.slf4j.Slf4j;
79

810
@Slf4j
911
public class CircuitBreakerConfigParser {
1012

1113
public static final String DEFAULT_CONFIG_KEY = "default";
14+
private static final Set<String> nonThresholdKeys =
15+
Set.of("enabled", "defaultCircuitBreakerKey", "defaultThresholds");
1216

1317
// Percentage of failures to trigger OPEN state
1418
private static final String FAILURE_RATE_THRESHOLD = "failureRateThreshold";
@@ -27,19 +31,38 @@ public class CircuitBreakerConfigParser {
2731
private static final String PERMITTED_NUMBER_OF_CALLS_IN_HALF_OPEN_STATE =
2832
"permittedNumberOfCallsInHalfOpenState";
2933
private static final String SLIDING_WINDOW_TYPE = "slidingWindowType";
34+
public static final String ENABLED = "enabled";
35+
public static final String DEFAULT_CIRCUIT_BREAKER_KEY = "defaultCircuitBreakerKey";
36+
public static final String DEFAULT_THRESHOLDS = "defaultThresholds";
3037

3138
public static <T> CircuitBreakerConfiguration.CircuitBreakerConfigurationBuilder<T> parseConfig(
3239
Config config) {
3340
CircuitBreakerConfiguration.CircuitBreakerConfigurationBuilder<T> builder =
3441
CircuitBreakerConfiguration.builder();
42+
if (config.hasPath(ENABLED)) {
43+
builder.enabled(config.getBoolean(ENABLED));
44+
}
45+
46+
if (config.hasPath(DEFAULT_CIRCUIT_BREAKER_KEY)) {
47+
builder.defaultCircuitBreakerKey(config.getString(DEFAULT_CIRCUIT_BREAKER_KEY));
48+
}
49+
50+
if (config.hasPath(DEFAULT_THRESHOLDS)) {
51+
builder.defaultThresholds(
52+
buildCircuitBreakerThresholds(config.getConfig(DEFAULT_THRESHOLDS)));
53+
} else {
54+
builder.defaultThresholds(buildCircuitBreakerDefaultThresholds());
55+
}
56+
3557
Map<String, CircuitBreakerThresholds> circuitBreakerThresholdsMap =
3658
config.root().keySet().stream()
59+
.filter(key -> !nonThresholdKeys.contains(key)) // Filter out non-threshold keys
3760
.collect(
3861
Collectors.toMap(
3962
key -> key, // Circuit breaker key
4063
key -> buildCircuitBreakerThresholds(config.getConfig(key))));
4164
builder.circuitBreakerThresholdsMap(circuitBreakerThresholdsMap);
42-
log.info("Loaded circuit breaker configs: {}", circuitBreakerThresholdsMap);
65+
log.info("Loaded circuit breaker configs: {}", builder);
4366
return builder;
4467
}
4568

@@ -57,6 +80,19 @@ private static CircuitBreakerThresholds buildCircuitBreakerThresholds(Config con
5780
.build();
5881
}
5982

83+
public static CircuitBreakerThresholds buildCircuitBreakerDefaultThresholds() {
84+
return CircuitBreakerThresholds.builder()
85+
.failureRateThreshold(50f)
86+
.slowCallRateThreshold(50f)
87+
.slowCallDurationThreshold(Duration.ofSeconds(2))
88+
.slidingWindowType(CircuitBreakerThresholds.SlidingWindowType.TIME_BASED)
89+
.slidingWindowSize(60)
90+
.waitDurationInOpenState(Duration.ofSeconds(60))
91+
.permittedNumberOfCallsInHalfOpenState(5)
92+
.minimumNumberOfCalls(10)
93+
.build();
94+
}
95+
6096
private static CircuitBreakerThresholds.SlidingWindowType getSlidingWindowType(
6197
String slidingWindowType) {
6298
return CircuitBreakerThresholds.SlidingWindowType.valueOf(slidingWindowType);

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,11 @@
1111
public class CircuitBreakerConfiguration<T> {
1212
Class<T> requestClass;
1313
BiFunction<RequestContext, T, String> keyFunction;
14-
boolean enabled;
15-
Map<String, CircuitBreakerThresholds> circuitBreakerThresholdsMap;
14+
@Builder.Default boolean enabled = false;
15+
// Default value be "global" if not override.
16+
@Builder.Default String defaultCircuitBreakerKey = "global";
17+
// Standard/default thresholds
18+
CircuitBreakerThresholds defaultThresholds;
19+
// Custom overrides for specific cases (less common)
20+
@Builder.Default Map<String, CircuitBreakerThresholds> circuitBreakerThresholdsMap = Map.of();
1621
}

grpc-circuitbreaker-utils/src/main/java/org/hypertrace/circuitbreaker/grpcutils/resilience/ResilienceCircuitBreakerConfigParser.java renamed to grpc-circuitbreaker-utils/src/main/java/org/hypertrace/circuitbreaker/grpcutils/resilience/ResilienceCircuitBreakerConfigConverter.java

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,15 @@
66
import org.hypertrace.circuitbreaker.grpcutils.CircuitBreakerThresholds;
77

88
/** Utility class to parse CircuitBreakerConfiguration to Resilience4j CircuitBreakerConfig */
9-
public class ResilienceCircuitBreakerConfigParser {
9+
public class ResilienceCircuitBreakerConfigConverter {
1010

1111
public static Map<String, CircuitBreakerConfig> getCircuitBreakerConfigs(
1212
Map<String, CircuitBreakerThresholds> configurationMap) {
1313
return configurationMap.entrySet().stream()
14-
.collect(Collectors.toMap(Map.Entry::getKey, entry -> getConfig(entry.getValue())));
14+
.collect(Collectors.toMap(Map.Entry::getKey, entry -> convertConfig(entry.getValue())));
1515
}
1616

17-
static CircuitBreakerConfig getConfig(CircuitBreakerThresholds configuration) {
17+
static CircuitBreakerConfig convertConfig(CircuitBreakerThresholds configuration) {
1818
return CircuitBreakerConfig.custom()
1919
.failureRateThreshold(configuration.getFailureRateThreshold())
2020
.slowCallRateThreshold(configuration.getSlowCallRateThreshold())
@@ -30,9 +30,6 @@ static CircuitBreakerConfig getConfig(CircuitBreakerThresholds configuration) {
3030

3131
private static CircuitBreakerConfig.SlidingWindowType getSlidingWindowType(
3232
CircuitBreakerThresholds.SlidingWindowType slidingWindowType) {
33-
if (slidingWindowType == null) {
34-
return CircuitBreakerConfig.SlidingWindowType.TIME_BASED;
35-
}
3633
switch (slidingWindowType) {
3734
case COUNT_BASED:
3835
return CircuitBreakerConfig.SlidingWindowType.COUNT_BASED;

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

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

3-
import com.google.common.annotations.VisibleForTesting;
43
import io.github.resilience4j.circuitbreaker.CircuitBreaker;
54
import io.github.resilience4j.circuitbreaker.CircuitBreakerConfig;
65
import io.github.resilience4j.circuitbreaker.CircuitBreakerRegistry;
@@ -36,25 +35,25 @@ public ResilienceCircuitBreakerInterceptor(
3635
this.circuitBreakerConfiguration = circuitBreakerConfiguration;
3736
this.clock = clock;
3837
this.resilienceCircuitBreakerConfigMap =
39-
ResilienceCircuitBreakerConfigParser.getCircuitBreakerConfigs(
38+
ResilienceCircuitBreakerConfigConverter.getCircuitBreakerConfigs(
4039
circuitBreakerConfiguration.getCircuitBreakerThresholdsMap());
4140
this.resilicenceCircuitBreakerRegistry =
42-
new ResilienceCircuitBreakerRegistryProvider(resilienceCircuitBreakerConfigMap)
41+
new ResilienceCircuitBreakerRegistryProvider(
42+
circuitBreakerConfiguration.getDefaultThresholds())
4343
.getCircuitBreakerRegistry();
4444
this.resilienceCircuitBreakerProvider =
4545
new ResilienceCircuitBreakerProvider(
4646
resilicenceCircuitBreakerRegistry, resilienceCircuitBreakerConfigMap);
4747
}
4848

49-
@VisibleForTesting
5049
public ResilienceCircuitBreakerInterceptor(
50+
CircuitBreakerConfiguration<?> circuitBreakerConfiguration,
5151
Clock clock,
5252
CircuitBreakerRegistry resilicenceCircuitBreakerRegistry,
53-
ResilienceCircuitBreakerProvider resilienceCircuitBreakerProvider,
54-
CircuitBreakerConfiguration<?> circuitBreakerConfiguration) {
53+
ResilienceCircuitBreakerProvider resilienceCircuitBreakerProvider) {
5554
this.circuitBreakerConfiguration = circuitBreakerConfiguration;
5655
this.resilienceCircuitBreakerConfigMap =
57-
ResilienceCircuitBreakerConfigParser.getCircuitBreakerConfigs(
56+
ResilienceCircuitBreakerConfigConverter.getCircuitBreakerConfigs(
5857
circuitBreakerConfiguration.getCircuitBreakerThresholdsMap());
5958
this.resilicenceCircuitBreakerRegistry = resilicenceCircuitBreakerRegistry;
6059
this.resilienceCircuitBreakerProvider = resilienceCircuitBreakerProvider;
@@ -88,14 +87,10 @@ public void start(Listener<RespT> responseListener, Metadata headers) {
8887
public void sendMessage(ReqT message) {
8988
CircuitBreakerConfiguration<ReqT> config =
9089
(CircuitBreakerConfiguration<ReqT>) circuitBreakerConfiguration;
91-
if (config.getRequestClass() == null || config.getKeyFunction() == null) {
92-
log.debug("Circuit breaker will apply to all requests as config is not set");
93-
circuitBreakerKey = "default";
90+
if (config.getKeyFunction() == null) {
91+
log.debug("Circuit breaker will apply to all requests as keyFunction config is not set");
92+
circuitBreakerKey = config.getDefaultCircuitBreakerKey();
9493
} else {
95-
if (!message.getClass().equals(config.getRequestClass())) {
96-
log.warn("Invalid config for message type: {}", message.getClass());
97-
super.sendMessage(message);
98-
}
9994
circuitBreakerKey = config.getKeyFunction().apply(RequestContext.CURRENT.get(), message);
10095
}
10196
circuitBreaker = resilienceCircuitBreakerProvider.getCircuitBreaker(circuitBreakerKey);

‎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,14 +3,13 @@
33
import io.github.resilience4j.circuitbreaker.CircuitBreaker;
44
import io.github.resilience4j.circuitbreaker.CircuitBreakerConfig;
55
import io.github.resilience4j.circuitbreaker.CircuitBreakerRegistry;
6-
import jakarta.inject.Singleton;
76
import java.util.Map;
7+
import java.util.Optional;
88
import java.util.concurrent.ConcurrentHashMap;
99
import lombok.extern.slf4j.Slf4j;
1010

1111
/** Utility class to provide Resilience4j CircuitBreaker */
1212
@Slf4j
13-
@Singleton
1413
public class ResilienceCircuitBreakerProvider {
1514

1615
private final CircuitBreakerRegistry circuitBreakerRegistry;
@@ -29,10 +28,10 @@ public CircuitBreaker getCircuitBreaker(String circuitBreakerKey) {
2928
circuitBreakerKey,
3029
key -> {
3130
CircuitBreaker circuitBreaker =
32-
circuitBreakerRegistry.circuitBreaker(
33-
circuitBreakerKey,
34-
circuitBreakerConfigMap.getOrDefault(
35-
circuitBreakerKey, circuitBreakerConfigMap.get("default")));
31+
Optional.ofNullable(circuitBreakerConfigMap.get(circuitBreakerKey))
32+
.map(config -> circuitBreakerRegistry.circuitBreaker(circuitBreakerKey, config))
33+
.orElseGet(() -> circuitBreakerRegistry.circuitBreaker(circuitBreakerKey));
34+
3635
circuitBreaker
3736
.getEventPublisher()
3837
.onStateTransition(

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

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

33
import io.github.resilience4j.circuitbreaker.CircuitBreaker;
4-
import io.github.resilience4j.circuitbreaker.CircuitBreakerConfig;
54
import io.github.resilience4j.circuitbreaker.CircuitBreakerRegistry;
6-
import jakarta.inject.Singleton;
7-
import java.util.Map;
85
import lombok.extern.slf4j.Slf4j;
9-
import org.hypertrace.circuitbreaker.grpcutils.CircuitBreakerConfigParser;
6+
import org.hypertrace.circuitbreaker.grpcutils.CircuitBreakerThresholds;
107

118
/** Utility class to provide Resilience4j CircuitBreakerRegistry */
129
@Slf4j
13-
@Singleton
1410
public class ResilienceCircuitBreakerRegistryProvider {
15-
private final Map<String, CircuitBreakerConfig> circuitBreakerConfigMap;
11+
private final CircuitBreakerThresholds circuitBreakerThresholds;
1612

1713
public ResilienceCircuitBreakerRegistryProvider(
18-
Map<String, CircuitBreakerConfig> circuitBreakerConfigMap) {
19-
this.circuitBreakerConfigMap = circuitBreakerConfigMap;
14+
CircuitBreakerThresholds circuitBreakerThresholds) {
15+
this.circuitBreakerThresholds = circuitBreakerThresholds;
2016
}
2117

2218
public CircuitBreakerRegistry getCircuitBreakerRegistry() {
2319
CircuitBreakerRegistry circuitBreakerRegistry =
2420
CircuitBreakerRegistry.of(
25-
this.circuitBreakerConfigMap.get(CircuitBreakerConfigParser.DEFAULT_CONFIG_KEY));
21+
ResilienceCircuitBreakerConfigConverter.convertConfig(circuitBreakerThresholds));
2622
circuitBreakerRegistry
2723
.getEventPublisher()
2824
.onEntryAdded(

grpc-circuitbreaker-utils/src/test/java/org/hypertrace/circuitbreaker/grpcutils/resilience/ResilienceCircuitBreakerConfigParserTest.java renamed to grpc-circuitbreaker-utils/src/test/java/org/hypertrace/circuitbreaker/grpcutils/resilience/ResilienceCircuitBreakerConfigConverterTest.java

Lines changed: 4 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
import org.junit.jupiter.api.Assertions;
1212
import org.junit.jupiter.api.Test;
1313

14-
public class ResilienceCircuitBreakerConfigParserTest {
14+
public class ResilienceCircuitBreakerConfigConverterTest {
1515

1616
@Test
1717
void shouldParseValidConfiguration() {
@@ -31,7 +31,7 @@ void shouldParseValidConfiguration() {
3131
configMap.put("testService", thresholds);
3232

3333
Map<String, CircuitBreakerConfig> result =
34-
ResilienceCircuitBreakerConfigParser.getCircuitBreakerConfigs(configMap);
34+
ResilienceCircuitBreakerConfigConverter.getCircuitBreakerConfigs(configMap);
3535

3636
Assertions.assertTrue(result.containsKey("testService"));
3737

@@ -45,25 +45,10 @@ void shouldParseValidConfiguration() {
4545
assertEquals(20, config.getMinimumNumberOfCalls());
4646
}
4747

48-
@Test
49-
void shouldUseDefaultSlidingWindowTypeForInvalidType() {
50-
CircuitBreakerThresholds thresholds =
51-
CircuitBreakerThresholds.builder()
52-
.failureRateThreshold(50.0f)
53-
.slowCallRateThreshold(30.0f)
54-
.slowCallDurationThreshold(Duration.ofSeconds(2))
55-
.slidingWindowSize(100)
56-
.waitDurationInOpenState(Duration.ofSeconds(60))
57-
.permittedNumberOfCallsInHalfOpenState(5)
58-
.minimumNumberOfCalls(20)
59-
.build(); // Invalid type scenario
60-
CircuitBreakerConfig config = ResilienceCircuitBreakerConfigParser.getConfig(thresholds);
61-
assertEquals(CircuitBreakerConfig.SlidingWindowType.TIME_BASED, config.getSlidingWindowType());
62-
}
63-
6448
@Test
6549
void shouldThrowExceptionWhenConfigurationIsNull() {
6650
assertThrows(
67-
NullPointerException.class, () -> ResilienceCircuitBreakerConfigParser.getConfig(null));
51+
NullPointerException.class,
52+
() -> ResilienceCircuitBreakerConfigConverter.convertConfig(null));
6853
}
6954
}

‎grpc-circuitbreaker-utils/src/test/java/org/hypertrace/circuitbreaker/grpcutils/resilience/ResilienceCircuitBreakerInterceptorTest.java‎

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,9 @@
1111
import java.time.Clock;
1212
import java.time.Instant;
1313
import java.time.ZoneOffset;
14+
import java.util.Map;
1415
import java.util.concurrent.TimeUnit;
16+
import org.hypertrace.circuitbreaker.grpcutils.CircuitBreakerConfigParser;
1517
import org.hypertrace.circuitbreaker.grpcutils.CircuitBreakerConfiguration;
1618
import org.junit.jupiter.api.BeforeEach;
1719
import org.junit.jupiter.api.Test;
@@ -27,6 +29,7 @@ class ResilienceCircuitBreakerInterceptorTest {
2729
@Mock private CircuitBreaker mockCircuitBreaker;
2830
@Mock private Metadata mockMetadata;
2931
@Mock private ClientCall.Listener<Object> mockListener;
32+
@Mock private ResilienceCircuitBreakerRegistryProvider mockCircuitBreakerRegistryProvider;
3033
@Mock private ResilienceCircuitBreakerProvider mockCircuitBreakerProvider;
3134
@Mock private CircuitBreakerConfiguration<Object> mockCircuitBreakerConfig;
3235
@Mock private CircuitBreakerRegistry mockCircuitBreakerRegistry;
@@ -39,7 +42,13 @@ void setUp() {
3942

4043
fixedClock = Clock.fixed(Instant.now(), ZoneOffset.UTC);
4144
when(mockChannel.newCall(any(), any())).thenReturn(mockClientCall);
45+
// when(mockCircuitBreakerRegistryProvider.getCircuitBreakerRegistry())
46+
// .thenReturn(mockCircuitBreakerRegistry);
4247
when(mockCircuitBreakerProvider.getCircuitBreaker(anyString())).thenReturn(mockCircuitBreaker);
48+
when(mockCircuitBreakerConfig.getDefaultCircuitBreakerKey()).thenReturn("global");
49+
when(mockCircuitBreakerConfig.getCircuitBreakerThresholdsMap())
50+
.thenReturn(
51+
Map.of("global", CircuitBreakerConfigParser.buildCircuitBreakerDefaultThresholds()));
4352
}
4453

4554
@Test
@@ -49,10 +58,10 @@ void testSendMessage_CallsSuperSendMessage_Success() {
4958

5059
ResilienceCircuitBreakerInterceptor interceptor =
5160
new ResilienceCircuitBreakerInterceptor(
61+
mockCircuitBreakerConfig,
5262
fixedClock,
5363
mockCircuitBreakerRegistry,
54-
mockCircuitBreakerProvider,
55-
mockCircuitBreakerConfig);
64+
mockCircuitBreakerProvider);
5665

5766
ClientCall<Object, Object> interceptedCall =
5867
interceptor.createInterceptedCall(
@@ -70,10 +79,10 @@ void testSendMessage_CircuitBreakerRejectsRequest() {
7079
when(mockCircuitBreaker.getState()).thenReturn(CircuitBreaker.State.OPEN);
7180
ResilienceCircuitBreakerInterceptor interceptor =
7281
new ResilienceCircuitBreakerInterceptor(
82+
mockCircuitBreakerConfig,
7383
fixedClock,
7484
mockCircuitBreakerRegistry,
75-
mockCircuitBreakerProvider,
76-
mockCircuitBreakerConfig);
85+
mockCircuitBreakerProvider);
7786

7887
ClientCall<Object, Object> interceptedCall =
7988
interceptor.createInterceptedCall(
@@ -95,10 +104,10 @@ void testSendMessage_CircuitBreakerInHalfOpenState() {
95104
when(mockCircuitBreaker.getState()).thenReturn(CircuitBreaker.State.HALF_OPEN);
96105
ResilienceCircuitBreakerInterceptor interceptor =
97106
new ResilienceCircuitBreakerInterceptor(
107+
mockCircuitBreakerConfig,
98108
fixedClock,
99109
mockCircuitBreakerRegistry,
100-
mockCircuitBreakerProvider,
101-
mockCircuitBreakerConfig);
110+
mockCircuitBreakerProvider);
102111

103112
ClientCall<Object, Object> interceptedCall =
104113
interceptor.createInterceptedCall(
@@ -119,10 +128,10 @@ void testWrapListenerWithCircuitBreaker_Success() {
119128
when(mockCircuitBreaker.tryAcquirePermission()).thenReturn(true);
120129
ResilienceCircuitBreakerInterceptor interceptor =
121130
new ResilienceCircuitBreakerInterceptor(
131+
mockCircuitBreakerConfig,
122132
fixedClock,
123133
mockCircuitBreakerRegistry,
124-
mockCircuitBreakerProvider,
125-
mockCircuitBreakerConfig);
134+
mockCircuitBreakerProvider);
126135

127136
ClientCall<Object, Object> interceptedCall =
128137
interceptor.createInterceptedCall(
@@ -146,10 +155,10 @@ void testWrapListenerWithCircuitBreaker_Failure() {
146155
when(mockCircuitBreaker.tryAcquirePermission()).thenReturn(true);
147156
ResilienceCircuitBreakerInterceptor interceptor =
148157
new ResilienceCircuitBreakerInterceptor(
158+
mockCircuitBreakerConfig,
149159
fixedClock,
150160
mockCircuitBreakerRegistry,
151-
mockCircuitBreakerProvider,
152-
mockCircuitBreakerConfig);
161+
mockCircuitBreakerProvider);
153162

154163
ClientCall<Object, Object> interceptedCall =
155164
interceptor.createInterceptedCall(

0 commit comments

Comments
 (0)