spring-cloud / spring-cloud-circuitbreaker

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

Creating circuit breakers in advance #52

Closed georgeharley closed 4 years ago

georgeharley commented 4 years ago

Hi,

All of the demo code and documentation examples favour creating circuit breakers every time the protected method is called. That feels like it might not be efficient and earlier this year there seemed to be an admission that it is not the best approach.

Creating circuit breakers in advance seems to work fine but I've found that trying to configure them using Customizer beans has no effect. That appears to be because Customizer beans update the circuit breaker factory in auto-configuration code that runs after the circuit breaker factory has been used to create circuit breakers in the constructors of other components. The only way I've found to configure circuit breakers created in advance of any method calls is to provide a custom CircuitBreakerFactory bean that includes the desired customisations. Here's an example where the circuit breaker factory's defaults are configured along with some custom set-up for a particular circuit breaker named "myCircuitBreaker" that is created elsewhere (in the constructor of the bean that uses it):


    @Bean
    public CircuitBreakerFactory circuitBreakerFactory() {
        Resilience4JCircuitBreakerFactory factory = new Resilience4JCircuitBreakerFactory();

        factory.configureDefault(id -> new Resilience4JConfigBuilder(id)
                .circuitBreakerConfig(
                        CircuitBreakerConfig.custom()
                                .failureRateThreshold(50)
                                .waitDurationInOpenState(ofMillis(10000))
                                .slidingWindow(20, 20, COUNT_BASED)
                                .build())
                .timeLimiterConfig(
                        TimeLimiterConfig.custom()
                                .timeoutDuration(ofMillis(5000))
                                .build())
                .build());

        factory.configure(configBuilder -> configBuilder
                        .circuitBreakerConfig(
                                CircuitBreakerConfig.custom()
                                        .failureRateThreshold(33)
                                        .waitDurationInOpenState(ofMillis(5000))
                                        .slidingWindow(10, 10, COUNT_BASED)
                                        .build())
                        .timeLimiterConfig(
                                TimeLimiterConfig.custom()
                                        .timeoutDuration(ofMillis(2000))
                                        .build())
                        .build(),
                "myCircuitBreaker");

        return factory;
    }

Are there any guidelines or recommendations about whether it is better to create circuit breakers inside every method call or in advance? If creation in advance is supported then is it a bug that Customizer beans appears to have no effect on circuit breakers unless they are created in every method call - or is it just an omission in the documentation and examples?

ryanjbaxter commented 4 years ago

The simplest way I can think of doing this is basically delay creation until the first time it is used. For example.

@RestController
public class DemoController {

    Logger LOG = LoggerFactory.getLogger(DemoController.class);

    private HttpBinService httpBin;
    private CircuitBreakerFactory circuitBreakerFactory;
    private CircuitBreaker cb;

    public DemoController(CircuitBreakerFactory circuitBreakerFactory, HttpBinService httpBinService) {
        this.circuitBreakerFactory = circuitBreakerFactory;
        this.httpBin = httpBinService;
    }

    @GetMapping("/get")
    public Map get() {
        return httpBin.get();
    }

    @GetMapping("/delay/{seconds}")
    public Map delay(@PathVariable int seconds) {
        if(cb == null) {
            cb = circuitBreakerFactory.create("delay");
        }
        return cb.run(httpBin.delaySuppplier(seconds), t -> {
            LOG.warn("delay call failed error", t);
            Map<String, String> fallback = new HashMap<>();
            fallback.put("hello", "world");
            return fallback;
        });
    }
}

Let me know what you think of that. I will also see if I can come up with a more elegant solution.

ebussieres commented 4 years ago

I made a pull request to fix this (#56)

ryanjbaxter commented 4 years ago

@ebussieres thanks! We also need to make similar changes for the other implementations as well

ebussieres commented 4 years ago

@ryanjbaxter Just made a commit to the PR #56 to apply the fix for spring-retry

ryanjbaxter commented 4 years ago

@ebussieres thanks if you want to take a stab at reworking the Hystrix implementation in Spring Cloud Netflix, that would be appreciated as well

ebussieres commented 4 years ago

Sure I'll take a look

ryanjbaxter commented 4 years ago

If you do create a PR in spring-cloud-netflix be sure to link it here

ebussieres commented 4 years ago

@ryanjbaxter Just made the PR to spring-cloud-netflix (https://github.com/spring-cloud/spring-cloud-netflix/pull/3722)

ryanjbaxter commented 4 years ago

Great thanks I will take a look at this once I get back from vacation on January 6th