From 1a4b1c45bd819a1945d0f74a322f6aaaad077180 Mon Sep 17 00:00:00 2001 From: gbrodman Date: Fri, 5 Aug 2022 17:29:59 -0400 Subject: [PATCH] Add caches to ClaimsListDao and ClaimsList (#1731) We cache the ClaimsList Java object for six hours (we don't expect it to change frequently, and the cron job to update it only runs every twelve hours). Subsequent calls to ClaimsListDao::get will return the cached value. Within the ClaimsList Java object itself, we cache any labels that we have retrieved. While we already have a form of a cache here in the "labelsToKeys" map, that only handles situations where we've loaded the entire map from the database. We want to have a non-guaranteed cache in order to make repeated calls to getClaimKey fast. --- .../registry/config/RegistryConfig.java | 5 ++ .../config/RegistryConfigSettings.java | 1 + .../registry/config/files/default-config.yaml | 4 ++ .../model/tld/label/PremiumListDao.java | 2 +- .../model/tld/label/ReservedList.java | 5 +- .../registry/model/tmch/ClaimsList.java | 54 ++++++++++++++----- .../registry/model/tmch/ClaimsListDao.java | 34 +++++++++++- .../registry/tools/GetClaimsListCommand.java | 4 +- .../domain/DomainClaimsCheckFlowTest.java | 7 +++ .../model/tmch/ClaimsListDaoTest.java | 43 +++++++++++++++ .../registry/testing/TestCacheExtension.java | 7 +++ .../registry/tools/EppLifecycleToolsTest.java | 6 +++ 12 files changed, 154 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/google/registry/config/RegistryConfig.java b/core/src/main/java/google/registry/config/RegistryConfig.java index 97fda5615..3280d42ed 100644 --- a/core/src/main/java/google/registry/config/RegistryConfig.java +++ b/core/src/main/java/google/registry/config/RegistryConfig.java @@ -1427,6 +1427,11 @@ public final class RegistryConfig { return CONFIG_SETTINGS.get().caching.eppResourceMaxCachedEntries; } + /** Returns the amount of time that a particular claims list should be cached. */ + public static java.time.Duration getClaimsListCacheDuration() { + return java.time.Duration.ofSeconds(CONFIG_SETTINGS.get().caching.claimsListCachingSeconds); + } + /** Returns the email address that outgoing emails from the app are sent from. */ public static InternetAddress getGSuiteOutgoingEmailAddress() { return parseEmailAddress(CONFIG_SETTINGS.get().gSuite.outgoingEmailAddress); diff --git a/core/src/main/java/google/registry/config/RegistryConfigSettings.java b/core/src/main/java/google/registry/config/RegistryConfigSettings.java index 0f3c029b5..be48858e2 100644 --- a/core/src/main/java/google/registry/config/RegistryConfigSettings.java +++ b/core/src/main/java/google/registry/config/RegistryConfigSettings.java @@ -155,6 +155,7 @@ public class RegistryConfigSettings { public boolean eppResourceCachingEnabled; public int eppResourceCachingSeconds; public int eppResourceMaxCachedEntries; + public int claimsListCachingSeconds; } /** Configuration for ICANN monthly reporting. */ diff --git a/core/src/main/java/google/registry/config/files/default-config.yaml b/core/src/main/java/google/registry/config/files/default-config.yaml index 49c8a29a9..36118a77c 100644 --- a/core/src/main/java/google/registry/config/files/default-config.yaml +++ b/core/src/main/java/google/registry/config/files/default-config.yaml @@ -294,6 +294,10 @@ caching: # have to be very large to achieve the vast majority of possible gains. eppResourceMaxCachedEntries: 500 + # Length of time that a claims list will be cached after retrieval. A fairly + # long duration is acceptable because claims lists don't change frequently. + claimsListCachingSeconds: 21600 # six hours + oAuth: # OAuth scopes to detect on access tokens. Superset of requiredOauthScopes. availableOauthScopes: diff --git a/core/src/main/java/google/registry/model/tld/label/PremiumListDao.java b/core/src/main/java/google/registry/model/tld/label/PremiumListDao.java index 6118ff8e1..021aa81f1 100644 --- a/core/src/main/java/google/registry/model/tld/label/PremiumListDao.java +++ b/core/src/main/java/google/registry/model/tld/label/PremiumListDao.java @@ -73,7 +73,7 @@ public class PremiumListDao { } /** - * In-memory price cache for for a given premium list revision and domain label. + * In-memory price cache for a given premium list revision and domain label. * *

Note that premium list revision ids are globally unique, so this cache is specific to a * given premium list. Premium list entries might not be present, as indicated by the Optional diff --git a/core/src/main/java/google/registry/model/tld/label/ReservedList.java b/core/src/main/java/google/registry/model/tld/label/ReservedList.java index 75f1ee129..297409fb0 100644 --- a/core/src/main/java/google/registry/model/tld/label/ReservedList.java +++ b/core/src/main/java/google/registry/model/tld/label/ReservedList.java @@ -90,12 +90,13 @@ public final class ReservedList * *

We need to persist the list entries, but only on the initial insert (not on update) since * the entries themselves never get changed, so we only annotate it with {@link PostPersist}, not - * {@link PostUpdate}. + * PostUpdate. */ @PostPersist void postPersist() { if (reservedListMap != null) { - reservedListMap.values().stream() + reservedListMap + .values() .forEach( entry -> { // We can safely change the revision id since it's "Insignificant". diff --git a/core/src/main/java/google/registry/model/tmch/ClaimsList.java b/core/src/main/java/google/registry/model/tmch/ClaimsList.java index 4cc23702f..f4c428d1e 100644 --- a/core/src/main/java/google/registry/model/tmch/ClaimsList.java +++ b/core/src/main/java/google/registry/model/tmch/ClaimsList.java @@ -20,7 +20,10 @@ import static com.google.common.collect.ImmutableMap.toImmutableMap; import static google.registry.persistence.transaction.QueryComposer.Comparator.EQ; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; +import com.github.benmanes.caffeine.cache.LoadingCache; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableMap; +import google.registry.model.CacheUtils; import google.registry.model.CreateAutoTimestamp; import google.registry.model.ImmutableObject; import java.util.Map; @@ -81,9 +84,26 @@ public class ClaimsList extends ImmutableObject { *

This field requires special treatment since we want to lazy load it. We have to remove it * from the immutability contract so we can modify it after construction and we have to handle the * database processing on our own so we can detach it after load. + * + *

Unlike the cache below, this is guaranteed to contain all mappings from labels to claim keys + * if it is not null. */ @Insignificant @Transient ImmutableMap labelsToKeys; + /** + * A not-necessarily-complete cache of labels to claim keys. + * + *

At any point in time this might or might not contain none, some, or all of the mappings from + * labels to claim keys. Exists so that repeated calls to {@link #getClaimKey(String)} can be + * quick. + * + *

Note: this cache has no expiration because while claims list revisions can be added over + * time, each instance of a claims list is immutable. + */ + @Insignificant @Transient @VisibleForTesting + final LoadingCache> claimKeyCache = + CacheUtils.newCacheBuilder().build(this::getClaimKeyUncached); + @PreRemove void preRemove() { jpaTm() @@ -139,18 +159,7 @@ public class ClaimsList extends ImmutableObject { * entries and cache them locally. */ public Optional getClaimKey(String label) { - if (labelsToKeys != null) { - return Optional.ofNullable(labelsToKeys.get(label)); - } - return jpaTm() - .transact( - () -> - jpaTm() - .createQueryComposer(ClaimsEntry.class) - .where("revisionId", EQ, revisionId) - .where("domainLabel", EQ, label) - .first() - .map(ClaimsEntry::getClaimKey)); + return claimKeyCache.get(label); } /** @@ -192,6 +201,27 @@ public class ClaimsList extends ImmutableObject { return labelsToKeys.size(); } + /** + * Returns the claim key for a given domain if there is one, empty otherwise. + * + *

This attempts to load from the base {@link #labelsToKeys} if possible, otherwise it will + * query the database for the entry requested. + */ + private Optional getClaimKeyUncached(String label) { + if (labelsToKeys != null) { + return Optional.ofNullable(labelsToKeys.get(label)); + } + return jpaTm() + .transact( + () -> + jpaTm() + .createQueryComposer(ClaimsEntry.class) + .where("revisionId", EQ, revisionId) + .where("domainLabel", EQ, label) + .first() + .map(ClaimsEntry::getClaimKey)); + } + public static ClaimsList create( DateTime tmdbGenerationTime, ImmutableMap labelsToKeys) { ClaimsList instance = new ClaimsList(); diff --git a/core/src/main/java/google/registry/model/tmch/ClaimsListDao.java b/core/src/main/java/google/registry/model/tmch/ClaimsListDao.java index ff7df8671..69697a078 100644 --- a/core/src/main/java/google/registry/model/tmch/ClaimsListDao.java +++ b/core/src/main/java/google/registry/model/tmch/ClaimsListDao.java @@ -14,25 +14,57 @@ package google.registry.model.tmch; +import static google.registry.config.RegistryConfig.getClaimsListCacheDuration; import static google.registry.persistence.transaction.QueryComposer.Comparator.EQ; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.util.DateTimeUtils.START_OF_TIME; +import com.github.benmanes.caffeine.cache.LoadingCache; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableMap; +import google.registry.model.CacheUtils; +import java.time.Duration; +import java.util.Optional; /** Data access object for {@link ClaimsList}. */ public class ClaimsListDao { + /** + * Cache of the {@link ClaimsList} instance. + * + *

The key is meaningless since we only have one active claims list, this is essentially a + * memoizing Supplier that can be reset. + */ + @VisibleForTesting + static LoadingCache, ClaimsList> CACHE = + createCache(getClaimsListCacheDuration()); + + @VisibleForTesting + public static void setCacheForTest(Optional expiry) { + Duration effectiveExpiry = expiry.orElse(getClaimsListCacheDuration()); + CACHE = createCache(effectiveExpiry); + } + + private static LoadingCache, ClaimsList> createCache(Duration expiry) { + return CacheUtils.newCacheBuilder(expiry).build(ignored -> ClaimsListDao.getUncached()); + } + /** Saves the given {@link ClaimsList} to Cloud SQL. */ public static void save(ClaimsList claimsList) { jpaTm().transact(() -> jpaTm().insert(claimsList)); + CACHE.put(ClaimsListDao.class, claimsList); + } + + /** Returns the most recent revision of the {@link ClaimsList} from the cache. */ + public static ClaimsList get() { + return CACHE.get(ClaimsListDao.class); } /** * Returns the most recent revision of the {@link ClaimsList} in SQL or an empty list if it * doesn't exist. */ - public static ClaimsList get() { + private static ClaimsList getUncached() { return jpaTm() .transact( () -> { diff --git a/core/src/main/java/google/registry/tools/GetClaimsListCommand.java b/core/src/main/java/google/registry/tools/GetClaimsListCommand.java index 67dcd5e6f..272b0aa9a 100644 --- a/core/src/main/java/google/registry/tools/GetClaimsListCommand.java +++ b/core/src/main/java/google/registry/tools/GetClaimsListCommand.java @@ -30,8 +30,8 @@ import java.nio.file.Paths; /** * A command to download the current claims list. * - *

This is not the original file we fetched from TMCH. It is just a representation of what we - * are currently storing in Datastore. + *

This is not the original file we fetched from TMCH. It is just a representation of what we are + * currently storing in SQL. */ @Parameters(separators = " =", commandDescription = "Download the current claims list") final class GetClaimsListCommand implements CommandWithRemoteApi { diff --git a/core/src/test/java/google/registry/flows/domain/DomainClaimsCheckFlowTest.java b/core/src/test/java/google/registry/flows/domain/DomainClaimsCheckFlowTest.java index b49904ac0..c1c5a3a90 100644 --- a/core/src/test/java/google/registry/flows/domain/DomainClaimsCheckFlowTest.java +++ b/core/src/test/java/google/registry/flows/domain/DomainClaimsCheckFlowTest.java @@ -41,13 +41,20 @@ import google.registry.flows.exceptions.TooManyResourceChecksException; import google.registry.model.domain.Domain; import google.registry.model.tld.Registry; import google.registry.model.tld.Registry.TldState; +import google.registry.testing.TestCacheExtension; +import java.time.Duration; import org.joda.money.Money; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; /** Unit tests for {@link DomainClaimsCheckFlow}. */ public class DomainClaimsCheckFlowTest extends ResourceFlowTestCase { + @RegisterExtension + public final TestCacheExtension testCacheExtension = + new TestCacheExtension.Builder().withClaimsListCache(Duration.ofHours(6)).build(); + DomainClaimsCheckFlowTest() { setEppInput("domain_check_claims.xml"); } diff --git a/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java b/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java index 973dda958..43ccc4aac 100644 --- a/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java +++ b/core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java @@ -15,12 +15,16 @@ package google.registry.model.tmch; import static com.google.common.truth.Truth.assertThat; +import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static org.junit.jupiter.api.Assertions.assertThrows; import com.google.common.collect.ImmutableMap; +import com.google.common.truth.Truth8; import google.registry.persistence.transaction.JpaTestExtensions; import google.registry.persistence.transaction.JpaTestExtensions.JpaIntegrationWithCoverageExtension; import google.registry.testing.FakeClock; +import google.registry.testing.TestCacheExtension; +import java.time.Duration; import javax.persistence.PersistenceException; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -37,6 +41,11 @@ public class ClaimsListDaoTest { .withoutCannedData() .buildIntegrationWithCoverageExtension(); + // Set long persist times on the cache so it can be tested (cache times default to 0 in tests). + @RegisterExtension + public final TestCacheExtension testCacheExtension = + new TestCacheExtension.Builder().withClaimsListCache(Duration.ofHours(6)).build(); + @Test void save_insertsClaimsListSuccessfully() { ClaimsList claimsList = @@ -83,6 +92,40 @@ public class ClaimsListDaoTest { assertClaimsListEquals(newClaimsList, ClaimsListDao.get()); } + @Test + void testDaoCaching_savesAndUpdates() { + assertThat(ClaimsListDao.CACHE.getIfPresent(ClaimsListDao.class)).isNull(); + ClaimsList oldList = + ClaimsList.create(fakeClock.nowUtc(), ImmutableMap.of("label1", "key1", "label2", "key2")); + ClaimsListDao.save(oldList); + assertThat(ClaimsListDao.CACHE.getIfPresent(ClaimsListDao.class)).isEqualTo(oldList); + ClaimsList newList = + ClaimsList.create(fakeClock.nowUtc(), ImmutableMap.of("label3", "key3", "label4", "key4")); + ClaimsListDao.save(newList); + assertThat(ClaimsListDao.CACHE.getIfPresent(ClaimsListDao.class)).isEqualTo(newList); + } + + @Test + void testEntryCaching_savesAndUpdates() { + ClaimsList claimsList = + ClaimsList.create(fakeClock.nowUtc(), ImmutableMap.of("label1", "key1", "label2", "key2")); + // Bypass the DAO to avoid the cache + jpaTm().transact(() -> jpaTm().insert(claimsList)); + ClaimsList fromDatabase = ClaimsListDao.get(); + // At first, we haven't loaded any entries + assertThat(fromDatabase.claimKeyCache.getIfPresent("label1")).isNull(); + Truth8.assertThat(fromDatabase.getClaimKey("label1")).hasValue("key1"); + // After retrieval, the key exists + Truth8.assertThat(fromDatabase.claimKeyCache.getIfPresent("label1")).hasValue("key1"); + assertThat(fromDatabase.claimKeyCache.getIfPresent("label2")).isNull(); + // Loading labels-to-keys should still work + assertThat(fromDatabase.getLabelsToKeys()).containsExactly("label1", "key1", "label2", "key2"); + // We should also cache nonexistent values + assertThat(fromDatabase.claimKeyCache.getIfPresent("nonexistent")).isNull(); + Truth8.assertThat(fromDatabase.getClaimKey("nonexistent")).isEmpty(); + Truth8.assertThat(fromDatabase.claimKeyCache.getIfPresent("nonexistent")).isEmpty(); + } + private void assertClaimsListEquals(ClaimsList left, ClaimsList right) { assertThat(left.getRevisionId()).isEqualTo(right.getRevisionId()); assertThat(left.getTmdbGenerationTime()).isEqualTo(right.getTmdbGenerationTime()); diff --git a/core/src/test/java/google/registry/testing/TestCacheExtension.java b/core/src/test/java/google/registry/testing/TestCacheExtension.java index 1ad79756a..2c5c06673 100644 --- a/core/src/test/java/google/registry/testing/TestCacheExtension.java +++ b/core/src/test/java/google/registry/testing/TestCacheExtension.java @@ -19,6 +19,7 @@ import com.google.common.collect.Maps; import google.registry.model.EppResource; import google.registry.model.index.ForeignKeyIndex; import google.registry.model.tld.label.PremiumListDao; +import google.registry.model.tmch.ClaimsListDao; import java.time.Duration; import java.util.Map; import java.util.Optional; @@ -75,6 +76,12 @@ public class TestCacheExtension implements BeforeEachCallback, AfterEachCallback return this; } + public Builder withClaimsListCache(Duration expiry) { + cacheHandlerMap.put( + "ClaimsListDao.CACHE", new TestCacheHandler(ClaimsListDao::setCacheForTest, expiry)); + return this; + } + public TestCacheExtension build() { return new TestCacheExtension(ImmutableList.copyOf(cacheHandlerMap.values())); } diff --git a/core/src/test/java/google/registry/tools/EppLifecycleToolsTest.java b/core/src/test/java/google/registry/tools/EppLifecycleToolsTest.java index eed5f31e7..a149ee570 100644 --- a/core/src/test/java/google/registry/tools/EppLifecycleToolsTest.java +++ b/core/src/test/java/google/registry/tools/EppLifecycleToolsTest.java @@ -29,7 +29,9 @@ import google.registry.model.domain.Domain; import google.registry.model.domain.DomainHistory; import google.registry.model.reporting.HistoryEntry.Type; import google.registry.testing.AppEngineExtension; +import google.registry.testing.TestCacheExtension; import google.registry.util.Clock; +import java.time.Duration; import java.util.List; import org.joda.money.Money; import org.joda.time.DateTime; @@ -44,6 +46,10 @@ class EppLifecycleToolsTest extends EppTestCase { final AppEngineExtension appEngine = AppEngineExtension.builder().withClock(clock).withCloudSql().withTaskQueue().build(); + @RegisterExtension + public final TestCacheExtension testCacheExtension = + new TestCacheExtension.Builder().withClaimsListCache(Duration.ofHours(6)).build(); + @BeforeEach void beforeEach() { createTlds("example", "tld");