Denovo1998 commented on code in PR #25278:
URL: https://github.com/apache/pulsar/pull/25278#discussion_r2878215799
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/Backoff.java:
##########
@@ -19,108 +19,194 @@
package org.apache.pulsar.common.util;
import java.time.Clock;
+import java.time.Duration;
+import java.time.Instant;
import java.util.Random;
-import java.util.concurrent.TimeUnit;
-import lombok.Data;
+import lombok.Getter;
-// All variables are in TimeUnit millis by default
-@Data
+/**
+ * Exponential backoff with mandatory stop.
+ *
+ * <p>Delays start at {@code initialDelay} and double on every call to {@link
#next()}, up to
+ * {@code maxBackoff}. A random jitter of up to 10% is subtracted from each
value to avoid
+ * thundering-herd retries.
+ *
+ * <p>If a {@code mandatoryStop} duration is configured, the backoff tracks
wall-clock time from the
+ * first {@link #next()} call. Once the elapsed time plus the next delay would
exceed the mandatory
+ * stop, the delay is truncated so that the total does not exceed it, and
{@link #isMandatoryStopMade()}
+ * returns {@code true}. After the mandatory stop, backoff continues to grow
normally.
+ *
+ * <p>Use {@link #reset()} to restart the sequence from the initial delay.
+ *
+ * <pre>{@code
+ * Backoff backoff = Backoff.builder()
+ * .initialDelay(Duration.ofMillis(100))
+ * .maxBackoff(Duration.ofMinutes(1))
+ * .mandatoryStop(Duration.ofSeconds(30))
+ * .build();
+ *
+ * Duration delay = backoff.next();
+ * }</pre>
+ */
public class Backoff {
- public static final long DEFAULT_INTERVAL_IN_NANOSECONDS =
TimeUnit.MILLISECONDS.toNanos(100);
- public static final long MAX_BACKOFF_INTERVAL_NANOSECONDS =
TimeUnit.SECONDS.toNanos(30);
- private final long initial;
- private final long max;
- private final Clock clock;
- private long next;
- private long mandatoryStop;
+ private static final Duration DEFAULT_INITIAL_DELAY =
Duration.ofMillis(100);
+ private static final Duration DEFAULT_MAX_BACKOFF_INTERVAL =
Duration.ofSeconds(30);
+ private static final Random random = new Random();
- private long firstBackoffTimeInMillis;
- private boolean mandatoryStopMade = false;
+ @Getter
+ private final Duration initial;
+ @Getter
+ private final Duration max;
+ @Getter
+ private final Duration mandatoryStop;
+ private final Clock clock;
- private static final Random random = new Random();
+ private Duration next;
+ @Getter
+ private Instant firstBackoffTime;
+ @Getter
+ private boolean mandatoryStopMade;
- Backoff(long initial, TimeUnit unitInitial, long max, TimeUnit unitMax,
long mandatoryStop,
- TimeUnit unitMandatoryStop, Clock clock) {
- this.initial = unitInitial.toMillis(initial);
- this.max = unitMax.toMillis(max);
- if (initial == 0 && max == 0 && mandatoryStop == 0) {
+ private Backoff(Duration initial, Duration max, Duration mandatoryStop,
Clock clock) {
+ this.initial = initial;
+ this.max = max;
+ this.mandatoryStop = mandatoryStop;
+ this.next = initial;
+ this.clock = clock;
+ this.firstBackoffTime = Instant.EPOCH;
+ if (initial.isZero() && max.isZero() && mandatoryStop.isZero()) {
this.mandatoryStopMade = true;
}
- this.next = this.initial;
- this.mandatoryStop = unitMandatoryStop.toMillis(mandatoryStop);
- this.clock = clock;
- this.firstBackoffTimeInMillis = 0;
}
- public Backoff(long initial, TimeUnit unitInitial, long max, TimeUnit
unitMax, long mandatoryStop,
- TimeUnit unitMandatoryStop) {
- this(initial, unitInitial, max, unitMax, mandatoryStop,
unitMandatoryStop, Clock.systemDefaultZone());
+ /**
+ * Creates a new {@link Builder} with default settings.
+ *
+ * @return a new builder instance
+ */
+ public static Builder builder() {
+ return new Builder();
}
- public long next() {
- long current = this.next;
- if (current < max) {
- this.next = Math.min(this.next * 2, this.max);
+ /**
+ * Returns the next backoff delay, advancing the internal state.
+ *
+ * <p>The returned duration is never less than the initial delay and never
more than the max
+ * backoff. A random jitter of up to 10% is subtracted to spread out
concurrent retries.
+ *
+ * @return the delay to wait before the next retry attempt
+ */
+ public Duration next() {
+ Duration current = this.next;
+ if (current.compareTo(max) < 0) {
+ Duration doubled = this.next.multipliedBy(2);
+ this.next = doubled.compareTo(this.max) < 0 ? doubled : this.max;
}
// Check for mandatory stop
if (!mandatoryStopMade) {
- long now = clock.millis();
- long timeElapsedSinceFirstBackoff = 0;
- if (initial == current) {
- firstBackoffTimeInMillis = now;
+ Instant now = clock.instant();
+ Duration timeElapsedSinceFirstBackoff = Duration.ZERO;
+ if (initial.equals(current)) {
+ firstBackoffTime = now;
} else {
- timeElapsedSinceFirstBackoff = now - firstBackoffTimeInMillis;
+ timeElapsedSinceFirstBackoff =
Duration.between(firstBackoffTime, now);
}
Review Comment:
Here, as long as the returned delay (current) for this round equals initial,
firstBackoffTime will be refreshed to "now".
This implies an assumption: current == initial will only occur during the
"first next()". However, this assumption is likely invalid, as current ==
initial can occur in multiple scenarios.
---
1. **Assume building a constant backoff (initial = maximum):**
initialDelay = 100ms
maxBackoff = 100ms
mandatoryStop = 5s
So every time next():
- current = next = 100ms
- Since current.compareTo(max) < 0 is false (equal), next does not increase
and remains 100ms
- In the mandatoryStop logic, initial.equals(current) is always true
- Every time firstBackoffTime = now is executed
- elapsed is always 0 (or close to 0), so elapsed + current > mandatoryStop
is never true
- Result: Even if real time has passed for tens of seconds, mandatoryStop is
not triggered (mandatoryStopMade remains false)
2. **reduceToHalf() halves the next delay, but not below initial. This
produces an unintuitive side effect:**
- Assume initial=100ms, during a backoff process, next has increased to 200ms
- Call reduceToHalf(), it will change next back to 100ms (== initial)
- The next time next() is called, current will again equal initial
- So the code will execute firstBackoffTime = now (equivalent to "restarting
the timer")
But from the semantic perspective of the comments, reduceToHalf does not say
that the mandatoryStop timer window should be reset. Only reset() should reset
it.
Therefore, using initial.equals(current) as the "first call determination"
will cause: the firstBackoffTime might be reset even without calling reset,
thus making mandatoryStop more lenient (or even never reaching it).
---
Perhaps the judgment for the first next() should be:
- If firstBackoffTime == Instant.EPOCH, it means this period has not started
timing -> set it
- Otherwise, calculate elapsed using it
- When reset() is called, also reset firstBackoffTime back to Instant.EPOCH
(otherwise, the old start point will be used after reset)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
