From 9e61f1d6efa1430cb7ab6d1f777788592adf05d9 Mon Sep 17 00:00:00 2001 From: cgoldfeder Date: Tue, 25 Apr 2017 14:50:08 -0700 Subject: [PATCH] Make failfastForCreate for domain and application creates explicitly hit memcache TESTED=For all tests, I added @Cache to DomainBase because otherwise the tests will fail. We aren't ready to do this in prod yet, which is why the tests are still marked @Ignore. The new tests fail if you change line 134 in Ofy to not use memcache and either use the unchanged original DomainCreateFlow code, or use the new inlined code and change loadWithMemcache() to load(). They pass with the new inlined code that calls loadWithMemcache(), as long as the @Cache is added to DomainResource. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=154224748 --- .../flows/domain/DomainFlowUtils.java | 9 +++-- .../registry/model/EppResourceUtils.java | 35 ++++++++++++++-- .../registry/model/index/ForeignKeyIndex.java | 24 ++++++----- .../DomainApplicationCreateFlowTest.java | 40 +++++++++++++++++-- .../flows/domain/DomainCreateFlowTest.java | 37 ++++++++++++++--- 5 files changed, 119 insertions(+), 26 deletions(-) diff --git a/java/google/registry/flows/domain/DomainFlowUtils.java b/java/google/registry/flows/domain/DomainFlowUtils.java index 7aff94af9..aa5103c4f 100644 --- a/java/google/registry/flows/domain/DomainFlowUtils.java +++ b/java/google/registry/flows/domain/DomainFlowUtils.java @@ -22,7 +22,7 @@ import static com.google.common.collect.Sets.difference; import static com.google.common.collect.Sets.intersection; import static com.google.common.collect.Sets.union; import static google.registry.flows.domain.DomainPricingLogic.getMatchingLrpToken; -import static google.registry.model.EppResourceUtils.loadByForeignKey; +import static google.registry.model.EppResourceUtils.loadByForeignKeyWithMemcache; import static google.registry.model.domain.DomainResource.MAX_REGISTRATION_YEARS; import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.model.registry.Registries.findTldForName; @@ -800,8 +800,11 @@ public class DomainFlowUtils { DomainResource domain = ofy().doTransactionless(new Work() { @Override public DomainResource run() { - // This is cacheable because we are outside of a transaction. - return loadByForeignKey(DomainResource.class, targetId, now); + // We want to load the ForeignKeyIndex and DomainResource from memcache if possible so that + // repeated create attempts of the same domain will not put load on datastore. This is safe + // because this is only a failfast method, and if memcache is stale the worst case scenario + // is that we will fall through to the regular transactional flow and fail there. + return loadByForeignKeyWithMemcache(DomainResource.class, targetId, now); }}); // If the domain exists already and isn't in the add grace period then there is no way it will // be suddenly deleted and therefore the create must fail. diff --git a/java/google/registry/model/EppResourceUtils.java b/java/google/registry/model/EppResourceUtils.java index 89045bcf5..2868f1052 100644 --- a/java/google/registry/model/EppResourceUtils.java +++ b/java/google/registry/model/EppResourceUtils.java @@ -16,7 +16,6 @@ package google.registry.model; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.collect.Iterables.transform; -import static google.registry.model.index.ForeignKeyIndex.loadAndGetKey; import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.util.DateTimeUtils.isAtOrAfter; import static google.registry.util.DateTimeUtils.isBeforeOrAt; @@ -25,6 +24,7 @@ import static google.registry.util.DateTimeUtils.latestOf; import com.google.common.base.Function; import com.googlecode.objectify.Key; import com.googlecode.objectify.Result; +import com.googlecode.objectify.cmd.Loader; import com.googlecode.objectify.cmd.Query; import com.googlecode.objectify.util.ResultNow; import google.registry.model.EppResource.Builder; @@ -93,14 +93,41 @@ public final class EppResourceUtils { @Nullable public static T loadByForeignKey( Class clazz, String foreignKey, DateTime now) { + return loadByForeignKeyInternal(clazz, foreignKey, now, false); + } + + /** + * Loads the last created version of an {@link EppResource} from Datastore or memcache. + * + *

In general, prefer {@link #loadByForeignKey} over this method. Loading from memcache can, in + * rare instances, produce a stale result (when a memcache write fails and the previous result is + * not cleared out) and so using memcache should be avoided unless the caller can tolerate + * staleness until the memcache expiration time and there is a specific need for very low latency + * that is worth the extra complexity of reasoning about caching. + * + * @param clazz the resource type to load + * @param foreignKey id to match + * @param now the current logical time to project resources at + */ + @Nullable + public static T loadByForeignKeyWithMemcache( + Class clazz, String foreignKey, DateTime now) { + return loadByForeignKeyInternal(clazz, foreignKey, now, true); + } + + @Nullable + private static T loadByForeignKeyInternal( + Class clazz, String foreignKey, DateTime now, boolean useMemcache) { checkArgument( ForeignKeyedEppResource.class.isAssignableFrom(clazz), "loadByForeignKey may only be called for foreign keyed EPP resources"); - Key resourceKey = loadAndGetKey(clazz, foreignKey, now); - if (resourceKey == null) { + Loader loader = useMemcache ? ofy().loadWithMemcache() : ofy().load(); + ForeignKeyIndex fki = loader.type(ForeignKeyIndex.mapToFkiClass(clazz)).id(foreignKey).now(); + // The value of fki.getResourceKey() might be null for hard-deleted prober data. + if (fki == null || isAtOrAfter(now, fki.getDeletionTime()) || fki.getResourceKey() == null) { return null; } - T resource = ofy().load().key(resourceKey).now(); + T resource = loader.key(fki.getResourceKey()).now(); if (resource == null || isAtOrAfter(now, resource.getDeletionTime())) { return null; } diff --git a/java/google/registry/model/index/ForeignKeyIndex.java b/java/google/registry/model/index/ForeignKeyIndex.java index 231f71464..1b54ecce2 100644 --- a/java/google/registry/model/index/ForeignKeyIndex.java +++ b/java/google/registry/model/index/ForeignKeyIndex.java @@ -62,7 +62,7 @@ public abstract class ForeignKeyIndex extends BackupGroup @Entity public static class ForeignKeyHostIndex extends ForeignKeyIndex {} - private static final ImmutableMap< + static final ImmutableMap< Class, Class>> RESOURCE_CLASS_TO_FKI_CLASS = ImmutableMap., Class>>of( @@ -102,14 +102,19 @@ public abstract class ForeignKeyIndex extends BackupGroup return topReference; } - /** - * Create a {@link ForeignKeyIndex} instance for a resource, expiring at a specified time. - */ + + @SuppressWarnings("unchecked") + public static Class> mapToFkiClass( + Class resourceClass) { + return (Class>) RESOURCE_CLASS_TO_FKI_CLASS.get(resourceClass); + } + + /** Create a {@link ForeignKeyIndex} instance for a resource, expiring at a specified time. */ public static ForeignKeyIndex create( E resource, DateTime deletionTime) { @SuppressWarnings("unchecked") - Class> fkiClass = - (Class>) RESOURCE_CLASS_TO_FKI_CLASS.get(resource.getClass()); + Class> fkiClass = + ForeignKeyIndex.mapToFkiClass((Class) resource.getClass()); ForeignKeyIndex instance = instantiate(fkiClass); instance.topReference = Key.create(resource); instance.foreignKey = resource.getForeignKey(); @@ -120,7 +125,7 @@ public abstract class ForeignKeyIndex extends BackupGroup /** Create a {@link ForeignKeyIndex} key for a resource. */ public static Key> createKey(EppResource resource) { return Key.>create( - RESOURCE_CLASS_TO_FKI_CLASS.get(resource.getClass()), resource.getForeignKey()); + mapToFkiClass(resource.getClass()), resource.getForeignKey()); } /** @@ -162,11 +167,10 @@ public abstract class ForeignKeyIndex extends BackupGroup *

The returned map will omit any keys for which the {@link ForeignKeyIndex} doesn't exist or * has been soft deleted. */ - @SuppressWarnings("unchecked") public static Map> load( Class clazz, Iterable foreignKeys, final DateTime now) { - return (Map>) filterValues( - ofy().load().type(RESOURCE_CLASS_TO_FKI_CLASS.get(clazz)).ids(foreignKeys), + return filterValues( + ofy().load().type(mapToFkiClass(clazz)).ids(foreignKeys), new Predicate>() { @Override public boolean apply(ForeignKeyIndex fki) { diff --git a/javatests/google/registry/flows/domain/DomainApplicationCreateFlowTest.java b/javatests/google/registry/flows/domain/DomainApplicationCreateFlowTest.java index 7e27bc466..182d112b9 100644 --- a/javatests/google/registry/flows/domain/DomainApplicationCreateFlowTest.java +++ b/javatests/google/registry/flows/domain/DomainApplicationCreateFlowTest.java @@ -18,6 +18,7 @@ import static com.google.common.collect.Iterables.getLast; import static com.google.common.io.BaseEncoding.base16; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; +import static com.google.common.truth.Truth.assert_; import static google.registry.model.index.DomainApplicationIndex.loadActiveApplicationsByDomainName; import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.testing.DatastoreHelper.assertNoBillingEvents; @@ -32,6 +33,7 @@ import static google.registry.testing.DatastoreHelper.persistReservedList; import static google.registry.testing.DatastoreHelper.persistResource; import static google.registry.testing.DomainApplicationSubject.assertAboutApplications; import static google.registry.testing.EppExceptionSubject.assertAboutEppExceptions; +import static google.registry.testing.MemcacheHelper.setMemcacheContents; import static google.registry.util.DateTimeUtils.END_OF_TIME; import static google.registry.util.DateTimeUtils.START_OF_TIME; import static org.joda.money.CurrencyUnit.EUR; @@ -106,6 +108,7 @@ import google.registry.flows.domain.DomainFlowUtils.UnsupportedFeeAttributeExcep import google.registry.flows.domain.DomainFlowUtils.UnsupportedMarkTypeException; import google.registry.flows.exceptions.ResourceAlreadyExistsException; import google.registry.model.domain.DomainApplication; +import google.registry.model.domain.DomainResource; import google.registry.model.domain.GracePeriod; import google.registry.model.domain.LrpTokenEntity; import google.registry.model.domain.launch.ApplicationStatus; @@ -113,6 +116,8 @@ import google.registry.model.domain.launch.LaunchNotice; import google.registry.model.domain.launch.LaunchPhase; import google.registry.model.domain.rgp.GracePeriodStatus; import google.registry.model.domain.secdns.DelegationSignerData; +import google.registry.model.index.ForeignKeyIndex; +import google.registry.model.ofy.RequestCapturingAsyncDatastoreService; import google.registry.model.registrar.Registrar; import google.registry.model.registry.Registry; import google.registry.model.registry.Registry.TldState; @@ -1516,21 +1521,48 @@ public class DomainApplicationCreateFlowTest runFlow(); } + @Test - public void testFailure_alreadyExists() throws Exception { + public void testFailure_alreadyExists_triggersFailfast() throws Exception { + // This fails fast and throws DomainAlreadyExistsException from init() as a special case. persistContactsAndHosts(); clock.advanceOneMilli(); persistActiveDomain(getUniqueIdFromCommand()); try { runFlow(); - assertWithMessage("Expected ResourceAlreadyExistsException to be thrown").fail(); + assert_().fail( + "Expected to throw ResourceAlreadyExistsException with message " + + "Object with given ID (%s) already exists", + getUniqueIdFromCommand()); } catch (ResourceAlreadyExistsException e) { assertThat(e.isFailfast()).isTrue(); - assertAboutEppExceptions().that(e).marshalsToXml().and().hasMessage( - String.format("Object with given ID (%s) already exists", getUniqueIdFromCommand())); } } + @Test + public void testFailfast_withMemcachedDomainAndFki_hitsMemcache() throws Exception { + persistContactsAndHosts(); + clock.advanceOneMilli(); + DomainResource domain = persistActiveDomain(getUniqueIdFromCommand()); + setMemcacheContents(Key.create(domain), ForeignKeyIndex.createKey(domain)); + int numPreviousReads = RequestCapturingAsyncDatastoreService.getReads().size(); + try { + runFlow(); + assert_().fail("Expected to throw ResourceAlreadyExistsException"); + } catch (ResourceAlreadyExistsException e) { + assertThat(e.isFailfast()).isTrue(); + } + // Everything should have been loaded from memcache so nothing should hit datastore. + int numReadsInFlow = + RequestCapturingAsyncDatastoreService.getReads().size() - numPreviousReads; + // TODO(b/27424173): This is 1 because there is no @Cache annotation on DomainBase, and we + // don't want to blindly add it because that's a production change that adds potentially + // dangerous caching. When the recommendations from the audit in b/27424173 are done and we've + // tested the new safer caching this should be set to 0. + assertThat(numReadsInFlow).isEqualTo(1); + } + + @Test public void testFailure_registrantNotWhitelisted() throws Exception { persistActiveContact("someone"); diff --git a/javatests/google/registry/flows/domain/DomainCreateFlowTest.java b/javatests/google/registry/flows/domain/DomainCreateFlowTest.java index 5f86fa64a..fc5bf2e82 100644 --- a/javatests/google/registry/flows/domain/DomainCreateFlowTest.java +++ b/javatests/google/registry/flows/domain/DomainCreateFlowTest.java @@ -16,6 +16,7 @@ package google.registry.flows.domain; import static com.google.common.io.BaseEncoding.base16; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assert_; import static google.registry.model.domain.fee.Fee.FEE_EXTENSION_URIS; import static google.registry.model.eppcommon.StatusValue.OK; import static google.registry.model.eppcommon.StatusValue.SERVER_TRANSFER_PROHIBITED; @@ -39,6 +40,7 @@ import static google.registry.testing.DatastoreHelper.persistDeletedDomain; import static google.registry.testing.DatastoreHelper.persistReservedList; import static google.registry.testing.DatastoreHelper.persistResource; import static google.registry.testing.DomainResourceSubject.assertAboutDomains; +import static google.registry.testing.MemcacheHelper.setMemcacheContents; import static google.registry.testing.TaskQueueHelper.assertDnsTasksEnqueued; import static google.registry.testing.TaskQueueHelper.assertNoDnsTasksEnqueued; import static google.registry.testing.TaskQueueHelper.assertNoTasksEnqueued; @@ -55,6 +57,7 @@ import com.google.common.base.Strings; 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.flows.EppException.UnimplementedExtensionException; import google.registry.flows.EppRequestSource; import google.registry.flows.ExtensionManager.UndeclaredServiceExtensionException; @@ -120,6 +123,8 @@ import google.registry.model.domain.launch.LaunchNotice; import google.registry.model.domain.rgp.GracePeriodStatus; import google.registry.model.domain.secdns.DelegationSignerData; import google.registry.model.eppcommon.StatusValue; +import google.registry.model.index.ForeignKeyIndex; +import google.registry.model.ofy.RequestCapturingAsyncDatastoreService; import google.registry.model.poll.PollMessage; import google.registry.model.registrar.Registrar; import google.registry.model.registry.Registry; @@ -890,21 +895,43 @@ public class DomainCreateFlowTest extends ResourceFlowTestCase