From ffd952a60e3c9b196772d0a9156f8f4ae6dbabcd Mon Sep 17 00:00:00 2001 From: Weimin Yu Date: Wed, 23 Aug 2023 10:55:51 -0400 Subject: [PATCH] Fix Cloud Tasks retry failure (#2118) * Fix Cloud Tasks failure to retry Replace `SC_NOT_MODIFIED` (304) with `SC_SERVICE_UNAVAILABLE` (503) when data is not available yet. Affected actions are invoice- and spec11-publishing. It is confirmed that Cloud Tasks currently does not retry with code 304, despite the public documentation stating so. We will use 503 for now, pending the decision by Cloud Tasks whether to change behavior or documentation. The code `TOO_EARLY` (425) is another alternative. It is not meant for our use case but at least sounds like it is. However, it is not in any javax.servlet jar. We don't want to define our own constant, and we cannot upgrade to jakarta.servlet yet. Also revert previous mitigation. --- .../registry/reporting/billing/PublishInvoicesAction.java | 4 ++-- .../registry/reporting/spec11/GenerateSpec11ReportAction.java | 3 +-- .../registry/reporting/spec11/PublishSpec11ReportAction.java | 4 ++-- .../registry/reporting/billing/PublishInvoicesActionTest.java | 4 ++-- .../reporting/spec11/GenerateSpec11ReportActionTest.java | 3 +-- .../reporting/spec11/PublishSpec11ReportActionTest.java | 4 ++-- 6 files changed, 10 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/google/registry/reporting/billing/PublishInvoicesAction.java b/core/src/main/java/google/registry/reporting/billing/PublishInvoicesAction.java index 4db12d281..e31d962fe 100644 --- a/core/src/main/java/google/registry/reporting/billing/PublishInvoicesAction.java +++ b/core/src/main/java/google/registry/reporting/billing/PublishInvoicesAction.java @@ -17,9 +17,9 @@ package google.registry.reporting.billing; import static google.registry.reporting.ReportingModule.PARAM_YEAR_MONTH; import static google.registry.request.Action.Method.POST; import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR; -import static javax.servlet.http.HttpServletResponse.SC_NOT_MODIFIED; import static javax.servlet.http.HttpServletResponse.SC_NO_CONTENT; import static javax.servlet.http.HttpServletResponse.SC_OK; +import static javax.servlet.http.HttpServletResponse.SC_SERVICE_UNAVAILABLE; import com.google.api.services.dataflow.Dataflow; import com.google.api.services.dataflow.model.Job; @@ -111,7 +111,7 @@ public class PublishInvoicesAction implements Runnable { break; default: logger.atInfo().log("Job in non-terminal state %s, retrying:", state); - response.setStatus(SC_NOT_MODIFIED); + response.setStatus(SC_SERVICE_UNAVAILABLE); break; } } catch (IOException e) { diff --git a/core/src/main/java/google/registry/reporting/spec11/GenerateSpec11ReportAction.java b/core/src/main/java/google/registry/reporting/spec11/GenerateSpec11ReportAction.java index 57a97021f..d92ba0f4a 100644 --- a/core/src/main/java/google/registry/reporting/spec11/GenerateSpec11ReportAction.java +++ b/core/src/main/java/google/registry/reporting/spec11/GenerateSpec11ReportAction.java @@ -141,8 +141,7 @@ public class GenerateSpec11ReportAction implements Runnable { jobId, ReportingModule.PARAM_DATE, date.toString()), - // TODO(b/296582836): mitigating retry problem. Remove `+10` when bug is fixed. - Duration.standardMinutes(ReportingModule.ENQUEUE_DELAY_MINUTES + 10))); + Duration.standardMinutes(ReportingModule.ENQUEUE_DELAY_MINUTES))); } response.setStatus(SC_OK); response.setPayload(String.format("Launched Spec11 pipeline: %s", jobId)); diff --git a/core/src/main/java/google/registry/reporting/spec11/PublishSpec11ReportAction.java b/core/src/main/java/google/registry/reporting/spec11/PublishSpec11ReportAction.java index f3a91328e..5cc2e3749 100644 --- a/core/src/main/java/google/registry/reporting/spec11/PublishSpec11ReportAction.java +++ b/core/src/main/java/google/registry/reporting/spec11/PublishSpec11ReportAction.java @@ -18,9 +18,9 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet; import static google.registry.reporting.ReportingModule.PARAM_DATE; import static google.registry.request.Action.Method.POST; import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR; -import static javax.servlet.http.HttpServletResponse.SC_NOT_MODIFIED; import static javax.servlet.http.HttpServletResponse.SC_NO_CONTENT; import static javax.servlet.http.HttpServletResponse.SC_OK; +import static javax.servlet.http.HttpServletResponse.SC_SERVICE_UNAVAILABLE; import com.google.api.services.dataflow.Dataflow; import com.google.api.services.dataflow.model.Job; @@ -135,7 +135,7 @@ public class PublishSpec11ReportAction implements Runnable { break; default: logger.atInfo().log("Job in non-terminal state %s, retrying:", state); - response.setStatus(SC_NOT_MODIFIED); + response.setStatus(SC_SERVICE_UNAVAILABLE); break; } } catch (IOException | JSONException e) { diff --git a/core/src/test/java/google/registry/reporting/billing/PublishInvoicesActionTest.java b/core/src/test/java/google/registry/reporting/billing/PublishInvoicesActionTest.java index d8bafefc7..0e6f8aaf4 100644 --- a/core/src/test/java/google/registry/reporting/billing/PublishInvoicesActionTest.java +++ b/core/src/test/java/google/registry/reporting/billing/PublishInvoicesActionTest.java @@ -16,9 +16,9 @@ package google.registry.reporting.billing; import static com.google.common.truth.Truth.assertThat; import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR; -import static javax.servlet.http.HttpServletResponse.SC_NOT_MODIFIED; import static javax.servlet.http.HttpServletResponse.SC_NO_CONTENT; import static javax.servlet.http.HttpServletResponse.SC_OK; +import static javax.servlet.http.HttpServletResponse.SC_SERVICE_UNAVAILABLE; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -101,7 +101,7 @@ class PublishInvoicesActionTest { void testJobIndeterminate_returnsRetriableResponse() { expectedJob.setCurrentState("JOB_STATE_RUNNING"); uploadAction.run(); - assertThat(response.getStatus()).isEqualTo(SC_NOT_MODIFIED); + assertThat(response.getStatus()).isEqualTo(SC_SERVICE_UNAVAILABLE); } @Test diff --git a/core/src/test/java/google/registry/reporting/spec11/GenerateSpec11ReportActionTest.java b/core/src/test/java/google/registry/reporting/spec11/GenerateSpec11ReportActionTest.java index 000a139df..ca534566f 100644 --- a/core/src/test/java/google/registry/reporting/spec11/GenerateSpec11ReportActionTest.java +++ b/core/src/test/java/google/registry/reporting/spec11/GenerateSpec11ReportActionTest.java @@ -93,8 +93,7 @@ class GenerateSpec11ReportActionTest extends BeamActionTestBase { .scheduleTime( clock .nowUtc() - // TODO(b/296582836): mitigating retry problem. Remove `+10` when bug is fixed. - .plus(Duration.standardMinutes(ReportingModule.ENQUEUE_DELAY_MINUTES + 10)))); + .plus(Duration.standardMinutes(ReportingModule.ENQUEUE_DELAY_MINUTES)))); } @Test diff --git a/core/src/test/java/google/registry/reporting/spec11/PublishSpec11ReportActionTest.java b/core/src/test/java/google/registry/reporting/spec11/PublishSpec11ReportActionTest.java index 5db85d41b..f8baee81b 100644 --- a/core/src/test/java/google/registry/reporting/spec11/PublishSpec11ReportActionTest.java +++ b/core/src/test/java/google/registry/reporting/spec11/PublishSpec11ReportActionTest.java @@ -19,9 +19,9 @@ import static google.registry.reporting.spec11.Spec11RegistrarThreatMatchesParse import static google.registry.reporting.spec11.Spec11RegistrarThreatMatchesParserTest.getMatchB; import static google.registry.reporting.spec11.Spec11RegistrarThreatMatchesParserTest.sampleThreatMatches; import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR; -import static javax.servlet.http.HttpServletResponse.SC_NOT_MODIFIED; import static javax.servlet.http.HttpServletResponse.SC_NO_CONTENT; import static javax.servlet.http.HttpServletResponse.SC_OK; +import static javax.servlet.http.HttpServletResponse.SC_SERVICE_UNAVAILABLE; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -134,7 +134,7 @@ class PublishSpec11ReportActionTest { void testJobIndeterminate_returnsRetriableResponse() { expectedJob.setCurrentState("JOB_STATE_RUNNING"); publishAction.run(); - assertThat(response.getStatus()).isEqualTo(SC_NOT_MODIFIED); + assertThat(response.getStatus()).isEqualTo(SC_SERVICE_UNAVAILABLE); verifyNoMoreInteractions(emailUtils); }