Skip to content
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

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

msiegemund
Copy link

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.

Copy link
Contributor

@ryanjbaxter ryanjbaxter left a 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?

@ryanjbaxter ryanjbaxter linked an issue Nov 14, 2023 that may be closed by this pull request
@msiegemund msiegemund changed the base branch from main to 3.0.x November 25, 2023 13:45
@msiegemund
Copy link
Author

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
Copy link
Author

msiegemund commented Nov 26, 2023

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: resilience4j/resilience4j#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.

Resilience4JConfigBuilder.Resilience4JCircuitBreakerConfiguration config,
CircuitBreakerRegistry circuitBreakerRegistry, TimeLimiterRegistry timeLimiterRegistry,
Optional<Customizer<CircuitBreaker>> circuitBreakerCustomizer) {
this(
Copy link
Contributor

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?

Copy link
Author

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(
Copy link
Contributor

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?

Copy link
Author

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.

@ryanjbaxter ryanjbaxter merged commit 5c7026a into spring-cloud:3.0.x Nov 30, 2023
1 check passed
@gonmmarques
Copy link

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
Copy link
Contributor

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

@gonmmarques
Copy link

Thanks @ryanjbaxter!

@ryanjbaxter
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TimeLimiter should be optional
4 participants