Always use Cloud SQL as primary for Reserved and Premium Lists (#1113)

* Always use Cloud SQL as primary for Reserved and Premium Lists * small typos * Add a state check * Add test for bloom filter * fix import
2026-09-20 15:04:24 +00:00 · 2021-05-05 17:24:06 -04:00
parent b38574a9fc
commit 038825f254
13 changed files with 88 additions and 469 deletions
@@ -14,12 +14,11 @@

 package google.registry.model.registry.label;

+import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.collect.ImmutableList.toImmutableList;
 import static google.registry.model.DatabaseMigrationUtils.suppressExceptionUnlessInTest;

 import com.google.common.collect.Streams;
-import google.registry.model.DatabaseMigrationUtils;
-import google.registry.model.common.DatabaseTransitionSchedule.TransitionId;
 import google.registry.model.registry.Registry;
 import google.registry.model.registry.label.PremiumList.PremiumListEntry;
 import google.registry.schema.tld.PremiumListSqlDao;
@@ -32,27 +31,20 @@ import org.joda.money.Money;
 /**
 * DAO for {@link PremiumList} objects that handles the branching paths for SQL and Datastore.
 *
- * <p>For write actions, this class will perform the action against the primary database then, after
- * that success or failure, against the secondary database. If the secondary database fails, an
- * error is logged (but not thrown).
+ * <p>For write actions, this class will perform the action against Cloud SQL then, after that
+ * success or failure, against Datastore. If Datastore fails, an error is logged (but not thrown).
 *
 * <p>For read actions, when retrieving a price, we will log if the primary and secondary databases
- * have different values (or if the retrieval from the second database fails).
- *
- * <p>TODO (gbrodman): Change the isOfy() calls to the runtime selection of DBs when available
+ * have different values (or if the retrieval from Datastore fails).
 */
 public class PremiumListDualDao {

  /**
-   * Retrieves from the appropriate DB and returns the most recent premium list with the given name,
-   * or absent if no such list exists.
+   * Retrieves from Cloud SQL and returns the most recent premium list with the given name, or
+   * absent if no such list exists.
   */
  public static Optional<PremiumList> getLatestRevision(String premiumListName) {
-    if (DatabaseMigrationUtils.isDatastore(TransitionId.DOMAIN_LABEL_LISTS)) {
-      return PremiumListDatastoreDao.getLatestRevision(premiumListName);
-    } else {
      return PremiumListSqlDao.getLatestRevision(premiumListName);
-    }
  }

  /**
@@ -61,7 +53,7 @@ public class PremiumListDualDao {
   * <p>Returns absent if the label is not premium or there is no premium list for this registry.
   *
   * <p>Retrieves the price from both primary and secondary databases, and logs in the event of a
-   * failure in the secondary (but does not throw an exception).
+   * failure in Datastore (but does not throw an exception).
   */
  public static Optional<Money> getPremiumPrice(String label, Registry registry) {
    if (registry.getPremiumList() == null) {
@@ -69,98 +61,56 @@ public class PremiumListDualDao {
    }
    String premiumListName = registry.getPremiumList().getName();
    Optional<Money> primaryResult;
-    if (DatabaseMigrationUtils.isDatastore(TransitionId.DOMAIN_LABEL_LISTS)) {
-      primaryResult =
-          PremiumListDatastoreDao.getPremiumPrice(premiumListName, label, registry.getTldStr());
-    } else {
      primaryResult = PremiumListSqlDao.getPremiumPrice(premiumListName, label);
-    }
-    // Also load the value from the secondary DB, compare the two results, and log if different.
-    if (DatabaseMigrationUtils.isDatastore(TransitionId.DOMAIN_LABEL_LISTS)) {
-      suppressExceptionUnlessInTest(
-          () -> {
-            Optional<Money> secondaryResult =
-                PremiumListSqlDao.getPremiumPrice(premiumListName, label);
-            if (!primaryResult.equals(secondaryResult)) {
-              throw new IllegalStateException(
-                  String.format(
-                      "Unequal prices for domain %s.%s from primary Datastore DB (%s) and "
-                          + "secondary SQL db (%s).",
-                      label, registry.getTldStr(), primaryResult, secondaryResult));
-            }
-          },
-          String.format(
-              "Error loading price of domain %s.%s from Cloud SQL.", label, registry.getTldStr()));
-    } else {
-      suppressExceptionUnlessInTest(
-          () -> {
-            Optional<Money> secondaryResult =
-                PremiumListDatastoreDao.getPremiumPrice(
-                    premiumListName, label, registry.getTldStr());
-            if (!primaryResult.equals(secondaryResult)) {
-              throw new IllegalStateException(
-                  String.format(
-                      "Unequal prices for domain %s.%s from primary SQL DB (%s) and secondary "
-                          + "Datastore db (%s).",
-                      label, registry.getTldStr(), primaryResult, secondaryResult));
-            }
-          },
-          String.format(
-              "Error loading price of domain %s.%s from Datastore.", label, registry.getTldStr()));
-    }
+    // Also load the value from Datastore, compare the two results, and log if different.
+    suppressExceptionUnlessInTest(
+        () -> {
+          Optional<Money> secondaryResult =
+              PremiumListDatastoreDao.getPremiumPrice(premiumListName, label, registry.getTldStr());
+          checkState(
+              primaryResult.equals(secondaryResult),
+              "Unequal prices for domain %s.%s from primary SQL DB (%s) and secondary Datastore db"
+                  + " (%s).",
+              label,
+              registry.getTldStr(),
+              primaryResult,
+              secondaryResult);
+        },
+        String.format(
+            "Error loading price of domain %s.%s from Datastore.", label, registry.getTldStr()));
    return primaryResult;
  }

  /**
   * Saves the given list data to both primary and secondary databases.
   *
-   * <p>Logs but doesn't throw an exception in the event of a failure when writing to the secondary
-   * database.
+   * <p>Logs but doesn't throw an exception in the event of a failure when writing to Datastore.
   */
  public static PremiumList save(String name, List<String> inputData) {
-    PremiumList result;
-    if (DatabaseMigrationUtils.isDatastore(TransitionId.DOMAIN_LABEL_LISTS)) {
-      result = PremiumListDatastoreDao.save(name, inputData);
-      suppressExceptionUnlessInTest(
-          () -> PremiumListSqlDao.save(name, inputData), "Error when saving premium list to SQL.");
-    } else {
-      result = PremiumListSqlDao.save(name, inputData);
+    PremiumList result = PremiumListSqlDao.save(name, inputData);
      suppressExceptionUnlessInTest(
          () -> PremiumListDatastoreDao.save(name, inputData),
          "Error when saving premium list to Datastore.");
-    }
    return result;
  }

  /**
   * Deletes the premium list.
   *
-   * <p>Logs but doesn't throw an exception in the event of a failure when deleting from the
-   * secondary database.
+   * <p>Logs but doesn't throw an exception in the event of a failure when deleting from Datastore.
   */
  public static void delete(PremiumList premiumList) {
-    if (DatabaseMigrationUtils.isDatastore(TransitionId.DOMAIN_LABEL_LISTS)) {
-      PremiumListDatastoreDao.delete(premiumList);
-      suppressExceptionUnlessInTest(
-          () -> PremiumListSqlDao.delete(premiumList),
-          "Error when deleting premium list from SQL.");
-    } else {
      PremiumListSqlDao.delete(premiumList);
      suppressExceptionUnlessInTest(
          () -> PremiumListDatastoreDao.delete(premiumList),
          "Error when deleting premium list from Datastore.");
-    }
  }

  /** Returns whether or not there exists a premium list with the given name. */
  public static boolean exists(String premiumListName) {
    // It may seem like overkill, but loading the list has ways been the way we check existence and
    // given that we usually load the list around the time we check existence, we'll hit the cache
-    if (DatabaseMigrationUtils.isDatastore(TransitionId.DOMAIN_LABEL_LISTS)) {
-      return PremiumListDatastoreDao.getLatestRevision(premiumListName).isPresent();
-    } else {
-      return PremiumListSqlDao.getLatestRevision(premiumListName).isPresent();
-    }
+    return PremiumListSqlDao.getLatestRevision(premiumListName).isPresent();
  }

  /**
@@ -175,9 +125,6 @@ public class PremiumListDualDao {
                () ->
                    new IllegalArgumentException(
                        String.format("No premium list with name %s.", premiumListName)));
-    if (DatabaseMigrationUtils.isDatastore(TransitionId.DOMAIN_LABEL_LISTS)) {
-      return PremiumListDatastoreDao.loadPremiumListEntriesUncached(premiumList);
-    } else {
      CurrencyUnit currencyUnit = premiumList.getCurrency();
      return Streams.stream(PremiumListSqlDao.loadPremiumListEntriesUncached(premiumList))
          .map(
@@ -188,7 +135,6 @@ public class PremiumListDualDao {
                      .build())
          .collect(toImmutableList());
    }
-  }

  private PremiumListDualDao() {}
 }
@@ -15,13 +15,11 @@
 package google.registry.model.registry.label;

 import static com.google.common.collect.ImmutableMap.toImmutableMap;
-import static google.registry.model.DatabaseMigrationUtils.isDatastore;

 import com.google.common.collect.MapDifference;
 import com.google.common.collect.MapDifference.ValueDifference;
 import com.google.common.collect.Maps;
 import google.registry.model.DatabaseMigrationUtils;
-import google.registry.model.common.DatabaseTransitionSchedule.TransitionId;
 import google.registry.model.registry.label.ReservedList.ReservedListEntry;
 import java.util.Map;
 import java.util.Optional;
@@ -38,32 +36,18 @@ public class ReservedListDualDatabaseDao {

  /** Persist a new reserved list to the database. */
  public static void save(ReservedList reservedList) {
-    if (isDatastore(TransitionId.DOMAIN_LABEL_LISTS)) {
-      ReservedListDatastoreDao.save(reservedList);
-      DatabaseMigrationUtils.suppressExceptionUnlessInTest(
-          () -> ReservedListSqlDao.save(reservedList),
-          "Error saving the reserved list to Cloud SQL.");
-    } else {
      ReservedListSqlDao.save(reservedList);
      DatabaseMigrationUtils.suppressExceptionUnlessInTest(
          () -> ReservedListDatastoreDao.save(reservedList),
          "Error saving the reserved list to Datastore.");
-    }
  }

  /** Delete a reserved list from both databases. */
  public static void delete(ReservedList reservedList) {
-    if (isDatastore(TransitionId.DOMAIN_LABEL_LISTS)) {
-      ReservedListDatastoreDao.delete(reservedList);
-      DatabaseMigrationUtils.suppressExceptionUnlessInTest(
-          () -> ReservedListSqlDao.delete(reservedList),
-          "Error deleting the reserved list from Cloud SQL.");
-    } else {
      ReservedListSqlDao.delete(reservedList);
      DatabaseMigrationUtils.suppressExceptionUnlessInTest(
          () -> ReservedListDatastoreDao.delete(reservedList),
          "Error deleting the reserved list from Datastore.");
-    }
  }

  /**
@@ -72,9 +56,7 @@ public class ReservedListDualDatabaseDao {
   */
  public static Optional<ReservedList> getLatestRevision(String reservedListName) {
    Optional<ReservedList> maybePrimaryList =
-        isDatastore(TransitionId.DOMAIN_LABEL_LISTS)
-            ? ReservedListDatastoreDao.getLatestRevision(reservedListName)
-            : ReservedListSqlDao.getLatestRevision(reservedListName);
+        ReservedListSqlDao.getLatestRevision(reservedListName);
    DatabaseMigrationUtils.suppressExceptionUnlessInTest(
        () -> maybePrimaryList.ifPresent(primaryList -> loadAndCompare(primaryList)),
        "Error comparing reserved lists.");
@@ -83,14 +65,9 @@ public class ReservedListDualDatabaseDao {

  private static void loadAndCompare(ReservedList primaryList) {
    Optional<ReservedList> maybeSecondaryList =
-        isDatastore(TransitionId.DOMAIN_LABEL_LISTS)
-            ? ReservedListSqlDao.getLatestRevision(primaryList.getName())
-            : ReservedListDatastoreDao.getLatestRevision(primaryList.getName());
+        ReservedListDatastoreDao.getLatestRevision(primaryList.getName());
    if (!maybeSecondaryList.isPresent()) {
-      throw new IllegalStateException(
-          String.format(
-              "Reserved list in the secondary database (%s) is empty.",
-              isDatastore(TransitionId.DOMAIN_LABEL_LISTS) ? "Cloud SQL" : "Datastore"));
+      throw new IllegalStateException("Reserved list in Datastore is empty.");
    }
    Map<String, ReservedListEntry> labelsToReservations =
        primaryList.reservedListMap.entrySet().parallelStream()
@@ -110,12 +87,10 @@ public class ReservedListDualDatabaseDao {
        if (diff.entriesDiffering().size() > 10) {
        throw new IllegalStateException(
            String.format(
-                "Unequal reserved lists detected, %s list with revision"
+                "Unequal reserved lists detected, Datastore list with revision"
                    + " id %d has %d different records than the current"
-                    + " primary database list.",
-                isDatastore(TransitionId.DOMAIN_LABEL_LISTS) ? "Cloud SQL" : "Datastore",
-                secondaryList.getRevisionId(),
-                diff.entriesDiffering().size()));
+                    + " Cloud SQL list.",
+                secondaryList.getRevisionId(), diff.entriesDiffering().size()));
      }
          StringBuilder diffMessage = new StringBuilder("Unequal reserved lists detected:\n");
      diff.entriesDiffering().entrySet().stream()
@@ -125,12 +100,9 @@ public class ReservedListDualDatabaseDao {
                ValueDifference<ReservedListEntry> valueDiff = entry.getValue();
                diffMessage.append(
                    String.format(
-                        "Domain label %s has entry %s in %s and entry"
-                            + " %s in the secondary database.\n",
-                        label,
-                        valueDiff.leftValue(),
-                        isDatastore(TransitionId.DOMAIN_LABEL_LISTS) ? "Datastore" : "Cloud SQL",
-                        valueDiff.rightValue()));
+                        "Domain label %s has entry %s in Cloud SQL and entry"
+                            + " %s in the Datastore.\n",
+                        label, valueDiff.leftValue(), valueDiff.rightValue()));
              });
      diff.entriesOnlyOnLeft().entrySet().stream()
          .forEach(
@@ -138,9 +110,7 @@ public class ReservedListDualDatabaseDao {
                String label = entry.getKey();
                diffMessage.append(
                    String.format(
-                        "Domain label %s has entry in %s, but not in the secondary database.\n",
-                        label,
-                        isDatastore(TransitionId.DOMAIN_LABEL_LISTS) ? "Datastore" : "Cloud SQL"));
+                        "Domain label %s has entry in Cloud SQL, but not in Datastore.\n", label));
              });
      diff.entriesOnlyOnRight().entrySet().stream()
          .forEach(
@@ -148,9 +118,7 @@ public class ReservedListDualDatabaseDao {
                String label = entry.getKey();
                diffMessage.append(
                    String.format(
-                        "Domain label %s has entry in %s, but not in the primary database.\n",
-                        label,
-                        isDatastore(TransitionId.DOMAIN_LABEL_LISTS) ? "Cloud SQL" : "Datastore"));
+                        "Domain label %s has entry in Datastore, but not in Cloud SQL.\n", label));
              });
          throw new IllegalStateException(diffMessage.toString());
      }
@@ -30,8 +30,8 @@ import java.nio.file.Files;
 import java.util.List;
 import org.joda.time.DateTime;

-/** Command to create a {@link ReservedList} on Datastore. */
-@Parameters(separators = " =", commandDescription = "Create a ReservedList in Datastore.")
+/** Command to create a {@link ReservedList}. */
+@Parameters(separators = " =", commandDescription = "Create a ReservedList.")
 final class CreateReservedListCommand extends CreateOrUpdateReservedListCommand {

  @VisibleForTesting
@@ -25,10 +25,10 @@ import google.registry.model.registry.label.PremiumListDualDao;
 import javax.annotation.Nullable;

 /**
- * Command to delete a {@link PremiumList} in Datastore. This command will fail if the premium list
- * is currently in use on a tld.
+ * Command to delete a {@link PremiumList}. This command will fail if the premium list is currently
+ * in use on a tld.
 */
-@Parameters(separators = " =", commandDescription = "Delete a PremiumList from Datastore.")
+@Parameters(separators = " =", commandDescription = "Delete a PremiumList.")
 final class DeletePremiumListCommand extends ConfirmingCommand implements CommandWithRemoteApi {

  @Nullable PremiumList premiumList;
@@ -55,7 +55,7 @@ final class DeletePremiumListCommand extends ConfirmingCommand implements Comman

  @Override
  protected String prompt() {
-    return "You are about to delete the premium list: \n" + premiumList;
+    return "You are about to delete the premium list: \n" + premiumList.getName();
  }

  @Override
@@ -22,13 +22,11 @@ import com.google.common.base.Strings;
 import com.googlecode.objectify.Key;
 import google.registry.model.registry.label.ReservedList;
 import google.registry.persistence.VKey;
-import google.registry.util.SystemClock;
 import java.nio.file.Files;
 import java.util.List;
-import org.joda.time.DateTime;

-/** Command to safely update {@link ReservedList} on Datastore. */
-@Parameters(separators = " =", commandDescription = "Update a ReservedList in Datastore.")
+/** Command to safely update {@link ReservedList}. */
+@Parameters(separators = " =", commandDescription = "Update a ReservedList.")
 final class UpdateReservedListCommand extends CreateOrUpdateReservedListCommand {

  @Override
@@ -44,12 +42,10 @@ final class UpdateReservedListCommand extends CreateOrUpdateReservedListCommand
    boolean shouldPublish =
        this.shouldPublish == null ? existingReservedList.getShouldPublish() : this.shouldPublish;
    List<String> allLines = Files.readAllLines(input, UTF_8);
-    DateTime now = new SystemClock().nowUtc();
    ReservedList.Builder updated =
        existingReservedList
            .asBuilder()
            .setReservedListMapFromLines(allLines)
-            .setLastUpdateTime(now)
            .setShouldPublish(shouldPublish);
    reservedList = updated.build();
    // only call stageEntityChange if there are changes in entries
@@ -15,8 +15,7 @@
 package google.registry.tools.server;

 import static com.google.common.collect.ImmutableSortedSet.toImmutableSortedSet;
-import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
-import static google.registry.persistence.transaction.TransactionManagerUtil.transactIfJpaTm;
+import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
 import static google.registry.request.Action.Method.GET;
 import static google.registry.request.Action.Method.POST;

@@ -51,14 +50,15 @@ public final class ListPremiumListsAction extends ListObjectsAction<PremiumList>

  @Override
  public ImmutableSet<PremiumList> loadObjects() {
-    return transactIfJpaTm(
-        () ->
-            tm().loadAllOf(PremiumList.class).stream()
-                .map(PremiumList::getName)
-                .map(PremiumListDualDao::getLatestRevision)
-                .filter(Optional::isPresent)
-                .map(Optional::get)
-                .peek(list -> Hibernate.initialize(list.getLabelsToPrices()))
-                .collect(toImmutableSortedSet(Comparator.comparing(PremiumList::getName))));
+    return jpaTm()
+        .transact(
+            () ->
+                jpaTm().loadAllOf(PremiumList.class).stream()
+                    .map(PremiumList::getName)
+                    .map(PremiumListDualDao::getLatestRevision)
+                    .filter(Optional::isPresent)
+                    .map(Optional::get)
+                    .peek(list -> Hibernate.initialize(list.getLabelsToPrices()))
+                    .collect(toImmutableSortedSet(Comparator.comparing(PremiumList::getName))));
  }
 }