diff --git a/java/google/registry/config/RegistryConfig.java b/java/google/registry/config/RegistryConfig.java index 1e0d347a8..3da7f9abf 100644 --- a/java/google/registry/config/RegistryConfig.java +++ b/java/google/registry/config/RegistryConfig.java @@ -1018,6 +1018,10 @@ public final class RegistryConfig { * asynchronously fails the delete). Without this delay, the mapreduce might have started before * the domain flow committed, and could potentially miss the reference. * + *

If you are using EPP resource caching (eppResourceCachingEnabled in YAML), then this + * duration should also be longer than that cache duration (eppResourceCachingSeconds). + * + * @see google.registry.config.RegistryConfigSettings.Caching * @see google.registry.flows.async.AsyncFlowEnqueuer */ @Provides @@ -1324,6 +1328,27 @@ public final class RegistryConfig { return CONFIG_SETTINGS.get().caching.staticPremiumListMaxCachedEntries; } + public static boolean isEppResourceCachingEnabled() { + return CONFIG_SETTINGS.get().caching.eppResourceCachingEnabled; + } + + @VisibleForTesting + public static void overrideIsEppResourceCachingEnabledForTesting(boolean enabled) { + CONFIG_SETTINGS.get().caching.eppResourceCachingEnabled = enabled; + } + + /** + * Returns the amount of time an EPP resource or key should be cached in memory before expiring. + */ + public static Duration getEppResourceCachingDuration() { + return Duration.standardSeconds(CONFIG_SETTINGS.get().caching.eppResourceCachingSeconds); + } + + /** Returns the maximum number of EPP resources and keys to keep in in-memory cache. */ + public static int getEppResourceMaxCachedEntries() { + return CONFIG_SETTINGS.get().caching.eppResourceMaxCachedEntries; + } + /** Returns the email address that outgoing emails from the app are sent from. */ public static String getGSuiteOutgoingEmailAddress() { return CONFIG_SETTINGS.get().gSuite.outgoingEmailAddress; diff --git a/java/google/registry/config/RegistryConfigSettings.java b/java/google/registry/config/RegistryConfigSettings.java index bda47062c..32d233cbc 100644 --- a/java/google/registry/config/RegistryConfigSettings.java +++ b/java/google/registry/config/RegistryConfigSettings.java @@ -110,6 +110,9 @@ public class RegistryConfigSettings { public int domainLabelCachingSeconds; public int singletonCachePersistSeconds; public int staticPremiumListMaxCachedEntries; + public boolean eppResourceCachingEnabled; + public int eppResourceCachingSeconds; + public int eppResourceMaxCachedEntries; } /** Configuration for ICANN monthly reporting. */ diff --git a/java/google/registry/config/files/default-config.yaml b/java/google/registry/config/files/default-config.yaml index 582c811f1..b8f497f67 100644 --- a/java/google/registry/config/files/default-config.yaml +++ b/java/google/registry/config/files/default-config.yaml @@ -141,6 +141,28 @@ caching: # premium price entries that exist. staticPremiumListMaxCachedEntries: 200000 + # Whether to enable caching of EPP resource entities and keys. Enabling this + # caching allows for much higher domain create/update throughput when hosts + # and/or contacts are being frequently used (which is commonly the case). + # However, this may introduce transactional inconsistencies, such as allowing + # hosts or contacts to be used that are actually deleted (though in practice + # this will only happen for non-widely-used entities). Only set this to true + # if you need the performance, i.e. if you need >10 domain mutations per + # frequently used contact or host. This situation is typically caused by + # registrars reusing the same contact/host across many operations, e.g. a + # privacy/proxy contact or a common host pointing to a registrar-run + # nameserver. + eppResourceCachingEnabled: false + + # Length of time that EPP resource entities and keys are cached in memory + # before expiring. + eppResourceCachingSeconds: 60 + + # The maximum number of EPP resource entities and keys to cache in memory. + # LoadingCache evicts rarely-used keys first, so in practice this does not + # have to be very large to achieve the vast majority of possible gains. + eppResourceMaxCachedEntries: 500 + oAuth: # OAuth scopes to detect on access tokens. Superset of requiredOauthScopes. availableOauthScopes: diff --git a/java/google/registry/config/files/nomulus-config-unittest.yaml b/java/google/registry/config/files/nomulus-config-unittest.yaml index 34cb4ad11..6076d87e0 100644 --- a/java/google/registry/config/files/nomulus-config-unittest.yaml +++ b/java/google/registry/config/files/nomulus-config-unittest.yaml @@ -18,6 +18,8 @@ caching: domainLabelCachingSeconds: 0 singletonCachePersistSeconds: 0 staticPremiumListMaxCachedEntries: 50 + eppResourceCachingEnabled: true + eppResourceCachingSeconds: 0 braintree: merchantAccountIdsMap: diff --git a/java/google/registry/flows/domain/DomainFlowUtils.java b/java/google/registry/flows/domain/DomainFlowUtils.java index 3b18df8a2..d1a273092 100644 --- a/java/google/registry/flows/domain/DomainFlowUtils.java +++ b/java/google/registry/flows/domain/DomainFlowUtils.java @@ -17,7 +17,6 @@ package google.registry.flows.domain; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Predicates.equalTo; -import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableMap.toImmutableMap; import static com.google.common.collect.Iterables.any; import static com.google.common.collect.Sets.difference; @@ -275,12 +274,7 @@ public class DomainFlowUtils { nullToEmpty(contacts).stream().map(DesignatedContact::getContactKey).forEach(keysToLoad::add); Optional.ofNullable(registrant).ifPresent(keysToLoad::add); keysToLoad.addAll(nullToEmpty(nameservers)); - // This cast is safe because, in Objectify, Key can also be - // treated as a Key. - @SuppressWarnings("unchecked") - ImmutableList> typedKeys = - keysToLoad.build().stream().map(key -> (Key) key).collect(toImmutableList()); - verifyNotInPendingDelete(ofy().load().keys(typedKeys).values()); + verifyNotInPendingDelete(EppResource.loadCached(keysToLoad.build()).values()); } private static void verifyNotInPendingDelete(Iterable resources) diff --git a/java/google/registry/model/EppResource.java b/java/google/registry/model/EppResource.java index 59e6c3e4b..672e3295f 100644 --- a/java/google/registry/model/EppResource.java +++ b/java/google/registry/model/EppResource.java @@ -16,23 +16,39 @@ package google.registry.model; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.Sets.difference; import static com.google.common.collect.Sets.union; +import static google.registry.config.RegistryConfig.getEppResourceCachingDuration; +import static google.registry.config.RegistryConfig.getEppResourceMaxCachedEntries; +import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.util.CollectionUtils.nullToEmpty; import static google.registry.util.CollectionUtils.nullToEmptyImmutableCopy; import static google.registry.util.DateTimeUtils.END_OF_TIME; +import static java.util.concurrent.TimeUnit.MILLISECONDS; import com.google.common.annotations.VisibleForTesting; +import com.google.common.cache.CacheBuilder; +import com.google.common.cache.CacheLoader; +import com.google.common.cache.LoadingCache; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; +import com.google.common.collect.Streams; import com.googlecode.objectify.Key; import com.googlecode.objectify.annotation.Id; import com.googlecode.objectify.annotation.Index; +import google.registry.config.RegistryConfig; import google.registry.model.eppcommon.StatusValue; import google.registry.model.ofy.CommitLogManifest; import google.registry.model.transfer.TransferData; +import google.registry.util.NonFinalForTesting; +import java.util.Map; +import java.util.Map.Entry; import java.util.Optional; import java.util.Set; +import java.util.concurrent.ExecutionException; import org.joda.time.DateTime; /** An EPP entity object (i.e. a domain, application, contact, or host). */ @@ -301,4 +317,71 @@ public abstract class EppResource extends BackupGroupRoot implements Buildable { return ImmutableObject.cloneEmptyToNull(super.build()); } } + + static final CacheLoader, EppResource> CACHE_LOADER = + new CacheLoader, EppResource>() { + + @Override + public EppResource load(Key key) { + return ofy().doTransactionless(() -> ofy().load().key(key).now()); + } + + @Override + public Map, EppResource> loadAll( + Iterable> keys) { + return ofy().doTransactionless(() -> loadMultiple(keys)); + } + }; + + /** + * A limited size, limited time cache for EPP resource entities. + * + *

This is only used to cache contacts and hosts for the purposes of checking whether they are + * deleted or in pending delete during a few domain flows. Any operations on contacts and hosts + * directly should of course never use the cache. + */ + @NonFinalForTesting + static LoadingCache, EppResource> cacheEppResources = + CacheBuilder.newBuilder() + .expireAfterWrite(getEppResourceCachingDuration().getMillis(), MILLISECONDS) + .maximumSize(getEppResourceMaxCachedEntries()) + .build(CACHE_LOADER); + + private static ImmutableMap, EppResource> loadMultiple( + Iterable> keys) { + // This cast is safe because, in Objectify, Key can also be + // treated as a Key. + @SuppressWarnings("unchecked") + ImmutableList> typedKeys = + Streams.stream(keys).map(key -> (Key) key).collect(toImmutableList()); + + // Typing shenanigans are required to return the map with the correct required generic type. + return ofy() + .load() + .keys(typedKeys) + .entrySet() + .stream() + .collect( + ImmutableMap + ., EppResource>, Key, EppResource> + toImmutableMap(Entry::getKey, Entry::getValue)); + } + + /** + * Loads a given EppResource by its key using the cache (if enabled). + * + *

Don't use this unless you really need it for performance reasons, and be sure that you are + * OK with the trade-offs in loss of transactional consistency. + */ + public static ImmutableMap, EppResource> loadCached( + Iterable> keys) { + if (!RegistryConfig.isEppResourceCachingEnabled()) { + return loadMultiple(keys); + } + try { + return cacheEppResources.getAll(keys); + } catch (ExecutionException e) { + throw new RuntimeException("Error loading cached EppResources", e.getCause()); + } + } } diff --git a/java/google/registry/model/domain/DomainCommand.java b/java/google/registry/model/domain/DomainCommand.java index c879698e1..181ded544 100644 --- a/java/google/registry/model/domain/DomainCommand.java +++ b/java/google/registry/model/domain/DomainCommand.java @@ -19,7 +19,6 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.collect.Iterables.getOnlyElement; import static com.google.common.collect.Maps.transformValues; import static com.google.common.collect.Sets.difference; -import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.util.CollectionUtils.difference; import static google.registry.util.CollectionUtils.forceEmptyToNull; import static google.registry.util.CollectionUtils.nullSafeImmutableCopy; @@ -391,7 +390,7 @@ public class DomainCommand { clone.registrant = clone.registrantContactId == null ? null : getOnlyElement( - loadByForeignKey( + loadByForeignKeys( ImmutableSet.of(clone.registrantContactId), ContactResource.class, now) .values()); return clone; @@ -420,7 +419,7 @@ public class DomainCommand { return null; } return ImmutableSet.copyOf( - loadByForeignKey(fullyQualifiedHostNames, HostResource.class, now).values()); + loadByForeignKeys(fullyQualifiedHostNames, HostResource.class, now).values()); } private static Set linkContacts( @@ -433,7 +432,7 @@ public class DomainCommand { foreignKeys.add(contact.contactId); } ImmutableMap> loadedContacts = - loadByForeignKey(foreignKeys.build(), ContactResource.class, now); + loadByForeignKeys(foreignKeys.build(), ContactResource.class, now); ImmutableSet.Builder linkedContacts = new ImmutableSet.Builder<>(); for (ForeignKeyedDesignatedContact contact : contacts) { linkedContacts.add(DesignatedContact.create( @@ -442,12 +441,11 @@ public class DomainCommand { return linkedContacts.build(); } - /** Load keys to resources by their foreign keys. */ - private static ImmutableMap> loadByForeignKey( + /** Loads keys to EPP resources by their foreign keys. */ + private static ImmutableMap> loadByForeignKeys( final Set foreignKeys, final Class clazz, final DateTime now) throws InvalidReferencesException { - Map> fkis = - ofy().doTransactionless(() -> ForeignKeyIndex.load(clazz, foreignKeys, now)); + Map> fkis = ForeignKeyIndex.loadCached(clazz, foreignKeys, now); if (!fkis.keySet().equals(foreignKeys)) { throw new InvalidReferencesException( clazz, ImmutableSet.copyOf(difference(foreignKeys, fkis.keySet()))); diff --git a/java/google/registry/model/index/ForeignKeyIndex.java b/java/google/registry/model/index/ForeignKeyIndex.java index d27f9198d..4a14f15c3 100644 --- a/java/google/registry/model/index/ForeignKeyIndex.java +++ b/java/google/registry/model/index/ForeignKeyIndex.java @@ -14,23 +14,37 @@ package google.registry.model.index; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.Maps.filterValues; +import static google.registry.config.RegistryConfig.getEppResourceCachingDuration; +import static google.registry.config.RegistryConfig.getEppResourceMaxCachedEntries; import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.util.TypeUtils.instantiate; +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import com.google.common.cache.CacheBuilder; +import com.google.common.cache.CacheLoader; +import com.google.common.cache.LoadingCache; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Maps; +import com.google.common.collect.Streams; import com.googlecode.objectify.Key; import com.googlecode.objectify.annotation.Entity; import com.googlecode.objectify.annotation.Id; import com.googlecode.objectify.annotation.Index; +import google.registry.config.RegistryConfig; import google.registry.model.BackupGroupRoot; import google.registry.model.EppResource; import google.registry.model.annotations.ReportedOn; import google.registry.model.contact.ContactResource; import google.registry.model.domain.DomainResource; import google.registry.model.host.HostResource; +import google.registry.util.NonFinalForTesting; import java.util.Map; +import java.util.Optional; +import java.util.concurrent.ExecutionException; import javax.annotation.Nullable; import org.joda.time.DateTime; @@ -96,7 +110,6 @@ public abstract class ForeignKeyIndex extends BackupGroup return topReference; } - @SuppressWarnings("unchecked") public static Class> mapToFkiClass( Class resourceClass) { @@ -167,4 +180,87 @@ public abstract class ForeignKeyIndex extends BackupGroup ofy().load().type(mapToFkiClass(clazz)).ids(foreignKeys), (ForeignKeyIndex fki) -> now.isBefore(fki.deletionTime)); } + + static final CacheLoader>, Optional>> CACHE_LOADER = + new CacheLoader>, Optional>>() { + + @Override + public Optional> load(Key> key) { + return Optional.ofNullable(ofy().doTransactionless(() -> ofy().load().key(key).now())); + } + + @Override + public Map>, Optional>> loadAll( + Iterable>> keys) { + ImmutableSet>> typedKeys = ImmutableSet.copyOf(keys); + Map>, ForeignKeyIndex> existingFkis = + ofy().doTransactionless(() -> ofy().load().keys(typedKeys)); + // ofy() omits keys that don't have values in Datastore, so re-add them in + // here with Optional.empty() values. + return Maps.asMap( + typedKeys, + (Key> key) -> + Optional.ofNullable(existingFkis.getOrDefault(key, null))); + } + }; + + /** + * A limited size, limited time cache for foreign key entities. + * + *

This is only used to cache foreign key entities for the purposes of checking whether they + * exist (and if so, what entity they point to) during a few domain flows. Any other operations on + * foreign keys should not use this cache. + * + *

Note that the value type of this cache is Optional because the foreign keys in question are + * coming from external commands, and thus don't necessarily represent entities in our system that + * actually exist. So we cache the fact that they *don't* exist by using Optional.empty(), and + * then several layers up the EPP command will fail with an error message like "The contact with + * given IDs (blah) don't exist." + */ + @NonFinalForTesting + static LoadingCache>, Optional>> + cacheForeignKeyIndexes = + CacheBuilder.newBuilder() + .expireAfterWrite(getEppResourceCachingDuration().getMillis(), MILLISECONDS) + .maximumSize(getEppResourceMaxCachedEntries()) + .build(CACHE_LOADER); + + /** + * Load a list of {@link ForeignKeyIndex} instances by class and id strings that are active at or + * after the specified moment in time, using the cache if enabled. + * + *

The returned map will omit any keys for which the {@link ForeignKeyIndex} doesn't exist or + * has been soft deleted. + * + *

Don't use the cached version of this method unless you really need it for performance + * reasons, and are OK with the trade-offs in loss of transactional consistency. + */ + public static Map> loadCached( + Class clazz, Iterable foreignKeys, final DateTime now) { + if (!RegistryConfig.isEppResourceCachingEnabled()) { + return ofy().doTransactionless(() -> load(clazz, foreignKeys, now)); + } + ImmutableList>> fkiKeys = + Streams.stream(foreignKeys) + .map(fk -> Key.>create(mapToFkiClass(clazz), fk)) + .collect(toImmutableList()); + try { + // This cast is safe because when we loaded ForeignKeyIndexes above we used type clazz, which + // is scoped to E. + @SuppressWarnings("unchecked") + Map> fkisFromCache = cacheForeignKeyIndexes + .getAll(fkiKeys) + .entrySet() + .stream() + .filter(entry -> entry.getValue().isPresent()) + .filter(entry -> now.isBefore(entry.getValue().get().getDeletionTime())) + .collect( + ImmutableMap.toImmutableMap( + entry -> entry.getKey().getName(), + entry -> (ForeignKeyIndex) entry.getValue().get())); + return fkisFromCache; + } catch (ExecutionException e) { + throw new RuntimeException("Error loading cached ForeignKeyIndexes", e.getCause()); + } + } } diff --git a/javatests/google/registry/flows/domain/DomainCreateFlowTest.java b/javatests/google/registry/flows/domain/DomainCreateFlowTest.java index 7e03e9d3a..e60f6f037 100644 --- a/javatests/google/registry/flows/domain/DomainCreateFlowTest.java +++ b/javatests/google/registry/flows/domain/DomainCreateFlowTest.java @@ -59,6 +59,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; import com.googlecode.objectify.Key; +import google.registry.config.RegistryConfig; import google.registry.flows.EppException; import google.registry.flows.EppException.UnimplementedExtensionException; import google.registry.flows.EppRequestSource; @@ -387,6 +388,13 @@ public class DomainCreateFlowTest extends ResourceFlowTestCase loadHostFki(String hostname) { + return ForeignKeyIndex.load(HostResource.class, hostname, clock.nowUtc()); + } + + private ForeignKeyIndex loadContactFki(String contactId) { + return ForeignKeyIndex.load(ContactResource.class, contactId, clock.nowUtc()); + } + + @Test + public void test_loadCached_cachesNonexistenceOfHosts() throws Exception { + setNonZeroCachingInterval(); + assertThat( + ForeignKeyIndex.loadCached( + HostResource.class, + ImmutableList.of("ns5.example.com", "ns6.example.com"), + clock.nowUtc())) + .isEmpty(); + persistActiveHost("ns4.example.com"); + persistActiveHost("ns5.example.com"); + persistActiveHost("ns6.example.com"); + clock.advanceOneMilli(); + assertThat( + ForeignKeyIndex.loadCached( + HostResource.class, + ImmutableList.of("ns6.example.com", "ns5.example.com", "ns4.example.com"), + clock.nowUtc())) + .containsExactly("ns4.example.com", loadHostFki("ns4.example.com")); + } + + @Test + public void test_loadCached_cachesExistenceOfHosts() throws Exception { + setNonZeroCachingInterval(); + HostResource host1 = persistActiveHost("ns1.example.com"); + HostResource host2 = persistActiveHost("ns2.example.com"); + assertThat( + ForeignKeyIndex.loadCached( + HostResource.class, + ImmutableList.of("ns1.example.com", "ns2.example.com"), + clock.nowUtc())) + .containsExactly("ns1.example.com", loadHostFki("ns1.example.com"), + "ns2.example.com", loadHostFki("ns2.example.com")); + deleteResource(host1); + deleteResource(host2); + persistActiveHost("ns3.example.com"); + assertThat( + ForeignKeyIndex.loadCached( + HostResource.class, + ImmutableList.of("ns3.example.com", "ns2.example.com", "ns1.example.com"), + clock.nowUtc())) + .containsExactly( + "ns1.example.com", loadHostFki("ns1.example.com"), + "ns2.example.com", loadHostFki("ns2.example.com"), + "ns3.example.com", loadHostFki("ns3.example.com")); + } + + @Test + public void test_loadCached_doesntSeeHostChangesWhileCacheIsValid() throws Exception { + setNonZeroCachingInterval(); + HostResource originalHost = persistActiveHost("ns1.example.com"); + ForeignKeyIndex originalFki = loadHostFki("ns1.example.com"); + clock.advanceOneMilli(); + assertThat( + ForeignKeyIndex.loadCached( + HostResource.class, ImmutableList.of("ns1.example.com"), clock.nowUtc())) + .containsExactly("ns1.example.com", originalFki); + HostResource modifiedHost = + persistResource( + originalHost.asBuilder().setPersistedCurrentSponsorClientId("OtherRegistrar").build()); + clock.advanceOneMilli(); + ForeignKeyIndex newFki = loadHostFki("ns1.example.com"); + assertThat(newFki).isNotEqualTo(originalFki); + assertThat( + EppResourceUtils.loadByForeignKey( + HostResource.class, "ns1.example.com", clock.nowUtc())) + .isEqualTo(modifiedHost); + assertThat( + ForeignKeyIndex.loadCached( + HostResource.class, ImmutableList.of("ns1.example.com"), clock.nowUtc())) + .containsExactly("ns1.example.com", originalFki); + } + + @Test + public void test_loadCached_filtersOutSoftDeletedHosts() throws Exception { + setNonZeroCachingInterval(); + persistActiveHost("ns1.example.com"); + persistDeletedHost("ns2.example.com", clock.nowUtc().minusDays(1)); + assertThat( + ForeignKeyIndex.loadCached( + HostResource.class, + ImmutableList.of("ns1.example.com", "ns2.example.com"), + clock.nowUtc())) + .containsExactly("ns1.example.com", loadHostFki("ns1.example.com")); + } + + @Test + public void test_loadCached_cachesContactFkis() throws Exception { + setNonZeroCachingInterval(); + persistActiveContact("contactid1"); + ForeignKeyIndex fki1 = loadContactFki("contactid1"); + assertThat( + ForeignKeyIndex.loadCached( + ContactResource.class, + ImmutableList.of("contactid1", "contactid2"), + clock.nowUtc())) + .containsExactly("contactid1", fki1); + persistActiveContact("contactid2"); + deleteResource(fki1); + assertThat( + ForeignKeyIndex.loadCached( + ContactResource.class, + ImmutableList.of("contactid1", "contactid2"), + clock.nowUtc())) + .containsExactly("contactid1", fki1); + assertThat( + ForeignKeyIndex.load( + ContactResource.class, + ImmutableList.of("contactid1", "contactid2"), + clock.nowUtc())) + .containsExactly("contactid2", loadContactFki("contactid2")); + } + + private static void setNonZeroCachingInterval() { + ForeignKeyIndex.cacheForeignKeyIndexes = + CacheBuilder.newBuilder().expireAfterWrite(1L, DAYS).build(CACHE_LOADER); + } }