Add builder for Backoff #5488
Add builder for Backoff #5488
Conversation
| return new Builder(); | ||
| } | ||
|
|
||
| public static class Builder { |
There was a problem hiding this comment.
Code style) We prefer top-level builder classes instead of nested classes.
Could you move this class to ExpontionalBackoffBuilder?
| public static Builder builder() { | ||
| return new Builder(); | ||
| } |
There was a problem hiding this comment.
Instead of adding a builder method to each implementation, should we add a builder method Backoff interface?
interface Backoff {
static ExpontionalBackoffBuilder builderForExpontional() {
...
}
}Note that ExponentialBackoff is a package-private class that users can't access.
| return new ExponentialBackoff(initialDelayMillis, maxDelayMillis, multiplier); | ||
| } | ||
|
|
||
| public Builder initialDelayMillis(long initialDelayMillis) { |
There was a problem hiding this comment.
Should we make jitter values configured with this builder?
| private long maxDelayMillis; | ||
|
|
||
| FibonacciBackoff build() { | ||
| return new FibonacciBackoff(initialDelayMillis, maxDelayMillis); |
There was a problem hiding this comment.
Should we migrate all factory methods where constructors are used into builder methods? e.g.
kth496
force-pushed
the
add-builder-for-backoff
branch
from
35255b9 to
6ed81ab
Compare
|
@ikhoon Thank you for your review. I will fix it within a week. |
|
@kth496 Looking forward to seeing more commits in this PR 💟 |
|
@kth496 Any updates? |
|
@ikhoon @trustin
Question: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5488 +/- ##
===========================================
+ Coverage 0 74.69% +74.69%
- Complexity 0 21721 +21721
===========================================
Files 0 1901 +1901
Lines 0 80424 +80424
Branches 0 10553 +10553
===========================================
+ Hits 0 60069 +60069
- Misses 0 15320 +15320
- Partials 0 5035 +5035 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
core/src/main/java/com/linecorp/armeria/client/retry/ExponentialBackoffBuilder.java
Show resolved
Hide resolved
| * | ||
| * @see ExponentialBackoff | ||
| */ | ||
| public class ExponentialBackoffBuilder { |
There was a problem hiding this comment.
Global comments:
- Add
@UnstableApito all new public classes. - Add
finalkeyword for builder classes.
| public class ExponentialBackoffBuilder { | |
| @UnstableApi | |
| public final class ExponentialBackoffBuilder { |
core/src/main/java/com/linecorp/armeria/client/retry/ExponentialBackoffBuilder.java
Show resolved
Hide resolved
| * | ||
| * @return a newly created {@link ExponentialBackoff} with the configured delays and multiplier | ||
| */ | ||
| ExponentialBackoff build() { |
There was a problem hiding this comment.
This must be public
| ExponentialBackoff build() { | |
| public ExponentialBackoff build() { |
| public class ExponentialBackoffBuilder { | ||
| private long initialDelayMillis; | ||
| private long maxDelayMillis; | ||
| private double multiplier; |
There was a problem hiding this comment.
Should we follow the default values used in the factory methods?
| private double multiplier; | |
| private double multiplier = 2.0; |
| * | ||
| * @return a newly created {@link ExponentialBackoff} with the configured delays and multiplier | ||
| */ | ||
| public Backoff build() { |
There was a problem hiding this comment.
Code style) We conventionally place build() method on the bottom of a builder method. Would you move it below multiplier()?
| } | ||
|
|
||
| /** | ||
| * Sets the maximum delay in milliseconds for the {@link FibonacciBackoff}. |
There was a problem hiding this comment.
FibonacciBackoff is an internal class to which we can't link in Javadoc.
| * Sets the maximum delay in milliseconds for the {@link FibonacciBackoff}. | |
| * Sets the maximum delay in milliseconds for the Fibonacci {@link Backoff}. |
| * | ||
| * @return a newly created {@link FixedBackoff} with the configured delay | ||
| */ | ||
| public Backoff build() { |
There was a problem hiding this comment.
Ditto. Please move to the bottom of this class.
| } | ||
|
|
||
| /** | ||
| * Sets the delay duration in milliseconds for the {@link FixedBackoff}. |
There was a problem hiding this comment.
| * Sets the delay duration in milliseconds for the {@link FixedBackoff}. | |
| * Sets the delay duration in milliseconds for the fixed {@link Backoff}. |
| * | ||
| * <pre> | ||
| * {@code | ||
| * FixedBackoff backoff = new FixedBackoffBuilder() |
There was a problem hiding this comment.
The constructor of FixedBackoffBuilder is now hidden.
| * FixedBackoff backoff = new FixedBackoffBuilder() | |
| * FixedBackoff backoff = Backoff.builderForFixed() |
| * | ||
| * @return a newly created {@link FibonacciBackoff} with the configured delays |
There was a problem hiding this comment.
It seems redundant.
| * | |
| * @return a newly created {@link FibonacciBackoff} with the configured delays |
| * | ||
| * <pre> | ||
| * {@code | ||
| * FibonacciBackoff backoff = new FibonacciBackoffBuilder() |
There was a problem hiding this comment.
Ditto. Please use builder methods.
| * @see ExponentialBackoff | ||
| */ | ||
| @UnstableApi | ||
| public final class ExponentialBackoffBuilder { |
There was a problem hiding this comment.
It would be worth allowing to configure jitter and max attempts in all builder methods.
What do you think of introducing AbstractBackoffBuilder to set them?
public abstract class AbstractBackoffBuilder<SELF extends AbstractBackoffBuilder<SELF>> {
SELF jitter(double minJitterRate, double maxJitterRate, Supplier<Random> randomSupplier) {
...
}
SELF maxAttempts(...) {
...
}
}
public final class ExponentialBackoffBuilder
extends AbstractBackoffBuilder<ExponentialBackoffBuilder>{
...
}
| abstract Backoff doBuild(); | ||
|
|
||
| /** | ||
| * Builds and returns {@link Backoff} instance with configured properties. | ||
| */ | ||
| public final Backoff build() { | ||
| Backoff backoff = doBuild(); | ||
| if (maxJitterRate > minJitterRate) { | ||
| backoff = new JitterAddingBackoff(backoff, minJitterRate, maxJitterRate, randomSupplier); | ||
| } | ||
| if (maxAttempts > 0) { | ||
| backoff = new AttemptLimitingBackoff(backoff, maxAttempts); | ||
| } | ||
| return backoff; | ||
| } |
There was a problem hiding this comment.
After much thought, I came up with the above solution. Is there a better way?
| /** | ||
| * Sets the minimum and maximum jitter rates to apply to the delay. | ||
| */ | ||
| SELF jitter(double minJitterRate, double maxJitterRate) { | ||
| this.minJitterRate = minJitterRate; | ||
| this.maxJitterRate = maxJitterRate; | ||
| return self(); | ||
| } | ||
|
|
||
| /** | ||
| * Sets the minimum and maximum jitter rates to apply to the delay, as well as a | ||
| * custom {@link Random} supplier for generating the jitter. | ||
| */ | ||
| SELF jitter(double minJitterRate, double maxJitterRate, Supplier<Random> randomSupplier) { | ||
| requireNonNull(randomSupplier, "randomSupplier"); | ||
| this.minJitterRate = minJitterRate; | ||
| this.maxJitterRate = maxJitterRate; | ||
| this.randomSupplier = randomSupplier; | ||
| return self(); | ||
| } | ||
|
|
||
| /** | ||
| * Sets the maximum number of attempts | ||
| * . | ||
| */ | ||
| SELF maxAttempts(int maxAttempts) { | ||
| this.maxAttempts = maxAttempts; | ||
| return self(); | ||
| } |
There was a problem hiding this comment.
Question) Shouldn't these methods be public?
| if (maxJitterRate > minJitterRate) { | ||
| backoff = new JitterAddingBackoff(backoff, minJitterRate, maxJitterRate, randomSupplier); | ||
| } | ||
| if (maxAttempts > 0) { | ||
| backoff = new AttemptLimitingBackoff(backoff, maxAttempts); | ||
| } |
There was a problem hiding this comment.
Can we just check nullability to decide whether to apply a wrapping backoff?
e.g.
@Nullable
private Double minJitterRate;
...
if (minJitterRate != null && maxJitterRate != null) {
...
}| /** | ||
| * A skeletal builder implementation for {@link Backoff}. | ||
| */ | ||
| public abstract class AbstractBackoffBuilder<SELF extends AbstractBackoffBuilder<SELF>> { |
There was a problem hiding this comment.
Question) Does the abstract builder need to be public?
| public abstract class AbstractBackoffBuilder<SELF extends AbstractBackoffBuilder<SELF>> { | |
| abstract class AbstractBackoffBuilder<SELF extends AbstractBackoffBuilder<SELF>> { |
| private long initialDelayMillis; | ||
| private long maxDelayMillis; |
There was a problem hiding this comment.
Would you mind adding sensible default values for all the parameters in builders?
|
@minwoox |
| */ | ||
| @UnstableApi | ||
| public final class ExponentialBackoffBuilder extends AbstractBackoffBuilder<ExponentialBackoffBuilder> { | ||
| private long initialDelayMillis = 100; |
There was a problem hiding this comment.
Let's use 200:
| private long initialDelayMillis; | ||
| private long maxDelayMillis; |
There was a problem hiding this comment.
Could you also add the default value for FibonacciBackoffBuilder, FixedBackoffBuilder, and RandomBackoffBuilder?
Motivation:
Backoff#5483Modifications:
Builderclass for various backoff implementations.Result:
Backoff#5483