From 74b7ec052f62fa9f45af65a79cf16c4c062a9109 Mon Sep 17 00:00:00 2001 From: Rachel Guan Date: Tue, 15 Mar 2022 10:49:35 -0400 Subject: [PATCH] Add a a delay to task in CommitLogCheckpointAction (#1554) * Add delay to task --- .../backup/CommitLogCheckpointAction.java | 24 +++++++++++++++---- .../backup/CommitLogCheckpointActionTest.java | 9 +++---- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/google/registry/backup/CommitLogCheckpointAction.java b/core/src/main/java/google/registry/backup/CommitLogCheckpointAction.java index 755dc2a71..057e54632 100644 --- a/core/src/main/java/google/registry/backup/CommitLogCheckpointAction.java +++ b/core/src/main/java/google/registry/backup/CommitLogCheckpointAction.java @@ -28,11 +28,11 @@ import google.registry.model.ofy.CommitLogCheckpointRoot; import google.registry.request.Action; import google.registry.request.Action.Service; import google.registry.request.auth.Auth; -import google.registry.util.Clock; import google.registry.util.CloudTasksUtils; import java.util.Optional; import javax.inject.Inject; import org.joda.time.DateTime; +import org.joda.time.Duration; /** * Action that saves commit log checkpoints to Datastore and kicks off a diff export task. @@ -57,7 +57,22 @@ public final class CommitLogCheckpointAction implements Runnable { private static final String QUEUE_NAME = "export-commits"; - @Inject Clock clock; + /** + * The amount of time enqueueing should be delayed. + * + *

The {@link ExportCommitLogDiffAction} is enqueued in {@link CommitLogCheckpointAction}, + * which is inside a Datastore transaction that persists the checkpoint to be exported. After the + * switch to CloudTasks API, the task may be invoked before the Datastore transaction commits. + * When this happens, the checkpoint is not found which leads to {@link + * com.google.common.base.VerifyException}. + * + *

In order to invoke the task after the transaction commits, a reasonable delay should be + * added to each task. The latency of the request is mostly in the range of 4-6 seconds; Choosing + * a value 30% greater than the upper bound should solve the issue invoking a task before the + * transaction commits. + */ + static final Duration ENQUEUE_DELAY_SECONDS = Duration.standardSeconds(8); + @Inject CommitLogCheckpointStrategy strategy; @Inject CloudTasksUtils cloudTasksUtils; @@ -96,14 +111,15 @@ public final class CommitLogCheckpointAction implements Runnable { // Enqueue a diff task between previous and current checkpoints. cloudTasksUtils.enqueue( QUEUE_NAME, - cloudTasksUtils.createPostTask( + cloudTasksUtils.createPostTaskWithDelay( ExportCommitLogDiffAction.PATH, Service.BACKEND.toString(), ImmutableMultimap.of( LOWER_CHECKPOINT_TIME_PARAM, lastWrittenTime.toString(), UPPER_CHECKPOINT_TIME_PARAM, - checkpoint.getCheckpointTime().toString()))); + checkpoint.getCheckpointTime().toString()), + ENQUEUE_DELAY_SECONDS)); return true; }); return isCheckPointPersisted ? Optional.of(checkpoint) : Optional.empty(); diff --git a/core/src/test/java/google/registry/backup/CommitLogCheckpointActionTest.java b/core/src/test/java/google/registry/backup/CommitLogCheckpointActionTest.java index 7d418b736..5286c2e1a 100644 --- a/core/src/test/java/google/registry/backup/CommitLogCheckpointActionTest.java +++ b/core/src/test/java/google/registry/backup/CommitLogCheckpointActionTest.java @@ -47,11 +47,10 @@ public class CommitLogCheckpointActionTest { private DateTime now = DateTime.now(UTC); private CommitLogCheckpointAction task = new CommitLogCheckpointAction(); - private final CloudTasksHelper cloudTasksHelper = new CloudTasksHelper(); + private final CloudTasksHelper cloudTasksHelper = new CloudTasksHelper(new FakeClock(now)); @BeforeEach void beforeEach() { - task.clock = new FakeClock(now); task.strategy = strategy; task.cloudTasksUtils = cloudTasksHelper.getTestCloudTasksUtils(); when(strategy.computeCheckpoint()) @@ -68,7 +67,8 @@ public class CommitLogCheckpointActionTest { new TaskMatcher() .url(ExportCommitLogDiffAction.PATH) .param(ExportCommitLogDiffAction.LOWER_CHECKPOINT_TIME_PARAM, START_OF_TIME.toString()) - .param(ExportCommitLogDiffAction.UPPER_CHECKPOINT_TIME_PARAM, now.toString())); + .param(ExportCommitLogDiffAction.UPPER_CHECKPOINT_TIME_PARAM, now.toString()) + .scheduleTime(now.plus(CommitLogCheckpointAction.ENQUEUE_DELAY_SECONDS))); assertThat(loadRoot().getLastWrittenTime()).isEqualTo(now); } @@ -82,7 +82,8 @@ public class CommitLogCheckpointActionTest { new TaskMatcher() .url(ExportCommitLogDiffAction.PATH) .param(ExportCommitLogDiffAction.LOWER_CHECKPOINT_TIME_PARAM, oneMinuteAgo.toString()) - .param(ExportCommitLogDiffAction.UPPER_CHECKPOINT_TIME_PARAM, now.toString())); + .param(ExportCommitLogDiffAction.UPPER_CHECKPOINT_TIME_PARAM, now.toString()) + .scheduleTime(now.plus(CommitLogCheckpointAction.ENQUEUE_DELAY_SECONDS))); assertThat(loadRoot().getLastWrittenTime()).isEqualTo(now); }