Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retryable with exponential backoff not working with delayExpression #399

Merged
merged 13 commits into from
Nov 15, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ static class ExponentialBackOffContext implements BackOffContext {

private final long maxInterval;

private Supplier<Long> intervalSupplier;
private final Supplier<Long> initialIntervalSupplier;

private Supplier<Double> multiplierSupplier;

Expand All @@ -271,7 +271,7 @@ public ExponentialBackOffContext(long interval, double multiplier, long maxInter
this.interval = interval;
this.multiplier = multiplier;
this.maxInterval = maxInterval;
this.intervalSupplier = intervalSupplier;
this.initialIntervalSupplier = intervalSupplier;
this.multiplierSupplier = multiplierSupplier;
this.maxIntervalSupplier = maxIntervalSupplier;
}
Expand All @@ -289,15 +289,27 @@ public synchronized long getSleepAndIncrement() {
}

protected long getNextInterval() {
return (long) (this.interval * getMultiplier());
if (initialIntervalSupplier == null) {
return (long) (this.interval * getMultiplier());
}
else {
return this.interval == DEFAULT_INITIAL_INTERVAL
? (long) (this.initialIntervalSupplier.get() * getMultiplier())
: (long) (this.interval * getMultiplier());
}
}

public double getMultiplier() {
return this.multiplierSupplier != null ? this.multiplierSupplier.get() : this.multiplier;
}

public long getInterval() {
return this.intervalSupplier != null ? this.intervalSupplier.get() : this.interval;
if (initialIntervalSupplier == null) {
R-Zer0 marked this conversation as resolved.
Show resolved Hide resolved
return this.interval;
}
else {
return this.interval == DEFAULT_INITIAL_INTERVAL ? this.initialIntervalSupplier.get() : this.interval;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to call intervalSupplier every time?
Looks like with your change we do that only once.

Copy link
Contributor Author

@R-Zer0 R-Zer0 Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only first time when we need initial value, further iteration store multiplayed value in the interval variable here

}
}

public long getMaxInterval() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,22 @@ public void testMultiBackOff() {
}
}

@Test
public void testMultiBackOffWithInitialDelaySupplier() {
ExponentialBackOffPolicy strategy = new ExponentialBackOffPolicy();
long seed = 40;
double multiplier = 1.2;
strategy.initialIntervalSupplier(() -> 40L);
strategy.setMultiplier(multiplier);
strategy.setSleeper(sleeper);
BackOffContext context = strategy.start(null);
for (int x = 0; x < 5; x++) {
strategy.backOff(context);
assertThat(sleeper.getLastBackOff()).isEqualTo(seed);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to test that we still have exponential back offs (which we no longer have with the current change).

Copy link
Contributor Author

@R-Zer0 R-Zer0 Nov 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you suggest any example here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually current test checking following, validated in debug.

Last backoff Seed
40 40
48 48
57 57
68 68
81 81

I was thinking it is sufficient for current issue.

seed *= multiplier;
}
}

@Test
public void testInterruptedStatusIsRestored() {
ExponentialBackOffPolicy strategy = new ExponentialBackOffPolicy();
Expand Down