spring-cloud / spring-cloud-circuitbreaker

Spring Cloud Circuit Breaker API and Implementations
Apache License 2.0
332 stars 111 forks source link

Using CircuitBreaker in Spring Cloud Gateway overrides route timeout properties #69

Closed mpc92 closed 3 years ago

mpc92 commented 4 years ago

I am using Spring Cloud Gateway, based on Hoxton.SR3.

When I use the metadata timeout settings to manage my connections and then add a circuit breaker filter, the timeout value set in metadata is overridden by the default timeout use in the resilience4j TimeLimiter (1s).

Here's my route config:

        - id: test_route
          uri: http://localhost:8992
          predicates:
            - Host=testsvc
          filters:
            - CircuitBreaker=pocCircuitBreaker
          metadata:
            response-timeout: 2000

My test service simple sleeps for 2500ms, so I expect a timeout after 2s. Instead the request times out after only 1s.

I have figured out a workaround by setting the TimeLimiter to a ridiculously large value, in which case the route response-timeout triggers first, but I don't know if that will be reliable as I will need to support several different circuit breaker/timeout configurations.

    @Bean
    public Customizer<ReactiveResilience4JCircuitBreakerFactory> defaultCustomizer() {
        return factory -> factory.configureDefault(id -> new Resilience4JConfigBuilder(id)
            .circuitBreakerConfig(CircuitBreakerConfig.custom()
                .slidingWindow(50, 50, CircuitBreakerConfig.SlidingWindowType.COUNT_BASED)
                .build())
            .timeLimiterConfig(TimeLimiterConfig.custom()
                .timeoutDuration(Duration.ofMinutes(5L)) 
                .cancelRunningFuture(false)
                .build())
            .build());
    }

Is there any way to completely suppress the TimeLimiter so I don't have to worry about this? I've tried omitting it from the above Customizer, but then the 1s default kicked in.

eatgrass commented 4 years ago

I have the same question. why we need a timelimiter in circuitbreaker, since we have already had httpclient.x-timeout for connection , and slowCallRateThreshold for circuit break.

spencergibb commented 4 years ago

wouldn't you want the circuit breaker timeout to be the source of truth at that point? /cc @ryanjbaxter

mpc92 commented 4 years ago

No. I have several endpoints that go to the same service, each with different timeout requirements. They all fall under the same circuit breaker because that is configured for the service.

ryanjbaxter commented 3 years ago

@mpc92 you can configure a timeout for a specific circuit breaker as opposed to all circuit breakers https://docs.spring.io/spring-cloud-circuitbreaker/docs/1.0.4.RELEASE/reference/html/#specific-circuit-breaker-configuration

spring-cloud-issues commented 3 years ago

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

spring-cloud-issues commented 3 years ago

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

dalegaspi commented 3 years ago

@ryanjbaxter, @spencergibb i have a similar dilemma and posted the details here so what is the way to configure the circuit breaker in application.yml to override the (global?) 1s that's in the timelimiter?

ryanjbaxter commented 3 years ago

If you are trying to configure R4J via properties see this comment https://github.com/spring-cloud/spring-cloud-circuitbreaker/issues/61#issuecomment-633654395

dalegaspi commented 3 years ago

@ryanjbaxter yes. and i tried that, it got close...but not to where i need it to be (i just left a comment in that thread); i think there shouldn't be a need to write additional code to override anything...i should be able to define overrides purely on config alone...but that's just me.

edit: this solution is actually better and gets to almost where i want (somewhat completely config driven)...but like i mentioned, this code should be the default behavior rather than having to write this custom Bean for it.

ryanjbaxter commented 3 years ago

Lets keep the conversation about this over in #61.