Add cache for deletion times of existing domains

[gbrodman]

This should help in instances of popular domains dropping, since we won't need to do an additional two database loads every time (assuming the deletion time is in the future).

---

This change is 

[CydeWeys]

Reviewable status: 0 of 11 files reviewed, 5 unresolved discussions

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java line 39 at r1 (raw file):

 *
 * <p>The cache is fairly short-lived (as we're concerned about many requests at basically the same
 * time), and entries also expire when the drop actually happens.

Also worth calling out here that the cache entry will be re-created immediately upon the first successful creation, so that the vast majority of requests will hit the cache both before the domain drops, and afterwards.

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java line 46 at r1 (raw file):

 * Put another way, if a domain really doesn't exist, we'll re-attempt the database load every time.
 *
 * <p>We don't explicitly register creates or deletes in the flows themselves in case the

I don't understand this paragraph. What does "explicitly register creates or deletes in the flows themselves" mean?

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java line 84 at r1 (raw file):

        }

        /** Reads do not change the expiry duration. */

Should they though?

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java line 103 at r1 (raw file):

        // Otherwise, we cache the existing domain's deletion time so that further queries can
        // return quickly and avoid database calls
        return tm().loadByKey(key).getDeletionTime();

This seems inefficient? Won't this load the entire domain object again, just to discard it all and only read one field off it?

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java line 120 at r1 (raw file):

  }

  /** Returns the domain's deletion time, or null if it doesn't currently exist. */

Seems like Optional<DateTime> would be a better return type here.

[CydeWeys]

Reviewable status: 0 of 11 files reviewed, 6 unresolved discussions (waiting on @gbrodman)

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java line 96 at r1 (raw file):

      (domainName) -> {
        VKey<Domain> key =
            ForeignKeyUtils.load(Domain.class, domainName, tm().getTransactionTime());

If you look at the implementation of ForeignKeyUtils.load(), you'll see that it's already doing a SELECT query that is loading the deletionTime. So you just need to do a little bit of refactoring to expose that to the cache, and then you don't need any additional loads below at all.

[CydeWeys]

Reviewable status: 0 of 11 files reviewed, 6 unresolved discussions (waiting on @gbrodman)

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java line 96 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

If you look at the implementation of ForeignKeyUtils.load(), you'll see that it's already doing a SELECT query that is loading the deletionTime. So you just need to do a little bit of refactoring to expose that to the cache, and then you don't need any additional loads below at all.

(And this ends up not doing any more DB loads when the cache is unpopulated than the current code, as you always need to run Fk.load to check existence.)

[gbrodman]

Reviewable status: 0 of 12 files reviewed, 6 unresolved discussions (waiting on @CydeWeys)

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java line 39 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Also worth calling out here that the cache entry will be re-created immediately upon the first successful creation, so that the vast majority of requests will hit the cache both before the domain drops, and afterwards.

Done.

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java line 46 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

I don't understand this paragraph. What does "explicitly register creates or deletes in the flows themselves" mean?

Theoretically, we could save a single DB load by saying something like "if we've finished successfully creating this domain, add it to the cache with a deletion time of END OF TIME". But because the cache "lives" outside of transactions, we can't really do that safely.

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java line 84 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Should they though?

i don't see a particular need for them to do so

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java line 96 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

(And this ends up not doing any more DB loads when the cache is unpopulated than the current code, as you always need to run Fk.load to check existence.)

i was kinda hesitant to expose anything from the foreign key utils class but we definitely can expose the loading method

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java line 103 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

This seems inefficient? Won't this load the entire domain object again, just to discard it all and only read one field off it?

sure (mooted by other comment)

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java line 120 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Seems like Optional<DateTime> would be a better return type here.

No. See the comments about nulls -- they're good because they don't get cached, and we don't want to cache lack-of-existence just in case it gets created while the cache entry is still live.

[CydeWeys]

Reviewable status: 0 of 12 files reviewed, 5 unresolved discussions (waiting on @gbrodman)

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java line 120 at r1 (raw file):

Previously, gbrodman wrote…

No. See the comments about nulls -- they're good because they don't get cached, and we don't want to cache lack-of-existence just in case it gets created while the cache entry is still live.

I'm not suggesting to change the implementation of the cache, only to change the return type of this method to be Optional<DateTime>, which is exactly what Optional is for (rather than the somewhat clunky handling of the null at the callsite, that if you forget to do, gets you in trouble).

[CydeWeys]

Reviewable status: 0 of 12 files reviewed, 4 unresolved discussions (waiting on @gbrodman)

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java line 46 at r1 (raw file):

Previously, gbrodman wrote…

Theoretically, we could save a single DB load by saying something like "if we've finished successfully creating this domain, add it to the cache with a deletion time of END OF TIME". But because the cache "lives" outside of transactions, we can't really do that safely.

Ah, I see what you mean. I would re-write it as "We don't explicitly set the cache inside domain create/delete flows ..."

[CydeWeys]

Reviewable status: 0 of 12 files reviewed, 4 unresolved discussions (waiting on @gbrodman)

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCacheModule.java line 23 at r2 (raw file):

/** Dagger module to provide the {@link DomainDeletionTimeCache}. */
@Module
public class DomainDeletionTimeCacheModule {

Any reason this has to be a new module just for doing this one thing, rather than just a new method inside the existing FlowModule or similar?

[CydeWeys]

Reviewable status: 0 of 12 files reviewed, 5 unresolved discussions (waiting on @gbrodman)

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java line 96 at r1 (raw file):

Previously, gbrodman wrote…

i was kinda hesitant to expose anything from the foreign key utils class but we definitely can expose the loading method

Yeah this is definitely more performant now.

---

core/src/main/java/google/registry/model/ForeignKeyUtils.java line 107 at r2 (raw file):

   * to duplicate keys.
   */
  public static <E extends EppResource> ImmutableMap<String, MostRecentResource> load(

You might consider renaming this method now rather than globally exposing another overload of load() that is not applicable in almost any other use case.

[CydeWeys]

Reviewable status: 0 of 12 files reviewed, 6 unresolved discussions (waiting on @gbrodman)

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java line 99 at r2 (raw file):

        ForeignKeyUtils.MostRecentResource mostRecentResource =
            ForeignKeyUtils.load(Domain.class, ImmutableSet.of(domainName), false).get(domainName);
        if (mostRecentResource == null) {

It's minor, but, you can replace these four lines with one lines that uses Optional.ofNullable() and .map(), or just the ternary operator, whichever is cleaner.

[gbrodman]

Reviewable status: 0 of 12 files reviewed, 4 unresolved discussions (waiting on @CydeWeys)

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java line 46 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Ah, I see what you mean. I would re-write it as "We don't explicitly set the cache inside domain create/delete flows ..."

Done.

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java line 120 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

I'm not suggesting to change the implementation of the cache, only to change the return type of this method to be Optional<DateTime>, which is exactly what Optional is for (rather than the somewhat clunky handling of the null at the callsite, that if you forget to do, gets you in trouble).

oh, sure eh yeah it's all good either way i think

---

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCacheModule.java line 23 at r2 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Any reason this has to be a new module just for doing this one thing, rather than just a new method inside the existing FlowModule or similar?

Yup. FlowModule (and most or all of the similar modules) are per-request modules, meaning that they're instantiated fresh for every request. All of the other singleton-type injections are related to things like the persistence module or something else pretty deep in the back end, where this wouldn't apply.

---

core/src/main/java/google/registry/model/ForeignKeyUtils.java line 107 at r2 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

You might consider renaming this method now rather than globally exposing another overload of load() that is not applicable in almost any other use case.

sure

[CydeWeys]

Reviewable status: 0 of 12 files reviewed, all discussions resolved