diff --git a/analytics/src/main/java/com/segment/analytics/Analytics.java b/analytics/src/main/java/com/segment/analytics/Analytics.java index c87e0948..ff52adb8 100644 --- a/analytics/src/main/java/com/segment/analytics/Analytics.java +++ b/analytics/src/main/java/com/segment/analytics/Analytics.java @@ -224,10 +224,10 @@ public Builder flushInterval(long flushInterval, TimeUnit unit) { return this; } - /** Set the interval at which the queue should be flushed. */ + /** Set how many retries should happen before getting exhausted */ public Builder retries(int maximumRetries) { - if (maximumFlushAttempts < 0) { - throw new IllegalArgumentException("retries must be greater than 0"); + if (maximumRetries < 1) { + throw new IllegalArgumentException("retries must be at least 1"); } this.maximumFlushAttempts = maximumRetries; return this; diff --git a/analytics/src/main/java/com/segment/analytics/internal/AnalyticsClient.java b/analytics/src/main/java/com/segment/analytics/internal/AnalyticsClient.java index f65c1361..e8e76a2d 100644 --- a/analytics/src/main/java/com/segment/analytics/internal/AnalyticsClient.java +++ b/analytics/src/main/java/com/segment/analytics/internal/AnalyticsClient.java @@ -269,7 +269,8 @@ boolean upload() { @Override public void run() { - for (int attempt = 0; attempt < maximumFlushAttempts; attempt++) { + int attempt = 0; + for (; attempt <= maximumFlushAttempts; attempt++) { boolean retry = upload(); if (!retry) return; try { @@ -282,7 +283,7 @@ public void run() { } client.log.print(ERROR, "Could not upload batch %s. Retries exhausted.", batch.sequence()); - notifyCallbacksWithException(batch, new IOException(maximumFlushAttempts + " retries exhausted")); + notifyCallbacksWithException(batch, new IOException(Integer.toString(attempt) + " retries exhausted")); } private static boolean is5xx(int status) { diff --git a/analytics/src/test/java/com/segment/analytics/AnalyticsBuilderTest.java b/analytics/src/test/java/com/segment/analytics/AnalyticsBuilderTest.java index b8c6890c..23182d08 100644 --- a/analytics/src/test/java/com/segment/analytics/AnalyticsBuilderTest.java +++ b/analytics/src/test/java/com/segment/analytics/AnalyticsBuilderTest.java @@ -95,6 +95,23 @@ public void nullLog() { } } + @Test + public void negativeRetryCount() { + try { + builder.retries(0); + fail("Should fail for retries less than 1"); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessage("retries must be at least 1"); + } + + try { + builder.retries(-1); + fail("Should fail for retries less than 1"); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessage("retries must be at least 1"); + } + } + @Test public void nullTransformer() { try { diff --git a/analytics/src/test/java/com/segment/analytics/internal/AnalyticsClientTest.java b/analytics/src/test/java/com/segment/analytics/internal/AnalyticsClientTest.java index 017e86df..0b95e890 100644 --- a/analytics/src/test/java/com/segment/analytics/internal/AnalyticsClientTest.java +++ b/analytics/src/test/java/com/segment/analytics/internal/AnalyticsClientTest.java @@ -333,12 +333,38 @@ public Call answer(InvocationOnMock invocation) { batchUploadTask.run(); // 10 == maximumFlushAttempts - // tries only 10 even though default is 50 in AnalyticsClient.java - verify(segmentService, times(10)).upload(batch); + // tries 11(one normal run + 10 retries) even though default is 50 in AnalyticsClient.java + verify(segmentService, times(11)).upload(batch); verify(callback).failure(eq(trackMessage), argThat(new ArgumentMatcher() { @Override public boolean matches(IOException exception) { - return exception.getMessage().equals("10 retries exhausted"); + return exception.getMessage().equals("11 retries exhausted"); + } + })); + } + + @Test + public void neverRetries() { + AnalyticsClient client = newClient(); + TrackMessage trackMessage = TrackMessage.builder("foo").userId("bar").build(); + Batch batch = batchFor(trackMessage); + + when(segmentService.upload(batch)).thenAnswer(new Answer>() { + public Call answer(InvocationOnMock invocation) { + Response failResponse = Response.error(429, ResponseBody.create(null, "Not Found")); + return Calls.response(failResponse); + } + }); + + BatchUploadTask batchUploadTask = new BatchUploadTask(client, BACKO, batch, 0); + batchUploadTask.run(); + + // runs once but never retries + verify(segmentService, times(1)).upload(batch); + verify(callback).failure(eq(trackMessage), argThat(new ArgumentMatcher() { + @Override + public boolean matches(IOException exception) { + return exception.getMessage().equals("1 retries exhausted"); } })); }