diff --git a/java/google/registry/backup/ExportCommitLogDiffAction.java b/java/google/registry/backup/ExportCommitLogDiffAction.java index 9163e3f1a..6652aa464 100644 --- a/java/google/registry/backup/ExportCommitLogDiffAction.java +++ b/java/google/registry/backup/ExportCommitLogDiffAction.java @@ -14,6 +14,7 @@ package google.registry.backup; +import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Verify.verifyNotNull; import static com.google.common.collect.ImmutableList.toImmutableList; @@ -175,10 +176,13 @@ public final class ExportCommitLogDiffAction implements Runnable { @Nullable CommitLogCheckpoint lowerCheckpoint, CommitLogCheckpoint upperCheckpoint, int bucketNum) { - // If no lower checkpoint exists, use START_OF_TIME as the effective exclusive lower bound. - DateTime lowerCheckpointBucketTime = lowerCheckpoint == null - ? START_OF_TIME - : lowerCheckpoint.getBucketTimestamps().get(bucketNum); + // If no lower checkpoint exists, or if it exists but had no timestamp for this bucket number + // (because the bucket count was increased between these checkpoints), then use START_OF_TIME + // as the effective exclusive lower bound. + DateTime lowerCheckpointBucketTime = + firstNonNull( + (lowerCheckpoint == null) ? null : lowerCheckpoint.getBucketTimestamps().get(bucketNum), + START_OF_TIME); // Since START_OF_TIME=0 is not a valid id in a key, add 1 to both bounds. Then instead of // loading lowerBound < x <= upperBound, we can load lowerBound <= x < upperBound. DateTime lowerBound = lowerCheckpointBucketTime.plusMillis(1); diff --git a/java/google/registry/env/alpha/default/WEB-INF/cron.xml b/java/google/registry/env/alpha/default/WEB-INF/cron.xml index bb0589566..316e770f9 100644 --- a/java/google/registry/env/alpha/default/WEB-INF/cron.xml +++ b/java/google/registry/env/alpha/default/WEB-INF/cron.xml @@ -128,7 +128,7 @@ This job checkpoints the commit log buckets and exports the diff since last checkpoint to GCS. - every 1 minutes synchronized + every 3 minutes synchronized backend diff --git a/java/google/registry/model/ofy/CommitLogCheckpoint.java b/java/google/registry/model/ofy/CommitLogCheckpoint.java index 6c648a9a2..771725723 100644 --- a/java/google/registry/model/ofy/CommitLogCheckpoint.java +++ b/java/google/registry/model/ofy/CommitLogCheckpoint.java @@ -17,6 +17,7 @@ package google.registry.model.ofy; import static com.google.common.base.Preconditions.checkArgument; import static org.joda.time.DateTimeZone.UTC; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.googlecode.objectify.Key; @@ -71,12 +72,11 @@ public class CommitLogCheckpoint extends ImmutableObject { } /** - * Creates a CommitLogCheckpoint for the given wall time and bucket checkpoint times, specified - * as a map from bucket ID to bucket commit timestamp. + * Creates a CommitLogCheckpoint for the given wall time and bucket checkpoint times, specified as + * a map from bucket ID to bucket commit timestamp. */ public static CommitLogCheckpoint create( - DateTime checkpointTime, - ImmutableMap bucketTimestamps) { + DateTime checkpointTime, ImmutableMap bucketTimestamps) { checkArgument( Objects.equals(CommitLogBucket.getBucketIds().asList(), bucketTimestamps.keySet().asList()), "Bucket ids are incorrect: %s", @@ -87,6 +87,20 @@ public class CommitLogCheckpoint extends ImmutableObject { return instance; } + /** + * Creates a CommitLogCheckpoint for the given wall time and bucket checkpoint times. Test only. + * + *

This lacks validation on the bucketTimestamps map. + */ + @VisibleForTesting + public static CommitLogCheckpoint createForTest( + DateTime checkpointTime, ImmutableMap bucketTimestamps) { + CommitLogCheckpoint instance = new CommitLogCheckpoint(); + instance.checkpointTime = checkpointTime.getMillis(); + instance.bucketTimestamps = ImmutableList.copyOf(bucketTimestamps.values()); + return instance; + } + /** Creates a key for the CommitLogCheckpoint for the given wall time. */ public static Key createKey(DateTime checkpointTime) { return Key.create( diff --git a/javatests/google/registry/backup/ExportCommitLogDiffActionTest.java b/javatests/google/registry/backup/ExportCommitLogDiffActionTest.java index cf36f40d4..37c189db9 100644 --- a/javatests/google/registry/backup/ExportCommitLogDiffActionTest.java +++ b/javatests/google/registry/backup/ExportCommitLogDiffActionTest.java @@ -319,6 +319,88 @@ public class ExportCommitLogDiffActionTest { assertThat(exported).containsExactly(upperCheckpoint); } + @Test + public void testRun_checkpointDiffWithNonExistentBucketTimestamps_exportsCorrectly() + throws Exception { + // Non-existent bucket timestamps can exist when the commit log bucket count was increased + // recently. + + task.lowerCheckpointTime = oneMinuteAgo; + task.upperCheckpointTime = now; + + // No lower checkpoint times are persisted for buckets 2 and 3 (simulating a recent increase in + // the number of commit log buckets from 1 to 3), so all mutations on buckets 2 and 3, even + // those older than the lower checkpoint, will be exported. + persistResource( + CommitLogCheckpoint.createForTest(oneMinuteAgo, ImmutableMap.of(1, oneMinuteAgo))); + CommitLogCheckpoint upperCheckpoint = + persistResource( + CommitLogCheckpoint.create( + now, + ImmutableMap.of( + 1, now, + 2, now.minusDays(1), + 3, oneMinuteAgo.minusDays(2)))); + + // These shouldn't be in the diff because the lower bound is exclusive. + persistManifestAndMutation(1, oneMinuteAgo); + // These shouldn't be in the diff because they are above the upper bound. + persistManifestAndMutation(1, now.plusMillis(1)); + persistManifestAndMutation(2, now.minusDays(1).plusMillis(1)); + persistManifestAndMutation(3, oneMinuteAgo.minusDays(2).plusMillis(1)); + // These should be in the diff because they happened after START_OF_TIME on buckets with + // non-existent timestamps. + persistManifestAndMutation(2, oneMinuteAgo.minusDays(1)); + persistManifestAndMutation(3, oneMinuteAgo.minusDays(2)); + // These should be in the diff because they are between the bounds. + persistManifestAndMutation(1, now.minusMillis(1)); + persistManifestAndMutation(2, now.minusDays(1).minusMillis(1)); + // These should be in the diff because they are at the upper bound. + persistManifestAndMutation(1, now); + persistManifestAndMutation(2, now.minusDays(1)); + + task.run(); + + GcsFilename expectedFilename = new GcsFilename("gcs bucket", "commit_diff_until_" + now); + assertWithMessage("GCS file not found: " + expectedFilename) + .that(gcsService.getMetadata(expectedFilename)) + .isNotNull(); + assertThat(gcsService.getMetadata(expectedFilename).getOptions().getUserMetadata()) + .containsExactly( + LOWER_BOUND_CHECKPOINT, + oneMinuteAgo.toString(), + UPPER_BOUND_CHECKPOINT, + now.toString(), + NUM_TRANSACTIONS, + "6"); + List exported = + deserializeEntities(GcsTestingUtils.readGcsFile(gcsService, expectedFilename)); + assertThat(exported.get(0)).isEqualTo(upperCheckpoint); + // We expect these manifests, in time order, with matching mutations. + CommitLogManifest manifest1 = createManifest(3, oneMinuteAgo.minusDays(2)); + CommitLogManifest manifest2 = createManifest(2, oneMinuteAgo.minusDays(1)); + CommitLogManifest manifest3 = createManifest(2, now.minusDays(1).minusMillis(1)); + CommitLogManifest manifest4 = createManifest(2, now.minusDays(1)); + CommitLogManifest manifest5 = createManifest(1, now.minusMillis(1)); + CommitLogManifest manifest6 = createManifest(1, now); + assertThat(exported) + .containsExactly( + upperCheckpoint, + manifest1, + createMutation(manifest1), + manifest2, + createMutation(manifest2), + manifest3, + createMutation(manifest3), + manifest4, + createMutation(manifest4), + manifest5, + createMutation(manifest5), + manifest6, + createMutation(manifest6)) + .inOrder(); + } + @Test public void testRun_exportingFromStartOfTime_exportsAllCommits() throws Exception { task.lowerCheckpointTime = START_OF_TIME;