search

danielfm / pybreaker

Python implementation of the Circuit Breaker pattern.
BSD 3-Clause "New" or "Revised" License
528 stars 73 forks source link

Get initial CircuitBreaker state from state_storage #27

Closed alukach closed 7 years ago

alukach commented 7 years ago

The PR intends to resolve danielfm/pybreak#26 by altering the CircuitBreaker to make no assumptions about an initialized CircuitBreaker state. Instead, on first call of breaker.state, the state is retrieved from the state_storage. This allows pybreaker to play nicely in environments where it is desirable share CircuitBreaker state between machines.

jcwilson commented 7 years ago

Thanks for jumping in on this one!

alukach commented 7 years ago

@jcwilson Thanks for the helpful input, I agree on almost all comments except for the optimization comment (I will change it if you twist my arm, but I prefer it as is).

jcwilson commented 7 years ago

@jcwilson Thanks for the helpful input, I agree on almost all comments except for the optimization comment (I will change it if you twist my arm, but I prefer it as is).

Nope, no arm twisting - just offering a perspective. :)

alukach commented 7 years ago

Thinking about this, I think some things aren't right with my current implementation. Giving it a once over and then will re-open.

alukach commented 7 years ago

Questions:

  1. How should notifications be handled in a multi-instance environment? For example, consider that Machine A and Machine B are both running the same codebase with a breaker set up for Service X and sharing state via Redis. When Machine A notices that Service X is down, it opens the breaker and sends a notification to our listeners. When Machine B tries to contact Service X, it looks at Redis and sees that the breaker is set to 'open'. Machine B now sets its cached state value to 'open', should it send a notification to the listeners? Basically, should we notify listeners once globally when the _state_storage changes or should we notify listeners once-per-environment?
  2. Additionally, if Machine B gets disconnected from Redis, then it will use its fallback state. Should that also send a notification?

I believe the answers are 1) Send notifications once-per-environment, 2) Send notification on fallback states as well. Basically, notifications will inform when a particular instance notices a circuit being closed due to a down service, rather than when the service is first noticed to be down. The only exception to this is when the CircuitBreaker is being initialized, in which case it does not notify of a new state.

alukach commented 7 years ago

Okay, code should be a bit better now. I realized that I was incorrectly sending the new state to the state classes (e.g. CircuitOpenState) rather than the previous state. While in there, I was getting a bit confused about having both the CircuitBreaker and CircuitBreakerState hold state about the status of the breaker. I think I've simplified it quite a bit, leaving the logic more-or-less the same but making it clearer what the roles of CircuitBreaker.state was.

alukach commented 7 years ago

@jcwilson @danielfm So any final thoughts on this? What are the odds that this could be merged in and release? I have a feature that depends on this change so I'm hoping we can get it released soon.

jcwilson commented 7 years ago

1) Send notifications once-per-environment

If this is the same as once per process, then I concur. Users of the library can perform their own deduplication or specialized handling in their Listener classes.

2) Send notification on fallback states as well.

I think this is fine. I'd rather know that my app has changed states, regardless of why.

danielfm commented 7 years ago

Thanks for your contribution, I just uploaded the 0.4.1 release to PyPI.