From 8f456bcf6496aa039caf794c9624ce8531f03b17 Mon Sep 17 00:00:00 2001 From: mcilwain Date: Wed, 23 May 2018 12:17:27 -0700 Subject: [PATCH] Clarify when to use cache (or not) when loading premium lists You don't want to use the cache when loading them for the purposes of updating them, but you definitely do still want to use the cache when checking the price of individual domains. In [] the cache clearing of premium lists on update was removed. This is a good thing in aggregate because the cache is per-instance and thus misleading, but it also caused us to not be able to update the same premium list twice within an hour because the second update would hit a "PremiumList was concurrently edited" exception, owing to first loading the stale version from the cache for the purposes of updating it. Now we bypass the cache for that purpose. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=197768142 --- .../model/registry/label/PremiumList.java | 25 ++++++++--------- .../registry/label/PremiumListUtils.java | 13 +++------ .../tools/CreateOrUpdateTldCommand.java | 5 ++-- .../tools/DeletePremiumListCommand.java | 2 +- .../tools/server/UpdatePremiumListAction.java | 2 +- .../registry/model/registry/RegistryTest.java | 2 +- .../model/registry/label/PremiumListTest.java | 4 +-- .../registry/label/PremiumListUtilsTest.java | 27 +++++++++---------- .../tools/DeletePremiumListCommandTest.java | 4 +-- .../server/CreatePremiumListActionTest.java | 6 ++--- .../server/UpdatePremiumListActionTest.java | 2 +- 11 files changed, 44 insertions(+), 48 deletions(-) diff --git a/java/google/registry/model/registry/label/PremiumList.java b/java/google/registry/model/registry/label/PremiumList.java index 40cda1ab8..c4eb49d52 100644 --- a/java/google/registry/model/registry/label/PremiumList.java +++ b/java/google/registry/model/registry/label/PremiumList.java @@ -138,19 +138,15 @@ public final class PremiumList extends BaseDomainLabelList() { @Override - public PremiumList load(final String listName) { - return ofy() - .doTransactionless( - () -> - ofy() - .load() - .type(PremiumList.class) - .parent(getCrossTldKey()) - .id(listName) - .now()); + public PremiumList load(final String name) { + return ofy().doTransactionless(() -> loadPremiumList(name)); } }); + private static PremiumList loadPremiumList(String name) { + return ofy().load().type(PremiumList.class).parent(getCrossTldKey()).id(name).now(); + } + /** * In-memory cache for {@link PremiumListRevision}s, used for retrieving Bloom filters quickly. * @@ -211,8 +207,8 @@ public final class PremiumList extends BaseDomainLabelList get(String name) { + /** Returns the PremiumList with the specified name, from cache. */ + public static Optional getCached(String name) { try { return Optional.of(cachePremiumLists.get(name)); } catch (InvalidCacheLoadException e) { @@ -222,6 +218,11 @@ public final class PremiumList extends BaseDomainLabelList getUncached(String name) { + return Optional.ofNullable(loadPremiumList(name)); + } + /** * A premium list entry entity, persisted to Datastore. Each instance represents the price of a * single label on a given TLD. diff --git a/java/google/registry/model/registry/label/PremiumListUtils.java b/java/google/registry/model/registry/label/PremiumListUtils.java index 46506fb86..12dda0800 100644 --- a/java/google/registry/model/registry/label/PremiumListUtils.java +++ b/java/google/registry/model/registry/label/PremiumListUtils.java @@ -76,7 +76,7 @@ public final class PremiumListUtils { } DateTime startTime = DateTime.now(UTC); String listName = registry.getPremiumList().getName(); - Optional optionalPremiumList = PremiumList.get(listName); + Optional optionalPremiumList = PremiumList.getCached(listName); checkState(optionalPremiumList.isPresent(), "Could not load premium list '%s'", listName); PremiumList premiumList = optionalPremiumList.get(); PremiumListRevision revision; @@ -141,7 +141,7 @@ public final class PremiumListUtils { public static PremiumList savePremiumListAndEntries( final PremiumList premiumList, ImmutableMap premiumListEntries) { - final Optional oldPremiumList = PremiumList.get(premiumList.getName()); + final Optional oldPremiumList = PremiumList.getUncached(premiumList.getName()); // Create the new revision (with its Bloom filter) and parent the entries on it. final PremiumListRevision newRevision = @@ -151,13 +151,8 @@ public final class PremiumListUtils { parentPremiumListEntriesOnRevision(premiumListEntries.values(), newRevisionKey); // Save the new child entities in a series of transactions. - for (final List batch : - partition(parentedEntries, TRANSACTION_BATCH_SIZE)) { - ofy().transactNew(new VoidWork() { - @Override - public void vrun() { - ofy().save().entities(batch); - }}); + for (final List batch : partition(parentedEntries, TRANSACTION_BATCH_SIZE)) { + ofy().transactNew(() -> ofy().save().entities(batch)); } // Save the new PremiumList and revision itself. diff --git a/java/google/registry/tools/CreateOrUpdateTldCommand.java b/java/google/registry/tools/CreateOrUpdateTldCommand.java index c9cce1f9b..dc1a7484e 100644 --- a/java/google/registry/tools/CreateOrUpdateTldCommand.java +++ b/java/google/registry/tools/CreateOrUpdateTldCommand.java @@ -362,8 +362,9 @@ abstract class CreateOrUpdateTldCommand extends MutatingCommand { if (premiumListName != null) { if (premiumListName.isPresent()) { - Optional premiumList = PremiumList.get(premiumListName.get()); - checkArgument(premiumList.isPresent(), + Optional premiumList = PremiumList.getUncached(premiumListName.get()); + checkArgument( + premiumList.isPresent(), String.format("The premium list '%s' doesn't exist", premiumListName.get())); builder.setPremiumList(premiumList.get()); } else { diff --git a/java/google/registry/tools/DeletePremiumListCommand.java b/java/google/registry/tools/DeletePremiumListCommand.java index a5d956b7b..0ae7d36ac 100644 --- a/java/google/registry/tools/DeletePremiumListCommand.java +++ b/java/google/registry/tools/DeletePremiumListCommand.java @@ -48,7 +48,7 @@ final class DeletePremiumListCommand extends ConfirmingCommand implements Remote doesPremiumListExist(name), "Cannot delete the premium list %s because it doesn't exist.", name); - premiumList = PremiumList.get(name).get(); + premiumList = PremiumList.getUncached(name).get(); ImmutableSet tldsUsedOn = premiumList.getReferencingTlds(); checkArgument( tldsUsedOn.isEmpty(), diff --git a/java/google/registry/tools/server/UpdatePremiumListAction.java b/java/google/registry/tools/server/UpdatePremiumListAction.java index 8c736a3d6..223cd327f 100644 --- a/java/google/registry/tools/server/UpdatePremiumListAction.java +++ b/java/google/registry/tools/server/UpdatePremiumListAction.java @@ -44,7 +44,7 @@ public class UpdatePremiumListAction extends CreateOrUpdatePremiumListAction { @Override protected void savePremiumList() { - Optional existingPremiumList = PremiumList.get(name); + Optional existingPremiumList = PremiumList.getUncached(name); checkArgument( existingPremiumList.isPresent(), "Could not update premium list %s because it doesn't exist.", diff --git a/javatests/google/registry/model/registry/RegistryTest.java b/javatests/google/registry/model/registry/RegistryTest.java index 014cd5ebd..8eeba7847 100644 --- a/javatests/google/registry/model/registry/RegistryTest.java +++ b/javatests/google/registry/model/registry/RegistryTest.java @@ -226,7 +226,7 @@ public class RegistryTest extends EntityTestCase { Registry registry = Registry.get("tld").asBuilder().setPremiumList(pl2).build(); Key plKey = registry.getPremiumList(); assertThat(plKey).isNotNull(); - PremiumList stored = PremiumList.get(plKey.getName()).get(); + PremiumList stored = PremiumList.getUncached(plKey.getName()).get(); assertThat(stored.getName()).isEqualTo("tld2"); } diff --git a/javatests/google/registry/model/registry/label/PremiumListTest.java b/javatests/google/registry/model/registry/label/PremiumListTest.java index 5800858b7..e3bbcffa4 100644 --- a/javatests/google/registry/model/registry/label/PremiumListTest.java +++ b/javatests/google/registry/model/registry/label/PremiumListTest.java @@ -70,7 +70,7 @@ public class PremiumListTest { @Test public void testProbablePremiumLabels() throws Exception { - PremiumList pl = PremiumList.get("tld").get(); + PremiumList pl = PremiumList.getUncached("tld").get(); PremiumListRevision revision = ofy().load().key(pl.getRevisionKey()).now(); assertThat(revision.getProbablePremiumLabels().mightContain("notpremium")).isFalse(); for (String label : ImmutableList.of("rich", "lol", "johnny-be-goode", "icann")) { @@ -86,7 +86,7 @@ public class PremiumListTest { assertThrows( IllegalStateException.class, () -> - PremiumList.get("tld") + PremiumList.getUncached("tld") .get() .parse( ImmutableList.of( diff --git a/javatests/google/registry/model/registry/label/PremiumListUtilsTest.java b/javatests/google/registry/model/registry/label/PremiumListUtilsTest.java index d49deb1d4..aa9aa6b44 100644 --- a/javatests/google/registry/model/registry/label/PremiumListUtilsTest.java +++ b/javatests/google/registry/model/registry/label/PremiumListUtilsTest.java @@ -110,7 +110,7 @@ public class PremiumListUtilsTest { @Test public void testGetPremiumPrice_throwsExceptionWhenNonExistentPremiumListConfigured() throws Exception { - deletePremiumList(PremiumList.get("tld").get()); + deletePremiumList(PremiumList.getUncached("tld").get()); IllegalStateException thrown = assertThrows( IllegalStateException.class, () -> getPremiumPrice("blah", Registry.get("tld"))); @@ -152,7 +152,7 @@ public class PremiumListUtilsTest { @Test public void testGetPremiumPrice_comesFromBloomFilter() throws Exception { - PremiumList pl = PremiumList.get("tld").get(); + PremiumList pl = PremiumList.getCached("tld").get(); PremiumListEntry entry = persistResource( new PremiumListEntry.Builder() @@ -201,7 +201,7 @@ public class PremiumListUtilsTest { .delete() .keys( Key.create( - PremiumList.get("tld").get().getRevisionKey(), + PremiumList.getCached("tld").get().getRevisionKey(), PremiumListEntry.class, "rich")); } @@ -239,8 +239,7 @@ public class PremiumListUtilsTest { .now() .price) .isEqualTo(Money.parse("JPY 1000")); - PremiumList pl2 = - savePremiumListAndEntries(pl, ImmutableList.of("genius,USD 10", "savant,USD 90")); + savePremiumListAndEntries(pl, ImmutableList.of("genius,USD 10", "savant,USD 90")); assertThat(getPremiumPrice("genius", registry)).hasValue(Money.parse("USD 10")); assertThat(getPremiumPrice("savant", registry)).hasValue(Money.parse("USD 90")); assertThat(getPremiumPrice("dolt", registry)).isEmpty(); @@ -277,7 +276,7 @@ public class PremiumListUtilsTest { assertThat(getPremiumPrice("lol", Registry.get("tld"))).hasValue(Money.parse("USD 999")); assertThat(getPremiumPrice("lol ", Registry.get("tld"))).isEmpty(); ImmutableMap entries = - loadPremiumListEntries(PremiumList.get("tld2").get()); + loadPremiumListEntries(PremiumList.getUncached("tld2").get()); assertThat(entries.keySet()).containsExactly("lol"); assertThat(entries).doesNotContainKey("lol "); PremiumListEntry entry = entries.get("lol"); @@ -305,12 +304,12 @@ public class PremiumListUtilsTest { new PremiumList.Builder().setName("pl").build(), ImmutableList.of("test,USD 1")); ImmutableMap entries = loadPremiumListEntries(pl); assertThat(entries.keySet()).containsExactly("test"); - assertThat(loadPremiumListEntries(PremiumList.get("pl").get())).isEqualTo(entries); + assertThat(loadPremiumListEntries(PremiumList.getUncached("pl").get())).isEqualTo(entries); // Save again with no changes, and clear the cache to force a re-load from Datastore. PremiumList resaved = savePremiumListAndEntries(pl, ImmutableList.of("test,USD 1")); ofy().clearSessionCache(); Map entriesReloaded = - loadPremiumListEntries(PremiumList.get("pl").get()); + loadPremiumListEntries(PremiumList.getUncached("pl").get()); assertThat(entriesReloaded).hasSize(1); assertThat(entriesReloaded).containsKey("test"); assertThat(entriesReloaded.get("test").parent).isEqualTo(resaved.getRevisionKey()); @@ -319,18 +318,18 @@ public class PremiumListUtilsTest { @Test public void testDelete() throws Exception { persistPremiumList("gtld1", "trombone,USD 10"); - assertThat(PremiumList.get("gtld1")).isPresent(); - Key parent = PremiumList.get("gtld1").get().getRevisionKey(); - deletePremiumList(PremiumList.get("gtld1").get()); - assertThat(PremiumList.get("gtld1")).isEmpty(); + assertThat(PremiumList.getUncached("gtld1")).isPresent(); + Key parent = PremiumList.getUncached("gtld1").get().getRevisionKey(); + deletePremiumList(PremiumList.getUncached("gtld1").get()); + assertThat(PremiumList.getUncached("gtld1")).isEmpty(); assertThat(ofy().load().type(PremiumListEntry.class).ancestor(parent).list()).isEmpty(); } @Test public void testDelete_largeNumberOfEntries_succeeds() { persistHumongousPremiumList("ginormous", 2500); - deletePremiumList(PremiumList.get("ginormous").get()); - assertThat(PremiumList.get("ginormous")).isEmpty(); + deletePremiumList(PremiumList.getUncached("ginormous").get()); + assertThat(PremiumList.getUncached("ginormous")).isEmpty(); } /** Persists a premium list with a specified number of nonsense entries. */ diff --git a/javatests/google/registry/tools/DeletePremiumListCommandTest.java b/javatests/google/registry/tools/DeletePremiumListCommandTest.java index ca402f621..bdd9b28da 100644 --- a/javatests/google/registry/tools/DeletePremiumListCommandTest.java +++ b/javatests/google/registry/tools/DeletePremiumListCommandTest.java @@ -36,7 +36,7 @@ public class DeletePremiumListCommandTest extends CommandTestCase runCommandForced("--name=" + premiumList.getName())); - assertThat(PremiumList.get(premiumList.getName())).isPresent(); + assertThat(PremiumList.getUncached(premiumList.getName())).isPresent(); assertThat(thrown) .hasMessageThat() .isEqualTo("Cannot delete premium list because it is used on these tld(s): xn--q9jyb4c"); diff --git a/javatests/google/registry/tools/server/CreatePremiumListActionTest.java b/javatests/google/registry/tools/server/CreatePremiumListActionTest.java index 62e682784..2fa659eef 100644 --- a/javatests/google/registry/tools/server/CreatePremiumListActionTest.java +++ b/javatests/google/registry/tools/server/CreatePremiumListActionTest.java @@ -46,7 +46,7 @@ public class CreatePremiumListActionTest { @Before public void init() throws Exception { createTlds("foo", "xn--q9jyb4c", "how"); - deletePremiumList(PremiumList.get("foo").get()); + deletePremiumList(PremiumList.getUncached("foo").get()); action = new CreatePremiumListAction(); response = new FakeJsonResponse(); action.response = response; @@ -78,7 +78,7 @@ public class CreatePremiumListActionTest { action.override = true; action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(loadPremiumListEntries(PremiumList.get("zanzibar").get())).hasSize(1); + assertThat(loadPremiumListEntries(PremiumList.getUncached("zanzibar").get())).hasSize(1); } @Test @@ -87,7 +87,7 @@ public class CreatePremiumListActionTest { action.inputData = "rich,USD 25\nricher,USD 1000\n"; action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - assertThat(loadPremiumListEntries(PremiumList.get("foo").get())).hasSize(2); + assertThat(loadPremiumListEntries(PremiumList.getUncached("foo").get())).hasSize(2); assertThat(getPremiumPrice("rich", Registry.get("foo"))).hasValue(Money.parse("USD 25")); assertThat(getPremiumPrice("diamond", Registry.get("foo"))).isEmpty(); } diff --git a/javatests/google/registry/tools/server/UpdatePremiumListActionTest.java b/javatests/google/registry/tools/server/UpdatePremiumListActionTest.java index 55af332c2..662465bae 100644 --- a/javatests/google/registry/tools/server/UpdatePremiumListActionTest.java +++ b/javatests/google/registry/tools/server/UpdatePremiumListActionTest.java @@ -80,7 +80,7 @@ public class UpdatePremiumListActionTest { action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); Registry registry = Registry.get("foo"); - assertThat(loadPremiumListEntries(PremiumList.get("foo").get())).hasSize(3); + assertThat(loadPremiumListEntries(PremiumList.getUncached("foo").get())).hasSize(3); assertThat(getPremiumPrice("rich", registry)).hasValue(Money.parse("USD 75")); assertThat(getPremiumPrice("richer", registry)).hasValue(Money.parse("USD 5000")); assertThat(getPremiumPrice("poor", registry)).hasValue(Money.parse("USD 0.99"));