From a8a14fd5122dbbcae50a2412e8b9b40df0c211f3 Mon Sep 17 00:00:00 2001 From: Petr Gladkikh Date: Tue, 28 Nov 2017 12:59:58 +0100 Subject: [PATCH 1/6] Cleanup: remove time unit from ExponentialRetry Default to milliseconds internally. The behavior would be incorrect with any other time unit anyway: due to multiple unit.toMillis(unit.toMillis(unit.toMillis(x))) conversions. Extract duplicating code in unit test. --- .../main/java/nakadi/ExponentialRetry.java | 9 +-- .../java/nakadi/ExponentialRetryTest.java | 75 ++++++++----------- 2 files changed, 35 insertions(+), 49 deletions(-) diff --git a/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java b/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java index 57b03847..4e0656e7 100644 --- a/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java +++ b/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java @@ -18,7 +18,6 @@ public class ExponentialRetry implements RetryPolicy { long workingAttempts = 1; long maxTime; long workingTime = 0L; - TimeUnit unit; private long workingInterval; private volatile long startTime = 0L; private float percentOfMaxIntervalForJitter; @@ -28,7 +27,6 @@ public class ExponentialRetry implements RetryPolicy { this.maxInterval = builder.maxInterval; this.workingInterval = initialInterval; this.maxAttempts = builder.maxAttempts; - this.unit = builder.unit; this.maxTime = builder.maxTime; this.percentOfMaxIntervalForJitter = builder.percentOfMaxIntervalForJitter; } @@ -61,11 +59,11 @@ public long nextBackoffMillis() { return STOP; } - workingInterval = unit.toMillis(workingInterval) * (workingAttempts * workingAttempts); + workingInterval = workingInterval * (workingAttempts * workingAttempts); workingAttempts++; if (workingInterval <= 0) { - workingInterval = unit.toMillis(maxInterval); + workingInterval = maxInterval; } if (initialInterval != workingInterval) { @@ -100,13 +98,10 @@ public int maxAttempts() { ", maxInterval=" + maxInterval + ", maxAttempts=" + maxAttempts + ", workingAttempts=" + workingAttempts + - ", unit=" + unit + '}'; } public static class Builder { - - private final TimeUnit unit = TimeUnit.MILLISECONDS; public float percentOfMaxIntervalForJitter = PERCENT_OF_MAX_INTERVAL_AS_JITTER; private long initialInterval = DEFAULT_INITIAL_INTERVAL_MILLIS; private long maxInterval = DEFAULT_MAX_INTERVAL_MILLIS; diff --git a/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java b/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java index 46ebc174..aaa6bcc7 100644 --- a/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java +++ b/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java @@ -8,8 +8,7 @@ public class ExponentialRetryTest { @Test - public void tempusFugit() throws Exception { - + public void tempusFugit_1() throws Exception { ExponentialRetry exponentialRetry = ExponentialRetry.newBuilder() .initialInterval(11, TimeUnit.MILLISECONDS) .maxAttempts(Integer.MAX_VALUE) @@ -17,72 +16,64 @@ public void tempusFugit() throws Exception { .percentOfMaxIntervalForJitter(20) .maxTime(3, TimeUnit.SECONDS) .build(); + runRetries(exponentialRetry); + validateTimeoutState(exponentialRetry); + } - while(true) { - long l = exponentialRetry.nextBackoffMillis(); - if(l == -1) { - break; - } - Thread.sleep(l); - } - - assertTrue(exponentialRetry.workingTime >= exponentialRetry.maxTime); - assertTrue(exponentialRetry.workingAttempts < exponentialRetry.maxAttempts); - - exponentialRetry = ExponentialRetry.newBuilder() + @Test + public void tempusFugit_2() throws Exception { + ExponentialRetry exponentialRetry = ExponentialRetry.newBuilder() .maxTime(3, TimeUnit.SECONDS) .maxInterval(100, TimeUnit.MILLISECONDS) .build(); + runRetries(exponentialRetry); + validateTimeoutState(exponentialRetry); + } - while(true) { - long l = exponentialRetry.nextBackoffMillis(); - if(l == -1) { - break; - } - Thread.sleep(l); - } - + private void validateTimeoutState(ExponentialRetry exponentialRetry) { assertTrue(exponentialRetry.workingTime >= exponentialRetry.maxTime); assertTrue(exponentialRetry.workingAttempts < exponentialRetry.maxAttempts); } @Test - public void annumero() throws Exception { - + public void annumero_1() throws Exception { ExponentialRetry exponentialRetry = ExponentialRetry.newBuilder() .initialInterval(101, TimeUnit.MILLISECONDS) .maxAttempts(20) .maxInterval(100, TimeUnit.MILLISECONDS) .maxTime(Integer.MAX_VALUE, TimeUnit.SECONDS) .build(); + runRetries(exponentialRetry); + validateRetriesExceededState(exponentialRetry); + } - while(true) { - long l = exponentialRetry.nextBackoffMillis(); - if(l == -1) { - break; - } - Thread.sleep(l); - } + @Test + public void annumero_2() throws Exception { + ExponentialRetry exponentialRetry = ExponentialRetry.newBuilder() + .maxAttempts(3) + .maxInterval(100, TimeUnit.MILLISECONDS) + .build(); + runRetries(exponentialRetry); + validateRetriesExceededState(exponentialRetry); + } + private void validateRetriesExceededState(ExponentialRetry exponentialRetry) { assertTrue(exponentialRetry.workingTime < exponentialRetry.maxTime); assertTrue(exponentialRetry.workingAttempts >= exponentialRetry.maxAttempts); + } - - exponentialRetry = ExponentialRetry.newBuilder() - .maxAttempts(3) - .maxInterval(100, TimeUnit.MILLISECONDS) - .build(); - + private void runRetries(ExponentialRetry exponentialRetry) throws InterruptedException { while(true) { long l = exponentialRetry.nextBackoffMillis(); - if(l == -1) { + if(l == RetryPolicy.STOP) { break; } + // This does not hold: l >= exponentialRetry.initialInterval() + assertTrue(l <= exponentialRetry.maxIntervalMillis()); + Thread.sleep(l); } - - assertTrue(exponentialRetry.workingTime < exponentialRetry.maxTime); - assertTrue(exponentialRetry.workingAttempts >= exponentialRetry.maxAttempts); + assertTrue(exponentialRetry.isFinished()); } -} \ No newline at end of file +} From 818aa4a095318642a40bd0689ba24c452d69a2c2 Mon Sep 17 00:00:00 2001 From: Petr Gladkikh Date: Tue, 28 Nov 2017 13:02:27 +0100 Subject: [PATCH 2/6] Cleanup: streamline validation NakadiException.throwNonNull throws IllegalStateException while NakadiException is itself exception, also throwNonNull is a non null check that is retrofitted to throw unconditionally. This all is rather confusing. --- nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java b/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java index 4e0656e7..9a3acf86 100644 --- a/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java +++ b/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java @@ -115,7 +115,7 @@ public Builder initialInterval(long initialInterval, TimeUnit unit) { NakadiException.throwNonNull(unit, "Please provide a TimeUnit"); this.initialInterval = unit.toMillis(initialInterval); if (this.initialInterval < INITIAL_INTERVAL_MIN_AS_MILLIS) { - NakadiException.throwNonNull(null, "Please provide an initial value of at least " + throw new IllegalArgumentException("Please provide an initial value of at least " + INITIAL_INTERVAL_MIN_AS_MILLIS + " millis"); } @@ -126,7 +126,7 @@ public Builder maxInterval(long maxInterval, TimeUnit unit) { NakadiException.throwNonNull(unit, "Please provide a TimeUnit"); this.maxInterval = unit.toMillis(maxInterval); if (this.maxInterval < MAX_INTERVAL_MIN_AS_MILLIS) { - NakadiException.throwNonNull(null, "Please provide a max interval value of at least " + throw new IllegalArgumentException("Please provide a max interval value of at least " + MAX_INTERVAL_MIN_AS_MILLIS + " millis"); } From d890edd77e86f45443baf7d9cb8bb2bfe37f1f13 Mon Sep 17 00:00:00 2001 From: Petr Gladkikh Date: Tue, 28 Nov 2017 13:03:22 +0100 Subject: [PATCH 3/6] Test that demonstrates incorrect working time calculation in retry --- .../src/main/java/nakadi/ExponentialRetry.java | 9 ++++++--- .../test/java/nakadi/ExponentialRetryTest.java | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java b/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java index 9a3acf86..f8659096 100644 --- a/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java +++ b/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java @@ -48,11 +48,14 @@ public boolean isFinished() { } public long nextBackoffMillis() { + return nextBackOffMillis(System.currentTimeMillis()); + } - if (startTime == 0L) { - startTime = System.currentTimeMillis(); + long nextBackOffMillis(long nowMillis) { + if (this.startTime == 0L) { + this.startTime = nowMillis; } else { - workingTime += (System.currentTimeMillis() - startTime); + workingTime += (nowMillis - this.startTime); } if (isFinished()) { diff --git a/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java b/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java index aaa6bcc7..843ad19a 100644 --- a/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java +++ b/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java @@ -57,6 +57,21 @@ public void annumero_2() throws Exception { validateRetriesExceededState(exponentialRetry); } + @Test + public void workingTimeCalculation() { + ExponentialRetry exponentialRetry = ExponentialRetry.newBuilder() + .maxAttempts(1000) + .maxInterval(100, TimeUnit.MILLISECONDS) + .maxTime(110, TimeUnit.MILLISECONDS) + .build(); + exponentialRetry.nextBackOffMillis(1); + exponentialRetry.nextBackOffMillis(100); + exponentialRetry.nextBackOffMillis(101); + assertFalse(exponentialRetry.isFinished()); + exponentialRetry.nextBackOffMillis(110); + assertTrue(exponentialRetry.isFinished()); + } + private void validateRetriesExceededState(ExponentialRetry exponentialRetry) { assertTrue(exponentialRetry.workingTime < exponentialRetry.maxTime); assertTrue(exponentialRetry.workingAttempts >= exponentialRetry.maxAttempts); From 171184c1cbe0da36dcd7bb5d38ee5319ebdede78 Mon Sep 17 00:00:00 2001 From: Petr Gladkikh Date: Tue, 28 Nov 2017 13:13:53 +0100 Subject: [PATCH 4/6] Correct workingTime calculation --- .../src/main/java/nakadi/ExponentialRetry.java | 11 +++++------ .../src/test/java/nakadi/ExponentialRetryTest.java | 7 +++---- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java b/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java index f8659096..ed68fe04 100644 --- a/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java +++ b/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java @@ -17,7 +17,7 @@ public class ExponentialRetry implements RetryPolicy { int maxAttempts; long workingAttempts = 1; long maxTime; - long workingTime = 0L; + long lastBackoff = 0L; private long workingInterval; private volatile long startTime = 0L; private float percentOfMaxIntervalForJitter; @@ -44,7 +44,7 @@ public long maxIntervalMillis() { } public boolean isFinished() { - return workingAttempts >= maxAttempts || workingTime >= maxTime; + return workingAttempts >= maxAttempts || (lastBackoff - startTime) >= maxTime; } public long nextBackoffMillis() { @@ -52,11 +52,10 @@ public long nextBackoffMillis() { } long nextBackOffMillis(long nowMillis) { - if (this.startTime == 0L) { - this.startTime = nowMillis; - } else { - workingTime += (nowMillis - this.startTime); + if (startTime == 0L) { + startTime = nowMillis; } + lastBackoff = nowMillis; if (isFinished()) { return STOP; diff --git a/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java b/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java index 843ad19a..6e1103b5 100644 --- a/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java +++ b/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java @@ -31,7 +31,7 @@ public void tempusFugit_2() throws Exception { } private void validateTimeoutState(ExponentialRetry exponentialRetry) { - assertTrue(exponentialRetry.workingTime >= exponentialRetry.maxTime); + assertTrue(exponentialRetry.lastBackoff >= exponentialRetry.maxTime); assertTrue(exponentialRetry.workingAttempts < exponentialRetry.maxAttempts); } @@ -60,7 +60,6 @@ public void annumero_2() throws Exception { @Test public void workingTimeCalculation() { ExponentialRetry exponentialRetry = ExponentialRetry.newBuilder() - .maxAttempts(1000) .maxInterval(100, TimeUnit.MILLISECONDS) .maxTime(110, TimeUnit.MILLISECONDS) .build(); @@ -68,12 +67,12 @@ public void workingTimeCalculation() { exponentialRetry.nextBackOffMillis(100); exponentialRetry.nextBackOffMillis(101); assertFalse(exponentialRetry.isFinished()); - exponentialRetry.nextBackOffMillis(110); + exponentialRetry.nextBackOffMillis(111); assertTrue(exponentialRetry.isFinished()); } private void validateRetriesExceededState(ExponentialRetry exponentialRetry) { - assertTrue(exponentialRetry.workingTime < exponentialRetry.maxTime); + assertTrue(exponentialRetry.lastBackoff < exponentialRetry.maxTime); assertTrue(exponentialRetry.workingAttempts >= exponentialRetry.maxAttempts); } From 19c77c18e549d8196cad3006a31297f9b36ff889 Mon Sep 17 00:00:00 2001 From: Petr Gladkikh Date: Tue, 28 Nov 2017 13:16:08 +0100 Subject: [PATCH 5/6] Stricter test assertions --- .../src/test/java/nakadi/ExponentialRetryTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java b/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java index 6e1103b5..cf10c9b0 100644 --- a/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java +++ b/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java @@ -64,7 +64,9 @@ public void workingTimeCalculation() { .maxTime(110, TimeUnit.MILLISECONDS) .build(); exponentialRetry.nextBackOffMillis(1); + assertFalse(exponentialRetry.isFinished()); exponentialRetry.nextBackOffMillis(100); + assertFalse(exponentialRetry.isFinished()); exponentialRetry.nextBackOffMillis(101); assertFalse(exponentialRetry.isFinished()); exponentialRetry.nextBackOffMillis(111); From c066f797bea9b30c18a8578962d94faac7cbd234 Mon Sep 17 00:00:00 2001 From: Petr Gladkikh Date: Tue, 28 Nov 2017 13:30:10 +0100 Subject: [PATCH 6/6] Correct timeout state check --- .../src/main/java/nakadi/ExponentialRetry.java | 8 ++++++-- .../src/test/java/nakadi/ExponentialRetryTest.java | 4 ++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java b/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java index ed68fe04..b4e900e6 100644 --- a/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java +++ b/nakadi-java-client/src/main/java/nakadi/ExponentialRetry.java @@ -17,7 +17,7 @@ public class ExponentialRetry implements RetryPolicy { int maxAttempts; long workingAttempts = 1; long maxTime; - long lastBackoff = 0L; + private long lastBackoff = 0L; private long workingInterval; private volatile long startTime = 0L; private float percentOfMaxIntervalForJitter; @@ -43,8 +43,12 @@ public long maxIntervalMillis() { return maxInterval; } + long workingTime() { + return lastBackoff - startTime; + } + public boolean isFinished() { - return workingAttempts >= maxAttempts || (lastBackoff - startTime) >= maxTime; + return workingAttempts >= maxAttempts || workingTime() >= maxTime; } public long nextBackoffMillis() { diff --git a/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java b/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java index cf10c9b0..0f808b77 100644 --- a/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java +++ b/nakadi-java-client/src/test/java/nakadi/ExponentialRetryTest.java @@ -31,7 +31,7 @@ public void tempusFugit_2() throws Exception { } private void validateTimeoutState(ExponentialRetry exponentialRetry) { - assertTrue(exponentialRetry.lastBackoff >= exponentialRetry.maxTime); + assertTrue(exponentialRetry.workingTime() >= exponentialRetry.maxTime); assertTrue(exponentialRetry.workingAttempts < exponentialRetry.maxAttempts); } @@ -74,7 +74,7 @@ public void workingTimeCalculation() { } private void validateRetriesExceededState(ExponentialRetry exponentialRetry) { - assertTrue(exponentialRetry.lastBackoff < exponentialRetry.maxTime); + assertTrue(exponentialRetry.workingTime() < exponentialRetry.maxTime); assertTrue(exponentialRetry.workingAttempts >= exponentialRetry.maxAttempts); }