diff --git a/core/src/main/java/google/registry/model/tmch/ClaimsListDualDatabaseDao.java b/core/src/main/java/google/registry/model/tmch/ClaimsListDualDatabaseDao.java index 7b8d31984..20e9fb9f9 100644 --- a/core/src/main/java/google/registry/model/tmch/ClaimsListDualDatabaseDao.java +++ b/core/src/main/java/google/registry/model/tmch/ClaimsListDualDatabaseDao.java @@ -16,9 +16,7 @@ package google.registry.model.tmch; import static google.registry.config.RegistryConfig.getDomainLabelListCacheDuration; import static google.registry.model.CacheUtils.tryMemoizeWithExpiration; -import static google.registry.model.DatabaseMigrationUtils.isDatastore; import static google.registry.model.DatabaseMigrationUtils.suppressExceptionUnlessInTest; -import static google.registry.model.common.DatabaseTransitionSchedule.TransitionId.CLAIMS_LIST; import static google.registry.util.DateTimeUtils.START_OF_TIME; import com.google.common.base.Supplier; @@ -31,12 +29,11 @@ import java.util.Optional; /** * DAO for {@link ClaimsListShard} objects that handles the branching paths for SQL and Datastore. * - *

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). + *

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). * - *

For read actions, we will log if the primary and secondary databases * have different values - * (or if the retrieval from the second database fails). + *

For read actions, we will log if the two databases have different values (or if the retrieval + * from Datastore fails). */ public class ClaimsListDualDatabaseDao { @@ -48,18 +45,12 @@ public class ClaimsListDualDatabaseDao { /** * Saves the given {@link ClaimsListShard} to both the primary and secondary databases, logging - * and skipping errors in the secondary DB. + * and skipping errors in Datastore. */ public static void save(ClaimsListShard claimsList) { - if (isDatastore(CLAIMS_LIST)) { - claimsList.saveToDatastore(); - suppressExceptionUnlessInTest( - () -> ClaimsListSqlDao.save(claimsList), "Error saving ClaimsList to SQL."); - } else { ClaimsListSqlDao.save(claimsList); suppressExceptionUnlessInTest( claimsList::saveToDatastore, "Error saving ClaimsListShard to Datastore."); - } } /** Returns the most recent revision of the {@link ClaimsListShard}, from cache. */ @@ -69,42 +60,31 @@ public class ClaimsListDualDatabaseDao { /** Retrieves and compares the latest revision from the databases. */ private static ClaimsListShard getUncached() { - Optional primaryResult; - if (isDatastore(CLAIMS_LIST)) { - primaryResult = ClaimsListShard.getFromDatastore(); - suppressExceptionUnlessInTest( - () -> { - Optional secondaryResult = ClaimsListSqlDao.get(); - compareClaimsLists(primaryResult, secondaryResult); - }, - "Error loading ClaimsList from SQL."); - } else { - primaryResult = ClaimsListSqlDao.get(); - suppressExceptionUnlessInTest( - () -> { - Optional secondaryResult = ClaimsListShard.getFromDatastore(); - compareClaimsLists(primaryResult, secondaryResult); - }, - "Error loading ClaimsListShard from Datastore."); - } - return primaryResult.orElse(ClaimsListShard.create(START_OF_TIME, ImmutableMap.of())); + Optional cloudSqlResult = ClaimsListSqlDao.get(); + suppressExceptionUnlessInTest( + () -> { + Optional datastoreResult = ClaimsListShard.getFromDatastore(); + compareClaimsLists(cloudSqlResult, datastoreResult); + }, + "Error loading ClaimsListShard from Datastore."); + return cloudSqlResult.orElse(ClaimsListShard.create(START_OF_TIME, ImmutableMap.of())); } private static void compareClaimsLists( - Optional maybePrimary, Optional maybeSecondary) { - if (maybePrimary.isPresent() && !maybeSecondary.isPresent()) { - throw new IllegalStateException("Claims list found in primary DB but not in secondary DB."); + Optional maybeCloudSql, Optional maybeDatastore) { + if (maybeCloudSql.isPresent() && !maybeDatastore.isPresent()) { + throw new IllegalStateException("Claims list found in Cloud SQL but not in Datastore."); } - if (!maybePrimary.isPresent() && maybeSecondary.isPresent()) { - throw new IllegalStateException("Claims list found in secondary DB but not in primary DB."); + if (!maybeCloudSql.isPresent() && maybeDatastore.isPresent()) { + throw new IllegalStateException("Claims list found in Datastore but not in Cloud SQL."); } - if (!maybePrimary.isPresent()) { + if (!maybeCloudSql.isPresent()) { return; } - ClaimsListShard primary = maybePrimary.get(); - ClaimsListShard secondary = maybeSecondary.get(); + ClaimsListShard sqlList = maybeCloudSql.get(); + ClaimsListShard datastoreList = maybeDatastore.get(); MapDifference diff = - Maps.difference(primary.labelsToKeys, secondary.getLabelsToKeys()); + Maps.difference(sqlList.labelsToKeys, datastoreList.getLabelsToKeys()); if (!diff.areEqual()) { if (diff.entriesDiffering().size() + diff.entriesOnlyOnRight().size() @@ -112,9 +92,9 @@ public class ClaimsListDualDatabaseDao { > 10) { throw new IllegalStateException( String.format( - "Unequal claims lists detected, secondary list with revision id %d has %d" - + " different records than the current primary list.", - secondary.getRevisionId(), diff.entriesDiffering().size())); + "Unequal claims lists detected, Datastore list with revision id %d has %d" + + " different records than the current Cloud SQL list.", + datastoreList.getRevisionId(), diff.entriesDiffering().size())); } else { StringBuilder diffMessage = new StringBuilder("Unequal claims lists detected:\n"); diff.entriesDiffering() @@ -122,22 +102,22 @@ public class ClaimsListDualDatabaseDao { (label, valueDiff) -> diffMessage.append( String.format( - "Domain label %s has key %s in the primary DB and key %s " - + "in the secondary DB.\n", + "Domain label %s has key %s in Cloud SQL and key %s " + + "in Datastore.\n", label, valueDiff.leftValue(), valueDiff.rightValue()))); diff.entriesOnlyOnLeft() .forEach( (label, valueDiff) -> diffMessage.append( String.format( - "Domain label %s with key %s only appears in the primary DB.\n", + "Domain label %s with key %s only appears in Cloud SQL.\n", label, valueDiff))); diff.entriesOnlyOnRight() .forEach( (label, valueDiff) -> diffMessage.append( String.format( - "Domain label %s with key %s only appears in the secondary DB.\n", + "Domain label %s with key %s only appears in Datastore.\n", label, valueDiff))); throw new IllegalStateException(diffMessage.toString()); } diff --git a/core/src/test/java/google/registry/model/tmch/ClaimsListDualDatabaseDaoTest.java b/core/src/test/java/google/registry/model/tmch/ClaimsListDualDatabaseDaoTest.java index 09cf3bd1c..1570c071b 100644 --- a/core/src/test/java/google/registry/model/tmch/ClaimsListDualDatabaseDaoTest.java +++ b/core/src/test/java/google/registry/model/tmch/ClaimsListDualDatabaseDaoTest.java @@ -16,23 +16,14 @@ package google.registry.model.tmch; 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.persistence.transaction.TransactionManagerFactory.tm; -import static google.registry.util.DateTimeUtils.START_OF_TIME; import static org.junit.jupiter.api.Assertions.assertThrows; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSortedMap; import google.registry.config.RegistryEnvironment; import google.registry.model.EntityTestCase; -import google.registry.model.common.DatabaseTransitionSchedule; -import google.registry.model.common.DatabaseTransitionSchedule.PrimaryDatabase; -import google.registry.model.common.DatabaseTransitionSchedule.PrimaryDatabaseTransition; -import google.registry.model.common.DatabaseTransitionSchedule.TransitionId; -import google.registry.model.common.TimedTransitionProperty; import google.registry.testing.SystemPropertyExtension; import org.joda.time.Duration; -import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Order; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -44,64 +35,25 @@ public class ClaimsListDualDatabaseDaoTest extends EntityTestCase { @Order(value = Integer.MAX_VALUE) final SystemPropertyExtension systemPropertyExtension = new SystemPropertyExtension(); - @BeforeEach - void beforeEach() { - DatabaseTransitionSchedule schedule = - DatabaseTransitionSchedule.create( - TransitionId.CLAIMS_LIST, - TimedTransitionProperty.fromValueMap( - ImmutableSortedMap.of( - START_OF_TIME, - PrimaryDatabase.DATASTORE, - fakeClock.nowUtc().plusDays(1), - PrimaryDatabase.CLOUD_SQL), - PrimaryDatabaseTransition.class)); - tm().transactNew(() -> ofy().saveWithoutBackup().entity(schedule).now()); - } - - @Test - void testGetList_missingSql() { - createClaimsList().saveToDatastore(); - assertThat(assertThrows(IllegalStateException.class, ClaimsListDualDatabaseDao::get)) - .hasMessageThat() - .isEqualTo("Claims list found in primary DB but not in secondary DB."); - } - @Test void testGetList_missingOfy() { - fakeClock.advanceBy(Duration.standardDays(5)); ClaimsListSqlDao.save(createClaimsList()); assertThat(assertThrows(IllegalStateException.class, ClaimsListDualDatabaseDao::get)) .hasMessageThat() - .isEqualTo("Claims list found in primary DB but not in secondary DB."); - } - - @Test - void testGetList_fromOfy_different() { - createClaimsList().saveToDatastore(); - ClaimsListSqlDao.save( - ClaimsListShard.create(fakeClock.nowUtc(), ImmutableMap.of("foo", "bar"))); - assertThat(assertThrows(IllegalStateException.class, ClaimsListDualDatabaseDao::get)) - .hasMessageThat() - .isEqualTo( - "Unequal claims lists detected:\n" - + "Domain label label1 with key key1 only appears in the primary DB.\n" - + "Domain label label2 with key key2 only appears in the primary DB.\n" - + "Domain label foo with key bar only appears in the secondary DB.\n"); + .isEqualTo("Claims list found in Cloud SQL but not in Datastore."); } @Test void testGetList_fromSql_different() { - fakeClock.advanceBy(Duration.standardDays(5)); ClaimsListShard.create(fakeClock.nowUtc(), ImmutableMap.of("foo", "bar")).saveToDatastore(); ClaimsListSqlDao.save(createClaimsList()); assertThat(assertThrows(IllegalStateException.class, ClaimsListDualDatabaseDao::get)) .hasMessageThat() .isEqualTo( "Unequal claims lists detected:\n" - + "Domain label label1 with key key1 only appears in the primary DB.\n" - + "Domain label label2 with key key2 only appears in the primary DB.\n" - + "Domain label foo with key bar only appears in the secondary DB.\n"); + + "Domain label label1 with key key1 only appears in Cloud SQL.\n" + + "Domain label label2 with key key2 only appears in Cloud SQL.\n" + + "Domain label foo with key bar only appears in Datastore.\n"); } @Test @@ -117,15 +69,6 @@ public class ClaimsListDualDatabaseDaoTest extends EntityTestCase { assertThat(tm().transact(ClaimsListDualDatabaseDao::get).getLabelsToKeys()).isEmpty(); } - @Test - void testGetList_missingSql_notInTest() { - RegistryEnvironment.PRODUCTION.setup(systemPropertyExtension); - createClaimsList().saveToDatastore(); - // Shouldn't fail in production - assertThat(ClaimsListDualDatabaseDao.get().getLabelsToKeys()) - .isEqualTo(createClaimsList().getLabelsToKeys()); - } - @Test void testGetList_missingOfy_notInTest() { RegistryEnvironment.PRODUCTION.setup(systemPropertyExtension);