search

spring-cloud / spring-cloud-circuitbreaker

Spring Cloud Circuit Breaker API and Implementations
Apache License 2.0
335 stars 110 forks source link

update - separate ConditionalOnProperty for blocking and reactive #63

Closed srinivasa-vasu closed 3 years ago

srinivasa-vasu commented 4 years ago
codecov[bot] commented 4 years ago

Codecov Report

Merging #63 into master will increase coverage by 0.83%. The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #63      +/-   ##
============================================
+ Coverage     88.61%   89.44%   +0.83%     
- Complexity       52       53       +1     
============================================
  Files            11       11              
  Lines           202      218      +16     
  Branches          4        4              
============================================
+ Hits            179      195      +16     
  Misses           18       18              
  Partials          5        5
Impacted Files Coverage Δ Complexity Δ
...ience4j/ReactiveResilience4JAutoConfiguration.java 90.90% <ø> (+0.90%) 3.00 <0.00> (ø)
...er/resilience4j/Resilience4JAutoConfiguration.java 90.90% <ø> (+0.90%) 3.00 <0.00> (ø)
...tbreaker/springretry/SpringRetryConfigBuilder.java 76.19% <0.00%> (ø) 3.00% <0.00%> (ø%)
...breaker/springretry/SpringRetryCircuitBreaker.java 100.00% <0.00%> (ø) 4.00% <0.00%> (+1.00%)
...reaker/resilience4j/Resilience4JConfigBuilder.java 95.83% <0.00%> (ø) 4.00% <0.00%> (ø%)
...eaker/resilience4j/Resilience4JCircuitBreaker.java 100.00% <0.00%> (ø) 4.00% <0.00%> (ø%)
...e4j/ReactiveResilience4JCircuitBreakerFactory.java 90.47% <0.00%> (+0.47%) 8.00% <0.00%> (ø%)
...silience4j/ReactiveResilience4JCircuitBreaker.java 93.75% <0.00%> (+0.89%) 7.00% <0.00%> (ø%)
...esilience4j/Resilience4JCircuitBreakerFactory.java 84.00% <0.00%> (+1.39%) 8.00% <0.00%> (ø%)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f99e680...10e14b8. Read the comment docs.

ryanjbaxter commented 4 years ago

I am not necessarily opposed to this, but I am wondering if there is a problem with not having 2 separate properties?

srinivasa-vasu commented 4 years ago

Having separate properties control the bean initialization part. In v1.0.0. I hit a null pointer exception in the blocking flow circuit-breaker registry initialization while I was using a reactive context. Though it's already fixed in 1.0.2, this sort of gives the flexibility to control.

ryanjbaxter commented 4 years ago

Though it's already fixed in 1.0.2

If it is fixed, what is the point of this change?

srinivasa-vasu commented 4 years ago

Blocking bean creation is not needed from a reactive context if I don't need it, right? This way I've a way to control the initialization in a reactive context and vice versa.

ryanjbaxter commented 4 years ago

OK.

I assume this would be more useful in the 1.0.x branch as a release of master will be a ways off.

If we merge this against the 1.0.x branch you will need to keep the existing property name, maybe add an additional reactive one. Then we can remove the old one when I merge it forward in master.

We would also need documentation for the new properties.