diff --git a/core/src/main/java/google/registry/reporting/icann/IcannReportingUploadAction.java b/core/src/main/java/google/registry/reporting/icann/IcannReportingUploadAction.java index 351299ffe..323fd0ef7 100644 --- a/core/src/main/java/google/registry/reporting/icann/IcannReportingUploadAction.java +++ b/core/src/main/java/google/registry/reporting/icann/IcannReportingUploadAction.java @@ -39,6 +39,7 @@ import google.registry.util.Retrier; import google.registry.util.SendEmailService; import java.io.IOException; import java.io.InputStream; +import java.util.regex.Pattern; import java.util.stream.Collectors; import javax.inject.Inject; import javax.mail.internet.InternetAddress; @@ -102,7 +103,7 @@ public final class IcannReportingUploadAction implements Runnable { final byte[] payload = readBytesFromGcs(gcsFilename); return icannReporter.send(payload, reportFilename); }, - IOException.class); + IcannReportingUploadAction::isUploadFailureRetryable); } catch (RuntimeException e) { logger.atWarning().withCause(e).log("Upload to %s failed.", gcsFilename); } @@ -115,6 +116,21 @@ public final class IcannReportingUploadAction implements Runnable { String.format("OK, attempted uploading %d reports", manifestedFiles.size())); } + /** Don't retry when reports are already uploaded or can't be uploaded. */ + private static final String ICANN_UPLOAD_PERMANENT_ERROR_MESSAGE = + "A report for that month already exists, the cut-off date already passed."; + + /** Don't retry when the IP address isn't whitelisted, as retries go through the same IP. */ + private static final Pattern ICANN_UPLOAD_WHITELIST_ERROR = + Pattern.compile("Your IP address .+ is not allowed to connect"); + + /** Predicate to retry uploads on IOException, so long as they aren't non-retryable errors. */ + private static boolean isUploadFailureRetryable(Throwable e) { + return (e instanceof IOException) + && !e.getMessage().contains(ICANN_UPLOAD_PERMANENT_ERROR_MESSAGE) + && !ICANN_UPLOAD_WHITELIST_ERROR.matcher(e.getMessage()).matches(); + } + private void emailUploadResults(ImmutableMap reportSummary) { String subject = String.format( "ICANN Monthly report upload summary: %d/%d succeeded", diff --git a/core/src/test/java/google/registry/reporting/icann/IcannReportingUploadActionTest.java b/core/src/test/java/google/registry/reporting/icann/IcannReportingUploadActionTest.java index f8927075f..aa1109d2d 100644 --- a/core/src/test/java/google/registry/reporting/icann/IcannReportingUploadActionTest.java +++ b/core/src/test/java/google/registry/reporting/icann/IcannReportingUploadActionTest.java @@ -154,6 +154,45 @@ public class IcannReportingUploadActionTest { new InternetAddress("recipient@example.com"), new InternetAddress("sender@example.com"))); } + + @Test + public void testFailure_quicklySkipsOverNonRetryableUploadException() throws Exception { + runTest_nonRetryableException( + new IOException( + "A report for that month already exists, the cut-off date already" + + " passed.")); + } + + @Test + public void testFailure_quicklySkipsOverIpWhitelistException() throws Exception { + runTest_nonRetryableException( + new IOException("Your IP address 25.147.130.158 is not allowed to connect")); + } + + private void runTest_nonRetryableException(Exception nonRetryableException) throws Exception { + IcannReportingUploadAction action = createAction(); + when(mockReporter.send(PAYLOAD_FAIL, "a-activity-201706.csv")) + .thenThrow(nonRetryableException) + .thenThrow( + new AssertionError( + "This should never be thrown because the previous exception isn't retryable")); + action.run(); + verify(mockReporter, times(1)).send(PAYLOAD_FAIL, "a-activity-201706.csv"); + verify(mockReporter).send(PAYLOAD_SUCCESS, "test-transactions-201706.csv"); + verifyNoMoreInteractions(mockReporter); + assertThat(((FakeResponse) action.response).getPayload()) + .isEqualTo("OK, attempted uploading 2 reports"); + verify(emailService) + .sendEmail( + EmailMessage.create( + "ICANN Monthly report upload summary: 1/2 succeeded", + "Report Filename - Upload status:\n" + + "test-transactions-201706.csv - SUCCESS\n" + + "a-activity-201706.csv - FAILURE", + new InternetAddress("recipient@example.com"), + new InternetAddress("sender@example.com"))); + } + @Test public void testFail_FileNotFound() throws Exception { IcannReportingUploadAction action = createAction(); diff --git a/util/src/main/java/google/registry/util/Retrier.java b/util/src/main/java/google/registry/util/Retrier.java index 62ada3474..76b520db4 100644 --- a/util/src/main/java/google/registry/util/Retrier.java +++ b/util/src/main/java/google/registry/util/Retrier.java @@ -70,32 +70,8 @@ public class Retrier implements Serializable { * * @return the value returned by the {@link Callable}. */ - private V callWithRetry( - Callable callable, - FailureReporter failureReporter, - Predicate isRetryable) { - int failures = 0; - while (true) { - try { - return callable.call(); - } catch (Throwable e) { - if (++failures == attempts || !isRetryable.test(e)) { - throwIfUnchecked(e); - throw new RuntimeException(e); - } - failureReporter.beforeRetry(e, failures, attempts); - try { - // Wait 100ms on the first attempt, doubling on each subsequent attempt. - sleeper.sleep(Duration.millis(pow(2, failures) * 100)); - } catch (InterruptedException e2) { - // Since we're not rethrowing InterruptedException, set the interrupt state on the thread - // so the next blocking operation will know to abort the thread. - Thread.currentThread().interrupt(); - throwIfUnchecked(e); - throw new RuntimeException(e); - } - } - } + public V callWithRetry(Callable callable, Predicate isRetryable) { + return callWithRetry(callable, LOGGING_FAILURE_REPORTER, isRetryable); } /** @@ -169,6 +145,34 @@ public class Retrier implements Serializable { callWithRetry(callable.asCallable(), failureReporter, retryableError, moreRetryableErrors); } + private V callWithRetry( + Callable callable, + FailureReporter failureReporter, + Predicate isRetryable) { + int failures = 0; + while (true) { + try { + return callable.call(); + } catch (Throwable e) { + if (++failures == attempts || !isRetryable.test(e)) { + throwIfUnchecked(e); + throw new RuntimeException(e); + } + failureReporter.beforeRetry(e, failures, attempts); + try { + // Wait 100ms on the first attempt, doubling on each subsequent attempt. + sleeper.sleep(Duration.millis(pow(2, failures) * 100)); + } catch (InterruptedException e2) { + // Since we're not rethrowing InterruptedException, set the interrupt state on the thread + // so the next blocking operation will know to abort the thread. + Thread.currentThread().interrupt(); + throwIfUnchecked(e); + throw new RuntimeException(e); + } + } + } + } + private static final FailureReporter LOGGING_FAILURE_REPORTER = (thrown, failures, maxAttempts) -> logger.atInfo().withCause(thrown).log( diff --git a/util/src/test/java/google/registry/util/RetrierTest.java b/util/src/test/java/google/registry/util/RetrierTest.java index 5d499f918..21b4d3d94 100644 --- a/util/src/test/java/google/registry/util/RetrierTest.java +++ b/util/src/test/java/google/registry/util/RetrierTest.java @@ -38,7 +38,7 @@ public class RetrierTest { } } - /** Test object that always throws an exception with the current count. */ + /** Test object that throws CountingExceptions up to a given limit, then succeeds. */ static class CountingThrower implements Callable { int count = 0; @@ -125,4 +125,21 @@ public class RetrierTest { .isEqualTo(0); assertThat(reporter.numBeforeRetry).isEqualTo(0); } + + @Test + public void testRetryPredicate_succeedsWhenRetries() { + // Throws a retryable "1" exception is retryable, and then it succeeds on "1". + assertThat(retrier.callWithRetry(new CountingThrower(1), e -> e.getMessage().equals("1"))) + .isEqualTo(1); + } + + @Test + public void testRetryPredicate_failsWhenDoesntRetry() { + // Throws a retryable "1" exception, then a non-retryable "2" exception, resulting in failure. + CountingException ex = + assertThrows( + CountingException.class, + () -> retrier.callWithRetry(new CountingThrower(2), e -> e.getMessage().equals("1"))); + assertThat(ex).hasMessageThat().isEqualTo("2"); + } }