Sure - makes sense Cameron..
When CURATOR-533 was written, each ConnectionStateListener created was mapped a
new CircuitBreaker instance. In hindsight, this doesn't make sense. Only a
single, shared CircuitBreaker is needed. Here's the email that the Elastic
engineer sent to me originally that started this:
"We enabled circuit breaker in production a month ago and everything has
been working
fine so far....
Also, we implemented a retry policy that adds some jitter: we specify
minimum and maximum
allowed interval and every retry it picks a random value within the allowed
range.
However, while we were testing the feature we found out that it works a
little bit differently
than we expected: it creates a circuit breaker per a connection state
listener. As long as we use
the retry policy with jitter, the fact that there are many circuit breakers
can potentially
introduce some races between components."
-Jordan
> On Jul 28, 2019, at 7:28 PM, Cameron McKenzie <[email protected]> wrote:
>
> hey Jordan,
> Can you expand on why the previous implementation was not ideal?
>
> Given that the decorator approach was only introduced recently, I don't
> imagine it has had a lot of uptake, but it may be worth asking the question
> on the curator-user list to work out if there's going to be any impact in
> removing the existing implementation?
> cheers
>
> On Mon, Jul 29, 2019 at 2:50 AM Jordan Zimmerman <[email protected]
> <mailto:[email protected]>>
> wrote:
>
>> Hi Committers,
>>
>> CURATOR-505 introduced circuit breaking behavior via
>> CircuitBreakingConnectionStateListener and
>> ConnectionStateListenerDecorator. Elastic has been using it with success
>> but reports that the implementation can be improved. The existing
>> implementation uses a new CircuitBreaker for each ConnectionStateListener
>> set in a Curator client. It turns out that this is not ideal. Instead, a
>> shared CircuitBreaker should be used per Curator client.
>>
>> Unfortunately, the best way to do this is to remove the
>> ConnectionStateListenerDecorator semantics and use a different mechanism.
>> This Issue proposes to do this and remove ConnectionStateListenerDecorator.
>> This is a breaking change but given the short amount of time it's been in
>> Curator it's unlikely that it's been widely adopted.
>> If the community considers a breaking change too harsh the older classes
>> can be maintained for a while and marked as @Deprecated. Otherwise we can
>> make the next release 4.3.0 (note: our semantic versioning has always been
>> wrong - different issue) to denote a breaking change.
>>
>> I have a candidate PR here: https://github.com/apache/curator/pull/320 <
>> https://github.com/apache/curator/pull/320
>> <https://github.com/apache/curator/pull/320>> - the Jira issue is:
>> https://issues.apache.org/jira/browse/CURATOR-533
>> <https://issues.apache.org/jira/browse/CURATOR-533> <
>> https://issues.apache.org/jira/browse/CURATOR-533
>> <https://issues.apache.org/jira/browse/CURATOR-533>>
>>
>> -Jordan
