Skip to content

Commit 9e6e559

Browse files
injectivesinjectives
authored
Update retry delay multiplier in ExponentialBackoffRetryLogic (#1622)
* Update retry delay multiplier in ExponentialBackoffRetryLogic This fixes a bug in ExponentialBackoffRetryLogic and ensures that the retry delay works as expected. * Update verifyAfterConstruction
1 parent 1e71209 commit 9e6e559

File tree

2 files changed

+37
-25
lines changed

2 files changed

+37
-25
lines changed

‎driver/src/main/java/org/neo4j/driver/internal/retry/ExponentialBackoffRetryLogic.java

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -45,14 +45,14 @@ public class ExponentialBackoffRetryLogic implements RetryLogic {
4545
public static final long DEFAULT_MAX_RETRY_TIME_MS = SECONDS.toMillis(30);
4646

4747
private static final long INITIAL_RETRY_DELAY_MS = SECONDS.toMillis(1);
48-
private static final double RETRY_DELAY_MULTIPLIER = 1;
48+
private static final double RETRY_DELAY_MULTIPLIER = 2.0;
4949
private static final double RETRY_DELAY_JITTER_FACTOR = 0.2;
5050
private static final long MAX_RETRY_DELAY = Long.MAX_VALUE / 2;
5151

5252
private final long maxRetryTimeMs;
53-
private final long initialRetryDelayMs;
54-
private final double multiplier;
55-
private final double jitterFactor;
53+
final long initialRetryDelayMs;
54+
final double multiplier;
55+
final double jitterFactor;
5656
private final EventExecutorGroup eventExecutorGroup;
5757
private final Clock clock;
5858
private final SleepTask sleepTask;
@@ -325,8 +325,8 @@ private void verifyAfterConstruction() {
325325
if (initialRetryDelayMs < 0) {
326326
throw new IllegalArgumentException("Initial retry delay should >= 0: " + initialRetryDelayMs);
327327
}
328-
if (multiplier < 1.0) {
329-
throw new IllegalArgumentException("Multiplier should be >= 1.0: " + multiplier);
328+
if (multiplier < 2.0) {
329+
throw new IllegalArgumentException("Multiplier should be >= 2.0: " + multiplier);
330330
}
331331
if (jitterFactor < 0 || jitterFactor > 1) {
332332
throw new IllegalArgumentException("Jitter factor should be in [0.0, 1.0]: " + jitterFactor);

‎driver/src/test/java/org/neo4j/driver/internal/retry/ExponentialBackoffRetryLogicTest.java

Lines changed: 31 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import static java.lang.Long.MAX_VALUE;
2020
import static java.util.concurrent.CompletableFuture.completedFuture;
2121
import static java.util.concurrent.CompletableFuture.failedFuture;
22+
import static java.util.concurrent.TimeUnit.SECONDS;
2223
import static org.hamcrest.MatcherAssert.assertThat;
2324
import static org.hamcrest.Matchers.allOf;
2425
import static org.hamcrest.Matchers.closeTo;
@@ -97,29 +98,40 @@ void throwsForIllegalInitialRetryDelay() {
9798
@Test
9899
void throwsForIllegalMultiplier() {
99100
var error = assertThrows(
100-
IllegalArgumentException.class, () -> newRetryLogic(1, 1, 0.42, 1, Clock.systemUTC(), (ignored) -> {}));
101+
IllegalArgumentException.class, () -> newRetryLogic(1, 1, 1.99, 1, Clock.systemUTC(), (ignored) -> {}));
101102
assertThat(error.getMessage(), containsString("Multiplier"));
102103
}
103104

104105
@Test
105106
void throwsForIllegalJitterFactor() {
106107
var error1 = assertThrows(
107108
IllegalArgumentException.class,
108-
() -> newRetryLogic(1, 1, 1, -0.42, Clock.systemUTC(), (ignored) -> {}));
109+
() -> newRetryLogic(1, 1, 2, -0.42, Clock.systemUTC(), (ignored) -> {}));
109110
assertThat(error1.getMessage(), containsString("Jitter"));
110111

111112
var error2 = assertThrows(
112-
IllegalArgumentException.class, () -> newRetryLogic(1, 1, 1, 1.42, Clock.systemUTC(), (ignored) -> {}));
113+
IllegalArgumentException.class, () -> newRetryLogic(1, 1, 2, 1.42, Clock.systemUTC(), (ignored) -> {}));
113114
assertThat(error2.getMessage(), containsString("Jitter"));
114115
}
115116

116117
@Test
117118
void throwsForIllegalClock() {
118119
var error =
119-
assertThrows(IllegalArgumentException.class, () -> newRetryLogic(1, 1, 1, 1, null, (ignored) -> {}));
120+
assertThrows(IllegalArgumentException.class, () -> newRetryLogic(1, 1, 2, 1, null, (ignored) -> {}));
120121
assertThat(error.getMessage(), containsString("Clock"));
121122
}
122123

124+
@Test
125+
void shouldInitialiseWithExpectedDefaultValues() {
126+
var clock = mock(Clock.class);
127+
var sleepTask = mock(ExponentialBackoffRetryLogic.SleepTask.class);
128+
var logic = new ExponentialBackoffRetryLogic(MAX_VALUE, eventExecutor, clock, DEV_NULL_LOGGING, sleepTask);
129+
130+
assertEquals(SECONDS.toMillis(1), logic.initialRetryDelayMs);
131+
assertEquals(2.0, logic.multiplier);
132+
assertEquals(0.2, logic.jitterFactor);
133+
}
134+
123135
@Test
124136
void nextDelayCalculatedAccordingToMultiplier() throws Exception {
125137
var retries = 27;
@@ -318,7 +330,7 @@ void doesNotRetryWhenMaxRetryTimeExceededRx() {
318330
void sleepsOnServiceUnavailableException() throws Exception {
319331
var clock = mock(Clock.class);
320332
var sleepTask = mock(ExponentialBackoffRetryLogic.SleepTask.class);
321-
var logic = newRetryLogic(1, 42, 1, 0, clock, sleepTask);
333+
var logic = newRetryLogic(1, 42, 2, 0, clock, sleepTask);
322334

323335
Supplier<Void> workMock = newWorkMock();
324336
var error = serviceUnavailable();
@@ -335,7 +347,7 @@ void schedulesRetryOnServiceUnavailableExceptionAsync() {
335347
var result = "The Result";
336348
var clock = mock(Clock.class);
337349

338-
var retryLogic = newRetryLogic(1, 42, 1, 0, clock, (ignored) -> {});
350+
var retryLogic = newRetryLogic(1, 42, 2, 0, clock, (ignored) -> {});
339351

340352
Supplier<CompletionStage<Object>> workMock = newWorkMock();
341353
var error = serviceUnavailable();
@@ -353,7 +365,7 @@ void schedulesRetryOnServiceUnavailableExceptionAsync() {
353365
void sleepsOnSessionExpiredException() throws Exception {
354366
var clock = mock(Clock.class);
355367
var sleepTask = mock(ExponentialBackoffRetryLogic.SleepTask.class);
356-
var logic = newRetryLogic(1, 4242, 1, 0, clock, sleepTask);
368+
var logic = newRetryLogic(1, 4242, 2, 0, clock, sleepTask);
357369

358370
Supplier<Void> workMock = newWorkMock();
359371
var error = sessionExpired();
@@ -370,7 +382,7 @@ void schedulesRetryOnSessionExpiredExceptionAsync() {
370382
var result = "The Result";
371383
var clock = mock(Clock.class);
372384

373-
var retryLogic = newRetryLogic(1, 4242, 1, 0, clock, (ignored) -> {});
385+
var retryLogic = newRetryLogic(1, 4242, 2, 0, clock, (ignored) -> {});
374386

375387
Supplier<CompletionStage<Object>> workMock = newWorkMock();
376388
var error = sessionExpired();
@@ -388,7 +400,7 @@ void schedulesRetryOnSessionExpiredExceptionAsync() {
388400
void sleepsOnTransientException() throws Exception {
389401
var clock = mock(Clock.class);
390402
var sleepTask = mock(ExponentialBackoffRetryLogic.SleepTask.class);
391-
var logic = newRetryLogic(1, 23, 1, 0, clock, sleepTask);
403+
var logic = newRetryLogic(1, 23, 2, 0, clock, sleepTask);
392404

393405
Supplier<Void> workMock = newWorkMock();
394406
var error = transientException();
@@ -405,7 +417,7 @@ void schedulesRetryOnTransientExceptionAsync() {
405417
var result = "The Result";
406418
var clock = mock(Clock.class);
407419

408-
var retryLogic = newRetryLogic(1, 23, 1, 0, clock, (ignored) -> {});
420+
var retryLogic = newRetryLogic(1, 23, 2, 0, clock, (ignored) -> {});
409421

410422
Supplier<CompletionStage<Object>> workMock = newWorkMock();
411423
var error = transientException();
@@ -423,7 +435,7 @@ void schedulesRetryOnTransientExceptionAsync() {
423435
void throwsWhenUnknownError() throws Exception {
424436
var clock = mock(Clock.class);
425437
var sleepTask = mock(ExponentialBackoffRetryLogic.SleepTask.class);
426-
var logic = newRetryLogic(1, 1, 1, 1, clock, sleepTask);
438+
var logic = newRetryLogic(1, 1, 2, 1, clock, sleepTask);
427439

428440
Supplier<Void> workMock = newWorkMock();
429441
var error = new IllegalStateException();
@@ -439,7 +451,7 @@ void throwsWhenUnknownError() throws Exception {
439451
@Test
440452
void doesNotRetryOnUnknownErrorAsync() {
441453
var clock = mock(Clock.class);
442-
var retryLogic = newRetryLogic(1, 1, 1, 1, clock, (ignored) -> {});
454+
var retryLogic = newRetryLogic(1, 1, 2, 1, clock, (ignored) -> {});
443455

444456
Supplier<CompletionStage<Object>> workMock = newWorkMock();
445457
var error = new IllegalStateException();
@@ -456,7 +468,7 @@ void doesNotRetryOnUnknownErrorAsync() {
456468
void throwsWhenTransactionTerminatedError() throws Exception {
457469
var clock = mock(Clock.class);
458470
var sleepTask = mock(ExponentialBackoffRetryLogic.SleepTask.class);
459-
var logic = newRetryLogic(1, 13, 1, 0, clock, sleepTask);
471+
var logic = newRetryLogic(1, 13, 2, 0, clock, sleepTask);
460472

461473
Supplier<Void> workMock = newWorkMock();
462474
var error = new ClientException("Neo.ClientError.Transaction.Terminated", "");
@@ -472,7 +484,7 @@ void throwsWhenTransactionTerminatedError() throws Exception {
472484
@Test
473485
void doesNotRetryOnTransactionTerminatedErrorAsync() {
474486
var clock = mock(Clock.class);
475-
var retryLogic = newRetryLogic(1, 13, 1, 0, clock, (ignored) -> {});
487+
var retryLogic = newRetryLogic(1, 13, 2, 0, clock, (ignored) -> {});
476488

477489
Supplier<CompletionStage<Object>> workMock = newWorkMock();
478490
var error = new ClientException("Neo.ClientError.Transaction.Terminated", "");
@@ -489,7 +501,7 @@ void doesNotRetryOnTransactionTerminatedErrorAsync() {
489501
void throwsWhenTransactionLockClientStoppedError() throws Exception {
490502
var clock = mock(Clock.class);
491503
var sleepTask = mock(ExponentialBackoffRetryLogic.SleepTask.class);
492-
var logic = newRetryLogic(1, 13, 1, 0, clock, sleepTask);
504+
var logic = newRetryLogic(1, 13, 2, 0, clock, sleepTask);
493505

494506
Supplier<Void> workMock = newWorkMock();
495507
var error = new ClientException("Neo.ClientError.Transaction.LockClientStopped", "");
@@ -505,7 +517,7 @@ void throwsWhenTransactionLockClientStoppedError() throws Exception {
505517
@Test
506518
void doesNotRetryOnTransactionLockClientStoppedErrorAsync() {
507519
var clock = mock(Clock.class);
508-
var retryLogic = newRetryLogic(1, 15, 1, 0, clock, (ignored) -> {});
520+
var retryLogic = newRetryLogic(1, 15, 2, 0, clock, (ignored) -> {});
509521

510522
Supplier<CompletionStage<Object>> workMock = newWorkMock();
511523
var error = new ClientException("Neo.ClientError.Transaction.LockClientStopped", "");
@@ -523,7 +535,7 @@ void doesNotRetryOnTransactionLockClientStoppedErrorAsync() {
523535
void schedulesRetryOnErrorRx(Exception error) {
524536
var result = "The Result";
525537
var clock = mock(Clock.class);
526-
var retryLogic = newRetryLogic(1, 4242, 1, 0, clock, (ignored) -> {});
538+
var retryLogic = newRetryLogic(1, 4242, 2, 0, clock, (ignored) -> {});
527539

528540
Publisher<String> publisher = createMono(result, error);
529541
var single = Flux.from(retryLogic.retryRx(publisher)).single();
@@ -539,7 +551,7 @@ void schedulesRetryOnErrorRx(Exception error) {
539551
@MethodSource("cannotBeRetriedErrors")
540552
void scheduleNoRetryOnErrorRx(Exception error) {
541553
var clock = mock(Clock.class);
542-
var retryLogic = newRetryLogic(1, 10, 1, 1, clock, (ignored) -> {});
554+
var retryLogic = newRetryLogic(1, 10, 2, 1, clock, (ignored) -> {});
543555

544556
var single = Flux.from(retryLogic.retryRx(Mono.error(error))).single();
545557

@@ -555,7 +567,7 @@ void throwsWhenSleepInterrupted() throws Exception {
555567
var clock = mock(Clock.class);
556568
var sleepTask = mock(ExponentialBackoffRetryLogic.SleepTask.class);
557569
doThrow(new InterruptedException()).when(sleepTask).sleep(1);
558-
var logic = newRetryLogic(1, 1, 1, 0, clock, sleepTask);
570+
var logic = newRetryLogic(1, 1, 2, 0, clock, sleepTask);
559571

560572
Supplier<Void> workMock = newWorkMock();
561573
when(workMock.get()).thenThrow(serviceUnavailable());

0 commit comments

Comments
 (0)