Stop dual read and dual write of SMDRL (#1095)

* Stop dual read and dual write of SMDRL

* Remove some more stuff from SignedMarkRevocationListDaoTest

* Change some names

core/src/main/java/google/registry/model/smd/SignedMarkRevocationListDao.java

@@ -14,178 +14,44 @@
 
 package google.registry.model.smd;
 
-import static com.google.common.base.Preconditions.checkNotNull;
-import static com.google.common.base.Preconditions.checkState;
-import static com.google.common.collect.ImmutableList.toImmutableList;
-import static com.google.common.collect.Iterables.isEmpty;
-import static google.registry.model.DatabaseMigrationUtils.suppressExceptionUnlessInTest;
-import static google.registry.model.common.EntityGroupRoot.getCrossTldKey;
-import static google.registry.model.ofy.ObjectifyService.allocateId;
-import static google.registry.model.ofy.ObjectifyService.ofy;
-import static google.registry.model.smd.SignedMarkRevocationList.SHARD_SIZE;
 import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
-import static google.registry.persistence.transaction.TransactionManagerFactory.ofyTm;
-import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
-import static google.registry.util.CollectionUtils.isNullOrEmpty;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
 
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Iterables;
-import com.google.common.collect.MapDifference;
-import com.google.common.collect.Maps;
 import com.google.common.flogger.FluentLogger;
-import google.registry.util.CollectionUtils;
-import java.util.Map;
 import java.util.Optional;
-import org.joda.time.DateTime;
 
 public class SignedMarkRevocationListDao {
 
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
-  /**
+  /** Loads the {@link SignedMarkRevocationList}. */
-   * Loads the {@link SignedMarkRevocationList}.
-   *
-   * <p>Loads the list from Cloud SQL, and attempts to load from Datastore. If the load from
-   * Datastore fails, or the list from Datastore does not match the list from Cloud SQL, the error
-   * will be logged but no exception will be thrown.
-   */
   static SignedMarkRevocationList load() {
-    Optional<SignedMarkRevocationList> primaryList = loadFromCloudSql();
+    Optional<SignedMarkRevocationList> smdrl =
-    if (!primaryList.isPresent()) {
+        jpaTm()
-      return SignedMarkRevocationList.create(START_OF_TIME, ImmutableMap.of());
+            .transact(
-    }
+                () -> {
-    suppressExceptionUnlessInTest(
+                  Long revisionId =
-        () -> loadAndCompare(primaryList.get()),
+                      jpaTm()
-        "Error loading and comparing the SignedMarkRevocationList from Datastore");
+                          .query("SELECT MAX(revisionId) FROM SignedMarkRevocationList", Long.class)
-    return primaryList.get();
+                          .getSingleResult();
+                  return jpaTm()
+                      .query(
+                          "FROM SignedMarkRevocationList smrl LEFT JOIN FETCH smrl.revokes "
+                              + "WHERE smrl.revisionId = :revisionId",
+                          SignedMarkRevocationList.class)
+                      .setParameter("revisionId", revisionId)
+                      .getResultStream()
+                      .findFirst();
+                });
+    return smdrl.orElseGet(() -> SignedMarkRevocationList.create(START_OF_TIME, ImmutableMap.of()));
   }
 
-  /** Loads the list from Datastore and compares it to the list from Cloud SQL. */
+  /** Save the given {@link SignedMarkRevocationList} */
-  private static void loadAndCompare(SignedMarkRevocationList primaryList) {
-    Optional<SignedMarkRevocationList> secondaryList = loadFromDatastore();
-    if (secondaryList.isPresent() && !isNullOrEmpty(secondaryList.get().revokes)) {
-      MapDifference<String, DateTime> diff =
-          Maps.difference(primaryList.revokes, secondaryList.get().revokes);
-      if (!diff.areEqual()) {
-        if (diff.entriesDiffering().size() > 10) {
-          String message =
-              String.format(
-                  "Unequal SignedMarkRevocationList detected, Datastore list with revision id"
-                      + " %d has %d different records than the current Cloud SQL list.",
-                  secondaryList.get().revisionId, diff.entriesDiffering().size());
-          throw new IllegalStateException(message);
-        } else {
-          StringBuilder diffMessage =
-              new StringBuilder("Unequal SignedMarkRevocationList detected:\n");
-          diff.entriesDiffering()
-              .forEach(
-                  (label, valueDiff) ->
-                      diffMessage.append(
-                          String.format(
-                              "SMD %s has key %s in Cloud SQL and key %s in Datastore.\n",
-                              label, valueDiff.leftValue(), valueDiff.rightValue())));
-          throw new IllegalStateException(diffMessage.toString());
-        }
-      }
-    } else {
-      if (primaryList.size() != 0) {
-        throw new IllegalStateException(
-            "SignedMarkRevocationList in Datastore is empty while it is not empty in Cloud SQL.");
-      }
-    }
-  }
-
-  /** Loads the shards from Datastore and combines them into one list. */
-  private static Optional<SignedMarkRevocationList> loadFromDatastore() {
-    return ofyTm()
-        .transactNewReadOnly(
-            () -> {
-              Iterable<SignedMarkRevocationList> shards =
-                  ofy().load().type(SignedMarkRevocationList.class).ancestor(getCrossTldKey());
-              DateTime creationTime =
-                  isEmpty(shards)
-                      ? START_OF_TIME
-                      : checkNotNull(Iterables.get(shards, 0).creationTime, "creationTime");
-              ImmutableMap.Builder<String, DateTime> revokes = new ImmutableMap.Builder<>();
-              for (SignedMarkRevocationList shard : shards) {
-                revokes.putAll(shard.revokes);
-                checkState(
-                    creationTime.equals(shard.creationTime),
-                    "Inconsistent creation times in Datastore shard: %s vs. %s",
-                    creationTime,
-                    shard.creationTime);
-              }
-              return Optional.of(SignedMarkRevocationList.create(creationTime, revokes.build()));
-            });
-  }
-
-  private static Optional<SignedMarkRevocationList> loadFromCloudSql() {
-    return jpaTm()
-        .transact(
-            () -> {
-              Long revisionId =
-                  jpaTm()
-                      .query("SELECT MAX(revisionId) FROM SignedMarkRevocationList", Long.class)
-                      .getSingleResult();
-              return jpaTm()
-                  .query(
-                      "FROM SignedMarkRevocationList smrl LEFT JOIN FETCH smrl.revokes "
-                          + "WHERE smrl.revisionId = :revisionId",
-                      SignedMarkRevocationList.class)
-                  .setParameter("revisionId", revisionId)
-                  .getResultStream()
-                  .findFirst();
-            });
-  }
-
-  /**
-   * Save the given {@link SignedMarkRevocationList}
-   *
-   * <p>Saves the list to Cloud SQL, and attempts to save to Datastore. If the save to Datastore
-   * fails, the error will be logged but no exception will be thrown.
-   */
   static void save(SignedMarkRevocationList signedMarkRevocationList) {
-      SignedMarkRevocationListDao.saveToCloudSql(signedMarkRevocationList);
-      suppressExceptionUnlessInTest(
-          () ->
-              saveToDatastore(
-                  signedMarkRevocationList.revokes, signedMarkRevocationList.creationTime),
-          "Error inserting signed mark revocations into secondary database (Datastore).");
-  }
-
-  private static void saveToCloudSql(SignedMarkRevocationList signedMarkRevocationList) {
     jpaTm().transact(() -> jpaTm().insert(signedMarkRevocationList));
     logger.atInfo().log(
         "Inserted %,d signed mark revocations into Cloud SQL.",
         signedMarkRevocationList.revokes.size());
   }
-
-  private static void saveToDatastore(Map<String, DateTime> revokes, DateTime creationTime) {
-    tm().transact(
-            () -> {
-              ofy()
-                  .deleteWithoutBackup()
-                  .keys(
-                      ofy()
-                          .load()
-                          .type(SignedMarkRevocationList.class)
-                          .ancestor(getCrossTldKey())
-                          .keys());
-              ofy()
-                  .saveWithoutBackup()
-                  .entities(
-                      CollectionUtils.partitionMap(revokes, SHARD_SIZE).stream()
-                          .map(
-                              shardRevokes -> {
-                                SignedMarkRevocationList shard =
-                                    SignedMarkRevocationList.create(creationTime, shardRevokes);
-                                shard.id = allocateId();
-                                shard.isShard =
-                                    true; // Avoid the exception in disallowUnshardedSaves().
-                                return shard;
-                              })
-                          .collect(toImmutableList()));
-            });
-  }
 }

core/src/test/java/google/registry/model/smd/SignedMarkRevocationListDaoTest.java

@@ -16,39 +16,22 @@ package google.registry.model.smd;
 
 import static com.google.common.truth.Truth.assertThat;
 import static google.registry.model.ImmutableObjectSubject.assertAboutImmutableObjects;
-import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
-import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import com.google.common.collect.ImmutableMap;
-import google.registry.config.RegistryEnvironment;
 import google.registry.model.EntityTestCase;
 import google.registry.persistence.transaction.JpaTestRules;
 import google.registry.persistence.transaction.JpaTestRules.JpaIntegrationWithCoverageExtension;
-import google.registry.testing.DatastoreEntityExtension;
+import org.junit.jupiter.api.Test;
-import google.registry.testing.DualDatabaseTest;
-import google.registry.testing.SystemPropertyExtension;
-import google.registry.testing.TestOfyAndSql;
-import org.joda.time.DateTime;
-import org.junit.jupiter.api.Order;
 import org.junit.jupiter.api.extension.RegisterExtension;
 
-@DualDatabaseTest
 public class SignedMarkRevocationListDaoTest extends EntityTestCase {
 
   @RegisterExtension
   final JpaIntegrationWithCoverageExtension jpa =
       new JpaTestRules.Builder().withClock(fakeClock).buildIntegrationWithCoverageExtension();
 
-  @RegisterExtension
+  @Test
-  @Order(value = 1)
+  void testSave_success() {
-  final DatastoreEntityExtension datastoreEntityExtension = new DatastoreEntityExtension();
-
-  @RegisterExtension
-  @Order(value = Integer.MAX_VALUE)
-  final SystemPropertyExtension systemPropertyExtension = new SystemPropertyExtension();
-
-  @TestOfyAndSql
-  void testSave_cloudSqlPrimary_success() {
     SignedMarkRevocationList list =
         SignedMarkRevocationList.create(
             fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(1)));
@@ -57,8 +40,8 @@ public class SignedMarkRevocationListDaoTest extends EntityTestCase {
     assertAboutImmutableObjects().that(fromDb).isEqualExceptFields(list);
   }
 
-  @TestOfyAndSql
+  @Test
-  void testSaveAndLoad_cloudSqlPrimary_emptyList() {
+  void testSaveAndLoad_emptyList() {
     SignedMarkRevocationList list =
         SignedMarkRevocationList.create(fakeClock.nowUtc(), ImmutableMap.of());
     SignedMarkRevocationListDao.save(list);
@@ -66,8 +49,8 @@ public class SignedMarkRevocationListDaoTest extends EntityTestCase {
     assertAboutImmutableObjects().that(fromDb).isEqualExceptFields(list, "revisionId");
   }
 
-  @TestOfyAndSql
+  @Test
-  void testSave_cloudSqlPrimary_multipleVersions() {
+  void testSave_multipleVersions() {
     SignedMarkRevocationList list =
         SignedMarkRevocationList.create(
             fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(1)));
@@ -83,52 +66,4 @@ public class SignedMarkRevocationListDaoTest extends EntityTestCase {
         .isFalse();
   }
 
-  @TestOfyAndSql
-  void testLoad_cloudSqlPrimary_unequalLists() {
-    fakeClock.setTo(DateTime.parse("1984-12-26T00:00:00.000Z"));
-    SignedMarkRevocationList list =
-        SignedMarkRevocationList.create(
-            fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(1)));
-    SignedMarkRevocationListDao.save(list);
-    SignedMarkRevocationList list2 =
-        SignedMarkRevocationList.create(
-            fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(3)));
-    jpaTm().transact(() -> jpaTm().put(list2));
-    RuntimeException thrown =
-        assertThrows(RuntimeException.class, SignedMarkRevocationListDao::load);
-    assertThat(thrown)
-        .hasMessageThat()
-        .contains(
-            "SMD mark has key 1984-12-25T21:00:00.000Z in Cloud SQL and key"
-                + " 1984-12-25T23:00:00.000Z in Datastore.");
-  }
-
-  @TestOfyAndSql
-  void testLoad_cloudSqlPrimary_unequalLists_succeedsInProduction() {
-    RegistryEnvironment.PRODUCTION.setup(systemPropertyExtension);
-    SignedMarkRevocationList list =
-        SignedMarkRevocationList.create(
-            fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(1)));
-    SignedMarkRevocationListDao.save(list);
-    SignedMarkRevocationList list2 =
-        SignedMarkRevocationList.create(
-            fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(3)));
-    jpaTm().transact(() -> jpaTm().put(list2));
-    SignedMarkRevocationList fromDb = SignedMarkRevocationListDao.load();
-    assertAboutImmutableObjects().that(fromDb).isEqualExceptFields(list2, "revisionId");
-  }
-
-  @TestOfyAndSql
-  void testLoad_cloudSqlPrimary_noListInDatastore() {
-    SignedMarkRevocationList list =
-        SignedMarkRevocationList.create(
-            fakeClock.nowUtc(), ImmutableMap.of("mark", fakeClock.nowUtc().minusHours(1)));
-    jpaTm().transact(() -> jpaTm().put(list));
-    RuntimeException thrown =
-        assertThrows(RuntimeException.class, SignedMarkRevocationListDao::load);
-    assertThat(thrown)
-        .hasMessageThat()
-        .contains(
-            "SignedMarkRevocationList in Datastore is empty while it is not empty in Cloud SQL.");
-  }
 }

core/src/test/java/google/registry/model/smd/SignedMarkRevocationListTest.java

@@ -15,11 +15,7 @@
 package google.registry.model.smd;
 
 import static com.google.common.truth.Truth.assertThat;
-import static google.registry.model.ImmutableObjectSubject.assertAboutImmutableObjects;
-import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.model.smd.SignedMarkRevocationList.SHARD_SIZE;
-import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
-import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
 import static org.joda.time.Duration.standardDays;
 import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -40,24 +36,6 @@ public class SignedMarkRevocationListTest {
 
   private final FakeClock clock = new FakeClock(DateTime.parse("2013-01-01T00:00:00Z"));
 
-  @Test
-  void testUnshardedSaveFails() {
-    // Our @Entity's @OnSave method will notice that this shouldn't be saved.
-    assertThrows(
-        SignedMarkRevocationList.UnshardedSaveException.class,
-        () ->
-            tm()
-                .transact(
-                    () -> {
-                      SignedMarkRevocationList smdrl =
-                          SignedMarkRevocationList.create(
-                              tm().getTransactionTime(),
-                              ImmutableMap.of("a", tm().getTransactionTime()));
-                      smdrl.id = 1; // Without an id this won't save anyways.
-                      ofy().saveWithoutBackup().entity(smdrl).now();
-                    }));
-  }
-
   @Test
   void testEmpty() {
     // When Datastore is empty, it should give us an empty thing.
@@ -65,41 +43,6 @@ public class SignedMarkRevocationListTest {
         .isEqualTo(SignedMarkRevocationList.create(START_OF_TIME, ImmutableMap.of()));
   }
 
-  @Test
-  void testSharding2() {
-    final int rows = SHARD_SIZE + 1;
-    // Create a SignedMarkRevocationList that will need 2 shards to save.
-    ImmutableMap.Builder<String, DateTime> revokes = new ImmutableMap.Builder<>();
-    for (int i = 0; i < rows; i++) {
-      revokes.put(Integer.toString(i), clock.nowUtc());
-    }
-    // Save it with sharding, and make sure that reloading it works.
-    SignedMarkRevocationList unsharded =
-        SignedMarkRevocationList.create(clock.nowUtc(), revokes.build()).save();
-    assertAboutImmutableObjects()
-        .that(SignedMarkRevocationList.get())
-        .isEqualExceptFields(unsharded, "revisionId");
-    assertThat(ofy().load().type(SignedMarkRevocationList.class).count()).isEqualTo(2);
-  }
-
-  @Test
-  void testSharding4() {
-    final int rows = SHARD_SIZE * 3 + 1;
-    // Create a SignedMarkRevocationList that will need 4 shards to save.
-    ImmutableMap.Builder<String, DateTime> revokes = new ImmutableMap.Builder<>();
-    for (int i = 0; i < rows; i++) {
-      revokes.put(Integer.toString(i), clock.nowUtc());
-    }
-    // Save it with sharding, and make sure that reloading it works.
-    SignedMarkRevocationList unsharded = SignedMarkRevocationList
-        .create(clock.nowUtc(), revokes.build())
-        .save();
-    assertAboutImmutableObjects()
-        .that(SignedMarkRevocationList.get())
-        .isEqualExceptFields(unsharded, "revisionId");
-    assertThat(ofy().load().type(SignedMarkRevocationList.class).count()).isEqualTo(4);
-  }
-
   private SignedMarkRevocationList createSaveGetHelper(int rows) {
     ImmutableMap.Builder<String, DateTime> revokes = new ImmutableMap.Builder<>();
     for (int i = 0; i < rows; i++) {
@@ -139,22 +82,6 @@ public class SignedMarkRevocationListTest {
         .isEqualTo(DateTime.parse("2000-01-01T00:00:00Z"));
   }
 
-  @Test
-  void test_getCreationTime_unequalListsInDatabases() {
-    clock.setTo(DateTime.parse("2000-01-01T00:00:00Z"));
-    createSaveGetHelper(1);
-    ImmutableMap.Builder<String, DateTime> revokes = new ImmutableMap.Builder<>();
-    for (int i = 0; i < 3; i++) {
-      revokes.put(Integer.toString(i), clock.nowUtc());
-    }
-    jpaTm()
-        .transact(
-            () -> jpaTm().insert(SignedMarkRevocationList.create(clock.nowUtc(), revokes.build())));
-    RuntimeException thrown =
-        assertThrows(RuntimeException.class, () -> SignedMarkRevocationList.get());
-    assertThat(thrown).hasMessageThat().contains("Unequal SignedMarkRevocationList detected:");
-  }
-
   @Test
   void test_isSmdRevoked_present() {
     final int rows = SHARD_SIZE + 1;