search

spring-cloud / spring-cloud-circuitbreaker

Spring Cloud Circuit Breaker API and Implementations
Apache License 2.0
328 stars 109 forks source link

introduce disable-time-limiter property #176

Closed msiegemund closed 7 months ago

msiegemund commented 8 months ago

Provide and use the new configuration property spring.cloud.circuitbreaker.resilience4j.disable-time-timiter as intended by https://github.com/spring-cloud/spring-cloud-circuitbreaker/issues/174.

Setting the property to true disables the time limiting feature for both (default and reactive) implementations.

msiegemund commented 7 months ago

While intergrating the requested changes i noticed..

It seems like the spring-cloud-build parent is defining jdk17. The referenced/used resilience4j-timelimiter-2.0.2 contains an implementation (io.github.resilience4j.timelimiter.TimeLimiter#createdTimeoutExceptionWithName) which refers to the old javax annotation javax.annotation.Nullable. This annotation is currently not within the scope of the project and thus needs to be provided by some third party who wishes to use it. I did not investigate any further if this is some issue within my local setup, the resilience4j project, if maybe some dependecy is missing or if this is "built/works as designed".

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 io.github.resilience4j.timelimiter.internal.InMemoryTimeLimiterRegistry you'll stuck with the need of a defined timelimit.

msiegemund commented 7 months ago

It seems like the spring-cloud-build parent is defining jdk17. The referenced/used resilience4j-timelimiter-2.0.2 contains an implementation (io.github.resilience4j.timelimiter.TimeLimiter#createdTimeoutExceptionWithName) which refers to the old javax annotation javax.annotation.Nullable. This annotation is currently not within the scope of the project and thus needs to be provided by some third party who wishes to use it. I did not investigate any further if this is some issue within my local setup, the resilience4j project, if maybe some dependecy is missing or if this is "built/works as designed".

I took a look and it seems that this is the current state of resilience4j. Here is an issue, including search link for obsolete javax-package usage: https://github.com/resilience4j/resilience4j/issues/1978.

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 javax-dependency. On the other hand, one could consider providing it within the spring-cloud-circuitbreaker-project since it seems to be mandatory for certain resilience4j-functionalities. Nobody really likes runtime-class-not-found-errors which could be prevented because of some prior knowledge of a transitive library flaw like this. ;)

EDIT: I just learned that missing annotations do not cause a ClassNotFoundException at runtime because of the @Retention-definition within the JLS. This leaves it to be just an ugly piece of code i guess. The missing annotation does not jeopardize the linking, it is just not present at runtime.

gonmmarques commented 7 months ago

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 Resilience4JConfigurationProperties is available. Of course we fixed the tests by simply importing it, but my question is, shouldn't the ReactiveResilience4JAutoConfiguration, similar to some other autoconfiguration classes (like RedisAutoConfiguration or ActiveMQAutoConfiguration) then be responsible for importing the Properties? Or if Autoconfiguration here should also be conditional on the Resilience4JConfigurationProperties?

Let me know if I was clear and if my questions are pertinent.

Thanks in advance.

ryanjbaxter commented 6 months ago

@gonmmarques thanks! Yes I agree we should have EnableConfigurationProperties on ReactiveResilience4JAutoConfiguration. This should be fixed with my latest commit 27a9a4ec2e8b434980ab4589c30f544dfafe669f. A new snapshot build should be available within 24 hours if you would like to try 2023.0.1-SNAPSHOT.

gonmmarques commented 6 months ago

Thanks @ryanjbaxter!

ryanjbaxter commented 6 months ago

No problem, there should be a snapshot build available now if you want to try it