ZOOKEEPER-2849: Exponential back-off retry for Quorum port binding #419
Conversation
Added BackoffStrategy interface with ExponentialBackoffStrategy as a concrete implementation. Enhanced QuorumCnxManager to utilize the provided BackoffStrategy to determine retry of port binding as opposed to a fixed interval/count.
Added missing Apache license header
| @@ -946,7 +946,8 @@ protected Election createElectionAlgorithm(int electionAlgorithm){ | |||
| le = new AuthFastLeaderElection(this, true); | |||
| break; | |||
| case 3: | |||
| qcm = new QuorumCnxManager(this); | |||
| qcm = new QuorumCnxManager(this, | |||
| ExponentialBackoffStrategy.builder().build()); | |||
There was a problem hiding this comment.
Perhaps we should find a way to make this pluggable? Perhaps we can read which backoff strategy to use from a java system property?
There was a problem hiding this comment.
For instance, it may be nice to have the default strategy perform the existing behavior.
There was a problem hiding this comment.
Agreed, included in my upcoming push.
| /** | ||
| * Builder for instances of {@link ExponentialBackoffStrategy}. | ||
| */ | ||
| public static final class Builder { |
There was a problem hiding this comment.
I'm not sure a builder is the best way to handle this. Since I think it would be nice to have much of this be user configurable, perhaps we can just pull the config from system properties?
There was a problem hiding this comment.
I agree that it should be configurable, but I don't think using a Builder pattern precludes this. I'm about to push changes that adds support for using system properties, that's a good idea.
There was a problem hiding this comment.
So I probably wasn't clear here. It would be great if it was also possible, since backoff strategy is such a nicely defined interface, if the user could include jar with their own backoff strategy in the classpath and tell zookeeper to use that. What do you think?
| /** | ||
| * Unit tests for {@link ExponentialBackoffStrategy}. | ||
| */ | ||
| public class ExponentialBackoffStrategyTest { |
There was a problem hiding this comment.
This should extend ZKTestCase
Unit tests extend ZKTestCase
Added support for FixedIntervalBackoffStrategy to maintain the current retry behavior. BackoffStrategy is now configurable via system properties, user can select the type of BackoffStrategy as well as configure it as desired.
Add FixedIntervalBackoffStrategy and unit test that got missed.
Add support for external implementations of BackoffStrategy.
|
|
||
| private void validateInclusiveBetween(long min, long max, long value) { | ||
| if(value < min) { | ||
| throw new IllegalArgumentException(); |
There was a problem hiding this comment.
It would be useful to add some information about the parameter with illegal value. Exception message would be fine for that if you could add it as a parameter of this method.
Same applies to all places where you throw IllegalArgumentException.
| * @return a new {@link Builder} instance. | ||
| */ | ||
| public static Builder builder() { | ||
| return new Builder(); |
There was a problem hiding this comment.
I wonder why do you need a method for creating Builder instance.
| try { | ||
| builder.setBackoffMultiplier(Double.valueOf(multiplierProp)); | ||
| } catch(NumberFormatException nfe) { | ||
| LOG.warn("{} is not a valid floating point value, ignoring it.", multiplierProp); |
There was a problem hiding this comment.
I suggest to add:
...floating point value for backoffMultiplier, ignoring...
Added BackoffStrategy interface with ExponentialBackoffStrategy as a concrete implementation. Enhanced
QuorumCnxManager to utilize the provided BackoffStrategy to determine retry of port binding as opposed to
a fixed interval/count.