-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
introduce disable-time-limiter property #176
introduce disable-time-limiter property #176
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add documentation for the new property?
Also can you submit this against the 3.0.x branch?
...rg/springframework/cloud/circuitbreaker/resilience4j/ReactiveResilience4JCircuitBreaker.java
Show resolved
Hide resolved
...ngframework/cloud/circuitbreaker/resilience4j/ReactiveResilience4JCircuitBreakerFactory.java
Show resolved
Hide resolved
...n/java/org/springframework/cloud/circuitbreaker/resilience4j/Resilience4JCircuitBreaker.java
Show resolved
Hide resolved
9892b11
to
15c4bbb
Compare
While intergrating the requested changes i noticed.. It seems like the It is a bit of a pity that the resilience4j implementation is not providing a "disabled-timelimiter" functionality. Therefore it is currently only possible to deactivate the time limiter globally. I tried to describe this within the documentation. As soon as you make use of the |
I took a look and it seems that this is the current state of I guess then this can be considered as works as designed and anybody who wants to use it would probably be in need of providing the mandatory EDIT: I just learned that missing annotations do not cause a |
Resilience4JConfigBuilder.Resilience4JCircuitBreakerConfiguration config, | ||
CircuitBreakerRegistry circuitBreakerRegistry, TimeLimiterRegistry timeLimiterRegistry, | ||
Optional<Customizer<CircuitBreaker>> circuitBreakerCustomizer) { | ||
this( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you fix the formatting here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please have a look if the reformat suits the requirements.
TimeLimiterRegistry timeLimiterRegistry, ExecutorService executorService, | ||
Optional<Customizer<io.github.resilience4j.circuitbreaker.CircuitBreaker>> circuitBreakerCustomizer, | ||
Resilience4jBulkheadProvider bulkheadProvider) { | ||
this( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you fix the formatting here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please have a look if the reformat suits the requirements.
15c4bbb
to
08e4d44
Compare
Hey @ryanjbaxter, Sorry for jumping in here, but we recently updated to Spring Boot 3.2 and Spring Cloud 2023.0.0, and suddenly had a test starting to fail. I was tracking down what might have caused, and came across this Pull Request. Very shortly we have a SpringBootTest loading some configuration classes that setup the Customizer and we also ImportAutoConfiguration of ReactiveResilience4JAutoConfiguration. Before this change, everything worked but now the test fails since no bean of Let me know if I was clear and if my questions are pertinent. Thanks in advance. |
@gonmmarques thanks! Yes I agree we should have |
Thanks @ryanjbaxter! |
No problem, there should be a snapshot build available now if you want to try it |
Provide and use the new configuration property
spring.cloud.circuitbreaker.resilience4j.disable-time-timiter
as intended by #174.Setting the property to
true
disables the time limiting feature for both (default and reactive) implementations.