From dbb40926805eb0757ad739e29e861d1ad3155ef7 Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Fri, 25 Oct 2019 13:54:47 -0400 Subject: [PATCH] Don't retry permanent failures when uploading ICANN monthly reports (#328) * Don't retry permanent failures when uploading ICANN monthly reports There are two kinds of permanent failures that this checks for that we know will never succeed, so it makes no sense to continue retrying 11 more times before moving onto the next file to upload. These errors are: 1. com.google.api.client.http.HttpResponseException: 403 Your IP address xx.xx.xx.xx is not allowed to connect 2. com.google.api.client.http.HttpResponseException: 400 A report for that month already exists, the cut-off date already passed.Date: 2019-09 In order to implement this new functionality, this commit also adds a new way to call Retriable that allows specifying the isRetryable Predicate (which is quite useful). --- .../icann/IcannReportingUploadAction.java | 18 +++++- .../icann/IcannReportingUploadActionTest.java | 39 +++++++++++++ .../java/google/registry/util/Retrier.java | 56 ++++++++++--------- .../google/registry/util/RetrierTest.java | 19 ++++++- 4 files changed, 104 insertions(+), 28 deletions(-) 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"); + } }