Thanks for the updated explanation, that certainly makes sense. On Mon, Jul 29, 2019 at 10:47 AM Jordan Zimmerman < jor...@jordanzimmerman.com> wrote:
> 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 <cammcken...@apache.org> > 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 < > jor...@jordanzimmerman.com <mailto:jor...@jordanzimmerman.com>> > > 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 > >
