Allow replicateToDatastore to skip gaps (#1539)

* Allow replicateToDatastore to skip gaps

As it turns out, gaps in the transaction id sequence number are expected
because rollbacks do not rollback sequence numbers.

To deal with this, stop checking these.

This change is not adequate in and of itself, as it is possible for a gap to
be introduced if two transactions are committed out of order of their sequence
number.  We are currently discussing several strategies to mitigate this.

* Remove println, add a record verification

core/src/main/java/google/registry/model/replay/ReplicateToDatastoreAction.java

@@ -128,13 +128,10 @@ public class ReplicateToDatastoreAction implements Runnable {
                 LastSqlTransaction lastSqlTxn = LastSqlTransaction.load();
                 long nextTxnId = lastSqlTxn.getTransactionId() + 1;
                 if (nextTxnId < txnEntity.getId()) {
-                  // We're missing a transaction.  This is bad.  Transaction ids are supposed to
+                  // Missing transaction id.  This can happen normally.  If a transaction gets
-                  // increase monotonically, so we abort rather than applying anything out of
+                  // rolled back, the sequence counter doesn't.
-                  // order.
+                  logger.atWarning().log(
-                  throw new IllegalStateException(
+                      "Ignoring transaction %s, which does not exist.", nextTxnId);
-                      String.format(
-                          "Missing transaction: last txn id = %s, next available txn = %s",
-                          nextTxnId - 1, txnEntity.getId()));
                 } else if (nextTxnId > txnEntity.getId()) {
                   // We've already replayed this transaction.  This shouldn't happen, as GAE cron
                   // is supposed to avoid overruns and this action shouldn't be executed from any

core/src/main/java/google/registry/persistence/transaction/TransactionEntity.java

@@ -14,6 +14,7 @@
 
 package google.registry.persistence.transaction;
 
+import com.google.common.annotations.VisibleForTesting;
 import google.registry.model.ImmutableObject;
 import google.registry.model.replay.SqlOnlyEntity;
 import javax.persistence.Entity;
@@ -39,7 +40,8 @@ public class TransactionEntity extends ImmutableObject implements SqlOnlyEntity
 
   TransactionEntity() {}
 
-  TransactionEntity(byte[] contents) {
+  @VisibleForTesting
+  public TransactionEntity(byte[] contents) {
     this.contents = contents;
   }
 

core/src/test/java/google/registry/model/replay/ReplicateToDatastoreActionTest.java

@@ -21,16 +21,15 @@ import static google.registry.persistence.transaction.TransactionManagerFactory.
 import static google.registry.testing.DatabaseHelper.insertInDb;
 import static google.registry.testing.LogsSubject.assertAboutLogs;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
-import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
 import static javax.servlet.http.HttpServletResponse.SC_NO_CONTENT;
 import static javax.servlet.http.HttpServletResponse.SC_OK;
-import static org.junit.Assert.assertThrows;
 import static org.junit.jupiter.api.Assumptions.assumeTrue;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
 import com.google.common.base.Suppliers;
 import com.google.common.collect.ImmutableSortedMap;
+import com.google.common.flogger.FluentLogger;
 import com.google.common.testing.TestLogHandler;
 import com.google.common.truth.Truth8;
 import google.registry.model.common.DatabaseMigrationStateSchedule;
@@ -63,6 +62,7 @@ import org.junitpioneer.jupiter.RetryingTest;
 public class ReplicateToDatastoreActionTest {
 
   private final FakeClock fakeClock = new FakeClock(DateTime.parse("2000-01-01TZ"));
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
   @RegisterExtension
   public final AppEngineExtension appEngine =
@@ -203,41 +203,38 @@ public class ReplicateToDatastoreActionTest {
   }
 
   @RetryingTest(4)
-  void testMissingTransactions() {
+  void testNoTransactionIdUpdate() {
-    assumeTrue(ReplayExtension.replayTestsEnabled());
+    // Create an object.
-
-    // Write a transaction (should have a transaction id of 1).
     TestObject foo = TestObject.create("foo");
     insertInDb(foo);
 
-    // Force the last transaction id back to -1 so that we look for transaction 0.
+    // Fail during the transaction to delete it.
-    ofyTm().transact(() -> ofyTm().insert(new LastSqlTransaction(-1)));
+    try {
+      jpaTm()
+          .transact(
+              () -> {
+                jpaTm().delete(foo.key());
+                // Explicitly save the transaction entity to force the id update.
+                jpaTm().insert(new TransactionEntity(new byte[] {1, 2, 3}));
+                throw new RuntimeException("fail!!!");
+              });
+    } catch (Exception e) {
+      logger.atInfo().log("Got expected exception.");
+    }
 
+    TestObject bar = TestObject.create("bar");
+    insertInDb(bar);
+
+    // Make sure we have only the expected transaction ids.
     List<TransactionEntity> txns = action.getTransactionBatchAtSnapshot();
-    assertThat(txns).hasSize(1);
+    assertThat(txns).hasSize(2);
-    assertThat(assertThrows(IllegalStateException.class, () -> applyTransaction(txns.get(0))))
+    for (TransactionEntity txn : txns) {
-        .hasMessageThat()
+      assertThat(txn.getId()).isNotEqualTo(2);
-        .isEqualTo("Missing transaction: last txn id = -1, next available txn = 1");
+      applyTransaction(txn);
-  }
+    }
 
-  @Test
+    assertThat(ofyTm().transact(() -> ofyTm().loadByKey(foo.key()))).isEqualTo(foo);
-  void testMissingTransactions_fullTask() {
+    assertThat(ofyTm().transact(() -> ofyTm().loadByKey(bar.key()))).isEqualTo(bar);
-    assumeTrue(ReplayExtension.replayTestsEnabled());
-
-    // Write a transaction (should have a transaction id of 1).
-    TestObject foo = TestObject.create("foo");
-    insertInDb(foo);
-
-    // Force the last transaction id back to -1 so that we look for transaction 0.
-    ofyTm().transact(() -> ofyTm().insert(new LastSqlTransaction(-1)));
-    action.run();
-    assertAboutLogs()
-        .that(logHandler)
-        .hasSevereLogWithCause(
-            new IllegalStateException(
-                "Missing transaction: last txn id = -1, next available txn = 1"));
-    assertThat(response.getStatus()).isEqualTo(SC_INTERNAL_SERVER_ERROR);
-    assertThat(response.getPayload()).isEqualTo("Errored out replaying files.");
   }
 
   @Test