From 275d6ddc10ad9130889803a69b2c2ce47d56a07c Mon Sep 17 00:00:00 2001 From: cgoldfeder Date: Fri, 5 May 2017 12:31:00 -0700 Subject: [PATCH] Disable memcache completely We've determined that getting correctness semantics right, even in the few cases that it is possible to do so (see linked bug for audit) is not worth the bother in terms of highly complicated code and potential bugs. This CL turns off memcache at the Ofy level but doesn't rip out the annotations etc. so that we can quickly turn it back on if this turns out to have been a mistake. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=155227761 --- java/google/registry/model/ofy/Ofy.java | 6 ++-- .../DomainApplicationCreateFlowTest.java | 28 ------------------- .../domain/DomainApplicationInfoFlowTest.java | 24 ---------------- .../flows/domain/DomainCreateFlowTest.java | 26 ----------------- .../flows/domain/DomainInfoFlowTest.java | 25 ----------------- .../model/registrar/RegistrarTest.java | 11 -------- .../registry/model/registry/RegistryTest.java | 16 ----------- 7 files changed, 3 insertions(+), 133 deletions(-) diff --git a/java/google/registry/model/ofy/Ofy.java b/java/google/registry/model/ofy/Ofy.java index 58db06072..ebe35513a 100644 --- a/java/google/registry/model/ofy/Ofy.java +++ b/java/google/registry/model/ofy/Ofy.java @@ -140,8 +140,7 @@ public class Ofy { * worth the extra complexity of reasoning about caching. */ public Loader load() { - // TODO(b/27424173): change to false when memcache audit changes are implemented. - return ofy().cache(true).load(); + return ofy().cache(false).load(); } /** @@ -154,7 +153,8 @@ public class Ofy { * worth the extra complexity of reasoning about caching. */ public Loader loadWithMemcache() { - return ofy().cache(true).load(); + // TODO(b/27424173): Remove this method if we determine we are ok with no memcache. + return ofy().cache(false).load(); } /** diff --git a/javatests/google/registry/flows/domain/DomainApplicationCreateFlowTest.java b/javatests/google/registry/flows/domain/DomainApplicationCreateFlowTest.java index ca01e75b4..d804cafc3 100644 --- a/javatests/google/registry/flows/domain/DomainApplicationCreateFlowTest.java +++ b/javatests/google/registry/flows/domain/DomainApplicationCreateFlowTest.java @@ -33,7 +33,6 @@ 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; @@ -108,7 +107,6 @@ 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; @@ -116,8 +114,6 @@ 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; @@ -1547,30 +1543,6 @@ public class DomainApplicationCreateFlowTest } } - @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/DomainApplicationInfoFlowTest.java b/javatests/google/registry/flows/domain/DomainApplicationInfoFlowTest.java index f4b6ea8f3..ca41a6ba7 100644 --- a/javatests/google/registry/flows/domain/DomainApplicationInfoFlowTest.java +++ b/javatests/google/registry/flows/domain/DomainApplicationInfoFlowTest.java @@ -24,7 +24,6 @@ import static google.registry.testing.DatastoreHelper.persistActiveContact; import static google.registry.testing.DatastoreHelper.persistActiveHost; import static google.registry.testing.DatastoreHelper.persistResource; import static google.registry.testing.MemcacheHelper.clearMemcache; -import static google.registry.testing.MemcacheHelper.setMemcacheContents; import static google.registry.testing.TestDataHelper.loadFileWithSubstitutions; import static google.registry.util.DatastoreServiceUtils.KEY_TO_KIND_FUNCTION; @@ -53,7 +52,6 @@ import google.registry.model.domain.secdns.DelegationSignerData; import google.registry.model.eppcommon.AuthInfo.PasswordAuth; import google.registry.model.eppcommon.StatusValue; import google.registry.model.host.HostResource; -import google.registry.model.index.DomainApplicationIndex; import google.registry.model.ofy.RequestCapturingAsyncDatastoreService; import google.registry.model.registry.Registry.TldState; import google.registry.model.smd.EncodedSignedMark; @@ -330,28 +328,6 @@ public class DomainApplicationInfoFlowTest runFlow(); } - @Test - public void testLoadsComeFromMemcache() throws Exception { - persistTestEntities(HostsState.HOSTS_EXIST, MarksState.NO_MARKS_EXIST); - setMemcacheContents( - Key.create(application), - DomainApplicationIndex.createKey(application), - Key.create(registrant), - Key.create(contact), - Key.create(host1), - Key.create(host2)); - int numPreviousReads = RequestCapturingAsyncDatastoreService.getReads().size(); - doSuccessfulTest("domain_info_sunrise_response.xml", HostsState.HOSTS_EXIST); - - // Everything should be in 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 that we load contacts and hosts as a batch rather than individually. */ @Test public void testBatchLoadingOfReferences() throws Exception { diff --git a/javatests/google/registry/flows/domain/DomainCreateFlowTest.java b/javatests/google/registry/flows/domain/DomainCreateFlowTest.java index 8af36899f..01d0f924d 100644 --- a/javatests/google/registry/flows/domain/DomainCreateFlowTest.java +++ b/javatests/google/registry/flows/domain/DomainCreateFlowTest.java @@ -41,7 +41,6 @@ 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.EppExceptionSubject.assertAboutEppExceptions; -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; @@ -58,7 +57,6 @@ 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; @@ -124,8 +122,6 @@ 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; @@ -912,28 +908,6 @@ public class DomainCreateFlowTest extends ResourceFlowTestCase