From 377fe5f573e464ad045f0218e7d99587bdf520ba Mon Sep 17 00:00:00 2001 From: mcilwain Date: Tue, 3 Apr 2018 07:58:58 -0700 Subject: [PATCH] Allow number of commit log buckets to be increased Also increases the number of commit log buckets on alpha to 397 and correspondingly reduces the frequency of commit log diff exporting to once every 3 minutes. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=191440586 --- .../backup/ExportCommitLogDiffAction.java | 12 ++- .../env/alpha/default/WEB-INF/cron.xml | 2 +- .../model/ofy/CommitLogCheckpoint.java | 22 ++++- .../backup/ExportCommitLogDiffActionTest.java | 82 +++++++++++++++++++ 4 files changed, 109 insertions(+), 9 deletions(-) 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;