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.
This commit is contained in:
gbrodman 2022-08-05 17:29:59 -04:00 committed by GitHub
parent 53426a34d1
commit 1a4b1c45bd
12 changed files with 154 additions and 18 deletions

View file

@ -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);

View file

@ -155,6 +155,7 @@ public class RegistryConfigSettings {
public boolean eppResourceCachingEnabled;
public int eppResourceCachingSeconds;
public int eppResourceMaxCachedEntries;
public int claimsListCachingSeconds;
}
/** Configuration for ICANN monthly reporting. */

View file

@ -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:

View file

@ -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.
*
* <p>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

View file

@ -90,12 +90,13 @@ public final class ReservedList
*
* <p>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".

View file

@ -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 {
* <p>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.
*
* <p>Unlike the cache below, this is guaranteed to contain all mappings from labels to claim keys
* if it is not null.
*/
@Insignificant @Transient ImmutableMap<String, String> labelsToKeys;
/**
* A not-necessarily-complete cache of labels to claim keys.
*
* <p>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.
*
* <p>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<String, Optional<String>> 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<String> 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.
*
* <p>This attempts to load from the base {@link #labelsToKeys} if possible, otherwise it will
* query the database for the entry requested.
*/
private Optional<String> 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<String, String> labelsToKeys) {
ClaimsList instance = new ClaimsList();

View file

@ -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.
*
* <p>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<Class<ClaimsListDao>, ClaimsList> CACHE =
createCache(getClaimsListCacheDuration());
@VisibleForTesting
public static void setCacheForTest(Optional<Duration> expiry) {
Duration effectiveExpiry = expiry.orElse(getClaimsListCacheDuration());
CACHE = createCache(effectiveExpiry);
}
private static LoadingCache<Class<ClaimsListDao>, 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(
() -> {

View file

@ -30,8 +30,8 @@ import java.nio.file.Paths;
/**
* A command to download the current claims list.
*
* <p>This is not the original file we fetched from TMCH. It is just a representation of what we
* are currently storing in Datastore.
* <p>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 {

View file

@ -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<DomainClaimsCheckFlow, Domain> {
@RegisterExtension
public final TestCacheExtension testCacheExtension =
new TestCacheExtension.Builder().withClaimsListCache(Duration.ofHours(6)).build();
DomainClaimsCheckFlowTest() {
setEppInput("domain_check_claims.xml");
}

View file

@ -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());

View file

@ -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()));
}

View file

@ -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");