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

More Flexible Circuit-breaker Strategy #9

Open
benjchristensen opened this Issue Nov 27, 2012 · 15 comments

Comments

Projects
None yet
7 participants
@benjchristensen
Contributor

benjchristensen commented Nov 27, 2012

RIght now the circuit-breaker implementation is fixed.

Expose a CircuitBreakerStrategy to allow custom implementations that can be plugged in.

@lordofthejars

This comment has been minimized.

lordofthejars commented Aug 8, 2014

I think that this addition will be really useful for testing. Currently if you want to test what's happen when a circuit is tripped in a controlled way, the only solution I have found is to use Archaius and configure the forceOpen attribute to true or false depending of what scenario you want to test. But I think it will be closed to reality (not the reality itself of course) if we can implement our own circuitbreaker strategy by implementing HystricCircuitBreaker strategy, and setting to command (or using Archaius). Nowadays the HystrixCommand contructor that allows to do this is in default modifier so it means it cannot be touched (it is only used by Hystrix team for testing but not for the rest of us).

So we may wonder if we could do something to fix this (maybe changing scope of constructor with a big warning to people), maybe add a factory class in same package which does this job or provide a setter (I really don't like this approach).

WDYT?

@mattrjacobs mattrjacobs modified the milestones: 1.5, 1.3.x Dec 19, 2014

@mattrjacobs mattrjacobs modified the milestone: 1.5.x Mar 3, 2015

@mattrjacobs

This comment has been minimized.

Contributor

mattrjacobs commented Aug 3, 2015

This seems like a useful addition to me too. Updating milestone to 1.5.x

@mattrjacobs mattrjacobs added this to the 1.5.x milestone Aug 3, 2015

@mattrjacobs mattrjacobs changed the title from Circuit-breaker Strategy to More Flexible Circuit-breaker Strategy Aug 6, 2015

@deantoni

This comment has been minimized.

deantoni commented Nov 9, 2015

I am also looking for a way to customize the circuit breaker logic. Currently HystrixCircuitBreaker has a hardcoded implementation. I'm really looking forward to being able to customize how the circuit breaker works.

@HaloFour

This comment has been minimized.

HaloFour commented Dec 21, 2015

Would this also permit for a circuit breaker to be shared amongst multiple commands?

@mattrjacobs mattrjacobs removed this from the 1.5.x milestone Apr 11, 2016

@mcmadhan

This comment has been minimized.

mcmadhan commented Jun 30, 2016

Is there any plan to support this in the near future?

@mattrjacobs

This comment has been minimized.

Contributor

mattrjacobs commented Jul 6, 2016

This is still a nice-to-have, but we don't have a pressing need for it at the moment. Given that, no one is actively working on it. If you're interested in working on it, we can discuss how it might work.

@mcmadhan

This comment has been minimized.

mcmadhan commented Jul 19, 2016

Sure, I will be interested in working on this. Please let me know how this might work and how I can have the approach validated.

Also, I wanted to share my requirement and see if there are any alternatives other than introducing the custom circuit breaker strategy -

I want to have the circuit marked open on specific error codes (or specific HTTP status codes). If I forceOpen the circuit on such error codes, single requests are not allowed with current implementation of circuit breaker. So I wanted to inject a circuit breaker implementation that does the following:

  • isOpen() - can detect if the circuit is open using my custom logic (let's say based on a status from db or cache that were populated based on service error codes or http status codes, but not using the metrics and health count). A centralized status of circuit is primarily needed as I want to make the entire cluster aware of the dependent system's state so the circuit is open for all the instances of the app on the cluster(s). (As soon as one instance detects an error/status code, it updates db and something like a circuit state property, backed by archaius polled configuration that populates from DB source, distributes it to all instances)
  • allowRequest() - uses isOpen() as in the default implementation, but allows single requests even if the circuit is force closed. There are scenarios where a circuit cannot remain force closed beyond a time limit and wanted to have the single requests to go through at periodic intervals so it can markSuccess eventually when it succeeds
  • markSuccess() - Resets the custom circuit state in db
@mattrjacobs

This comment has been minimized.

Contributor

mattrjacobs commented Jul 22, 2016

@mcmadhan Great - would love to see this get done and happy to provide feedback.

The general constraints that exist today are:

  1. The HystrixCircuitBreaker interface: It has the 3 methods you mentioned above. If you need new/different methods, let's discuss that as early as possible to what we can do.
  2. Currently, the instance of HystrixCircuitBreaker is selected per-command key and then cached for all command instances with that key. This CircuitBreaker instance is selected in the AbstractCommand constructor and not explicitly passed in.

For your specific implementation, I think you are correct that adding custom circuit-breaker configuration is the way to go.

A note about your proposed circuit-breaker: We have explicitly decided that the implementation of circuit-breaker should not share state across instances. This introduces another service dependency and may itself see failures/latency. That's not to say that your solution will not work- just that you'll need to take care in its design.

Please let me know if I can help review code/ideas

@mcmadhan

This comment has been minimized.

mcmadhan commented Jul 29, 2016

@mattrjacobs: Thank you for the detailed comment. I understood the details of current implementation and constraints. One another constraint is, HystrixCommand and HystrixObservableCommand do not have a public constructor that allow injection of a custom circuit breaker (There is a constructor that takes everything but is with package level scope/access modifier).

So, here is what I am thinking to add to allow custom cb strategy:

Add a public constructor to allow injection of the following in both HystrixCommand and HystrixObservableCommand:

  1. HystrixCommandKey (to stick to CB per command behavior )
  2. Custom CB implementation and
  3. (optional) com.netflix.hystrix.HystrixCommand.Setter to pass other properties

I see everything else gets defaulted in AbstractCommand.

Please let me know your thoughts on this.

Other than this, I think it is best to have CB implementation generic and introduce an interface where the custom logic can be implemented and injected into the CB impl. Below is a sudo code that gives an overall idea, please review and share your thoughts.

public class SampleCircuitBreakerImpl implements HystrixCircuitBreaker {

  /* interface to be introduced with isHealthy() and markHealthy() methods that can have a custom impl to track circuit status*/
  private CircuitHealthProvider circuitHealthProvider;

  @Override
    public boolean allowRequest() {
    return !isOpen()  || allowSingleTest()
  }

  private boolean allowSingleTest() {
      // Similar to current impl in HystrixCircuitBreakerImpl
    }

  @Override
  public boolean isOpen() {
       boolean isHealthy = circuitHealthProvider.isHealthy();
       // TODO: use isHealthy to determine current status and update cached circuit status
       // TODO: Update last tested or opened timestamp
  }

  @Override
  public boolean markSuccess() {
       //  TODO: When a request turns good after being open for long
       circuitHealthProvider.markHealthy();
       //  TODO: Reset cached circuit status
  }
}

Thanks,
Madhan

@mattrjacobs

This comment has been minimized.

Contributor

mattrjacobs commented Aug 10, 2016

Both HystrixCommand and HystrixObservableCommand already permit a constructor with Setter. Setter also includes the HystrixCommandKey. So I'm not sure you need to add constructors with those arguments. Maybe you could provide examples of what API additions you're considering?

For the circuit breaker, I'm not convinced that passing it at constructor time is the best idea. That would put a lot of work onto the people writing Hystrix commands. In many other places in the system (concurrency strategy, metrics publisher, properties strategy, etc), the decision is made once per application and installed up-front in a plugin. Are you envisioning all commands in a system using the same circuit-breaker behavior or possibly each using an independent implementation?

@mattrjacobs

This comment has been minimized.

Contributor

mattrjacobs commented Aug 10, 2016

For the CircuitHealthProvider idea, this seems mostly like renaming the CircuitBreaker terminology into a new set of terms. What do you gain by introducing this class?

@jkubrynski

This comment has been minimized.

jkubrynski commented Jul 4, 2017

I see this issue is almost dead. If we agree that passing CB in the Setter or providing custom CircuitBreakerKey or resolution strategy is a good idea I can contribute is.

@jkubrynski

This comment has been minimized.

jkubrynski commented Jul 11, 2017

@mattrjacobs Any feedback?

@mattrjacobs

This comment has been minimized.

Contributor

mattrjacobs commented Jul 13, 2017

I think passing the circuit breaker in the Setter is the least disruptive change. Thanks for taking a look!

@jkubrynski

This comment has been minimized.

jkubrynski commented Jul 17, 2017

@mattrjacobs The problem with such approach is that it's good to be able to use the HystrixCircuitBreakerImpl. However, it uses metrics, that are initialized in the Command. It looks like the best way to solve this issue is simply pass the circuitBreakerKey, which by default is set to the HystrixCommandKey. It could be passed by the Setter

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