You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by Jordan Zimmerman <jo...@jordanzimmerman.com> on 2019/07/28 16:50:23 UTC
[DISCUSS] CURATOR-533
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> - the Jira issue is: https://issues.apache.org/jira/browse/CURATOR-533 <https://issues.apache.org/jira/browse/CURATOR-533>
-Jordan
Re: [DISCUSS] CURATOR-533
Posted by Cameron McKenzie <mc...@gmail.com>.
Thanks for the updated explanation, that certainly makes sense.
On Mon, Jul 29, 2019 at 10:47 AM Jordan Zimmerman <
jordan@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 <ca...@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 <
> jordan@jordanzimmerman.com <ma...@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
>
>
Re: [DISCUSS] CURATOR-533
Posted by Cameron McKenzie <mc...@gmail.com>.
Thanks for the updated explanation, that certainly makes sense.
On Mon, Jul 29, 2019 at 10:47 AM Jordan Zimmerman <
jordan@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 <ca...@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 <
> jordan@jordanzimmerman.com <ma...@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
>
>
Re: [DISCUSS] CURATOR-533
Posted by Jordan Zimmerman <jo...@jordanzimmerman.com>.
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 <ca...@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 <jordan@jordanzimmerman.com <ma...@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
Re: [DISCUSS] CURATOR-533
Posted by Jordan Zimmerman <jo...@jordanzimmerman.com>.
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 <ca...@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 <jordan@jordanzimmerman.com <ma...@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
Re: [DISCUSS] CURATOR-533
Posted by Cameron McKenzie <ca...@apache.org>.
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 <jo...@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> - the Jira issue is:
> https://issues.apache.org/jira/browse/CURATOR-533 <
> https://issues.apache.org/jira/browse/CURATOR-533>
>
> -Jordan