Allows the ability to configure bulkheads using config properties#228
Conversation
Also addresses issue where semaphore bulkhead was used when threadpool config was supplied. Fixes spring-cloud#224
|
@OathMeadow this PR addresses the issues in #224. Let me know what you think. |
| builder.bulkheadConfig(bulkheadConfiguration.orElse(bulkheadRegistry.getDefaultConfig())); | ||
| builder | ||
| .threadPoolBulkheadConfig(threadPoolBulkheadConfig.orElse(threadPoolBulkheadRegistry.getDefaultConfig())); | ||
| return builder.build(); |
There was a problem hiding this comment.
I was wondering what function the defaultConfiguration would serve.
Currently only used in addBulkheadCustomizer and addThreadPoolBulkheadCustomizer.
We could maybe handle this similair to what we do in Resilience4JCircuitBreakerFactory.
private Resilience4JCircuitBreaker create(String id, String groupName,
ExecutorService circuitBreakerExecutorService) {
Resilience4JConfigBuilder.Resilience4JCircuitBreakerConfiguration defaultConfig = getConfigurations()
.computeIfAbsent(id, defaultConfiguration);
CircuitBreakerConfig circuitBreakerConfig = this.circuitBreakerRegistry.getConfiguration(id)
.orElseGet(() -> this.circuitBreakerRegistry.getConfiguration(groupName)
.orElseGet(defaultConfig::getCircuitBreakerConfig));
TimeLimiterConfig timeLimiterConfig = this.timeLimiterRegistry.getConfiguration(id)
.orElseGet(() -> this.timeLimiterRegistry.getConfiguration(groupName)
.orElseGet(defaultConfig::getTimeLimiterConfig));
if (resilience4JConfigurationProperties.isDisableThreadPool()) {
return new Resilience4JCircuitBreaker(id, groupName, circuitBreakerConfig, timeLimiterConfig,
circuitBreakerRegistry, timeLimiterRegistry, Optional.ofNullable(circuitBreakerCustomizers.get(id)),
bulkheadProvider);
}
else {
return new Resilience4JCircuitBreaker(id, groupName, circuitBreakerConfig, timeLimiterConfig,
circuitBreakerRegistry, timeLimiterRegistry, circuitBreakerExecutorService,
Optional.ofNullable(circuitBreakerCustomizers.get(id)), bulkheadProvider,
this.resilience4JConfigurationProperties.isDisableTimeLimiter());
}
}
There was a problem hiding this comment.
Good catch, we should be using the defaultConfiguration variable in the case where that was specifically set by the user.
| /** | ||
| * @author Ryan Baxter | ||
| */ | ||
| public class BulkheadConfigurationTests { |
There was a problem hiding this comment.
Maybe we should also add default config tests.
example: resilience4j.threadPoolBulkHead.configs.default.queueCapacity=30
| */ | ||
| public class BulkheadConfigurationTests { | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Do we want to test these?
What is expected?
Resilience4jBulkheadProvider::configureDefault
Resilience4jBulkheadProvider::configure
Resilience4jBulkheadProvider::addBulkheadCustomizer
Resilience4jBulkheadProvider::addThreadPoolBulkheadCustomizer
There was a problem hiding this comment.
Example:
Resilience4jBulkheadProvider::configureDefault vs resilience4j.threadPoolBulkHead.configs.default.queueCapacity=30
In my opinion, when Resilience4jBulkheadProvider::configureDefault is used, then it take precedence over property. Perhaps same with Resilience4jBulkheadProvider::configure ?
There was a problem hiding this comment.
Resilience4jBulkheadProvider::configureDefault, Resilience4jBulkheadProvider::addBulkheadCustomizer, Resilience4jBulkheadProvider::addThreadPoolBulkheadCustomizer are used in Resilience4JBulkheadIntegrationTest and and Resilience4JBulkheadAndTimeLimiterIntegrationTest.
I agree on the configureDefault and I added some tests for that.
ryanjbaxter
left a comment
ryanjbaxter
left a comment
There was a problem hiding this comment.
Thanks, made some changes based on your feedback
| builder.bulkheadConfig(bulkheadConfiguration.orElse(bulkheadRegistry.getDefaultConfig())); | ||
| builder | ||
| .threadPoolBulkheadConfig(threadPoolBulkheadConfig.orElse(threadPoolBulkheadRegistry.getDefaultConfig())); | ||
| return builder.build(); |
There was a problem hiding this comment.
Good catch, we should be using the defaultConfiguration variable in the case where that was specifically set by the user.
| */ | ||
| public class BulkheadConfigurationTests { | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Resilience4jBulkheadProvider::configureDefault, Resilience4jBulkheadProvider::addBulkheadCustomizer, Resilience4jBulkheadProvider::addThreadPoolBulkheadCustomizer are used in Resilience4JBulkheadIntegrationTest and and Resilience4JBulkheadAndTimeLimiterIntegrationTest.
I agree on the configureDefault and I added some tests for that.
|
|
||
| private Resilience4jBulkheadConfigurationBuilder.BulkheadConfiguration getConfiguration(String id) { | ||
| Resilience4jBulkheadConfigurationBuilder builder = new Resilience4jBulkheadConfigurationBuilder(); | ||
| Resilience4jBulkheadConfigurationBuilder.BulkheadConfiguration defaultConfiguration = this.defaultConfiguration |
There was a problem hiding this comment.
This is just a minor suggestion really.
The method configureDefault(..) allows to set the variable defaultConfiguration to null.
ryanjbaxter
force-pushed
the
bulkhead-configuration-fix
branch
from
3a69848 to
b5bfcf7
Compare
3 participants
Also addresses issue where semaphore bulkhead was used when threadpool config was supplied.
Fixes #224