Add Jedis (for Valkey) caches for domains and hosts - #3013
Conversation
ffce812 to
2198f73
Compare
cfced4c to
37c8675
Compare
d7b0638 to
f83a27f
Compare
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys made 6 comments.
Reviewable status: 0 of 49 files reviewed, 6 unresolved discussions (waiting on gbrodman).
core/src/main/java/google/registry/cache/CacheModule.java line 145 at r5 (raw file):
return; } catch (Exception e) { // Verification failed, try the next one
Store this to a local variable outside the for loop and return it as an inner exception when throwing CertificateException below. Otherwise, as currently written, you're swallowing all exceptions that might actually contain hints as to why things aren't working, and just throwing the string "None of the server certificates were signed by the provided CA".
core/src/main/java/google/registry/cache/MultilayerDomainCache.java line 49 at r5 (raw file):
@Nullable protected Domain loadFromDatabase(String domainName) { Instant now = clock.now();
Shouldn't this use the transaction time?
core/src/main/java/google/registry/cache/MultilayerEppResourceCache.java line 35 at r5 (raw file):
Caffeine.newBuilder() .expireAfterWrite(Duration.ofHours(1)) .maximumSize(RegistryConfig.getEppResourceMaxCachedEntries())
Just to point out that you're doubling the max number of cached entries vs what existed previously, as we used to have a combined domain and host cache and now you're having two separate ones. So at a minimum, the documentation of this option in the config file should change. (And maybe it should be two separate ones, one for domains and one for hosts.)
core/src/main/java/google/registry/cache/MultilayerHostCache.java line 42 at r5 (raw file):
@Override @Nullable protected Host loadFromDatabase(String repoId) {
Seems like it should should return Optional<Host>
core/src/main/java/google/registry/cache/SimplifiedJedisClient.java line 52 at r5 (raw file):
} /** Gets the value from the remote cache. Returns null if it does not exist. */
Why is this returning null if it doesn't exist rather than Optional<V>?
core/src/main/java/google/registry/config/files/default-config.yaml line 643 at r5 (raw file):
valkey: # Optional: hosts and ports for remote Valkey caching
Add information on expected format here.
We add optional Valkey caching of hosts and domains for future use. Eventually, this will allow us to pre-warm large amounts of data in Valkey for quick retrieval during actions like RDAP. Note: this doesn't actually use the caches yet. We use Jedis instead of Redisson for speed purposes (https://www.instaclustr.com/blog/redis-java-clients-and-client-side-caching/) which means that we have to implement our own multilayer cache but that's not the worst thing in the world. Tested on crash with logging and RDAP code that's not included in this PR -- it behaves as you'd expect, where the local cache works for immediate re-lookups and the remote cache works after a restart.
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman made 6 comments.
Reviewable status: 0 of 49 files reviewed, 6 unresolved discussions (waiting on CydeWeys).
core/src/main/java/google/registry/cache/CacheModule.java line 145 at r5 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Store this to a local variable outside the for loop and return it as an inner exception when throwing
CertificateExceptionbelow. Otherwise, as currently written, you're swallowing all exceptions that might actually contain hints as to why things aren't working, and just throwing the string"None of the server certificates were signed by the provided CA".
good point, though we'll just do the last one
core/src/main/java/google/registry/cache/MultilayerDomainCache.java line 49 at r5 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Shouldn't this use the transaction time?
We're not in a transaction here. This is a bit tricky because we'd reeeeeeeally rather not re-implement the foreign key loading logic here.
Anything using this cache should tolerate stale data anyway, but it's probably a good move to move the clock.now() to after the ForeignKeyUtils call.
core/src/main/java/google/registry/cache/MultilayerEppResourceCache.java line 35 at r5 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Just to point out that you're doubling the max number of cached entries vs what existed previously, as we used to have a combined domain and host cache and now you're having two separate ones. So at a minimum, the documentation of this option in the config file should change. (And maybe it should be two separate ones, one for domains and one for hosts.)
depending on configuration, it may end up being less than double, actually.
Previously we have fk->resource cache and repoId->resource caches (though the latter is currently disabled at least in prod right now). This adds fk->domain and repoId->host caches, but once we're done with the code changes, the pubapi instances will only use these new caches and the frontend instances will only use the old cache(s).
But no matter what, we should absolutely CRANK this anyway. It's currently set at the default, which is 500. EppResources are not that big.
core/src/main/java/google/registry/cache/MultilayerHostCache.java line 42 at r5 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Seems like it should should return
Optional<Host>
Done.
core/src/main/java/google/registry/cache/SimplifiedJedisClient.java line 52 at r5 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Why is this returning
nullif it doesn't exist rather thanOptional<V>?
mostly just trying to mirror the cache / jedis API
i don't have a strong opinion on the forever-argument of when to use optional vs null, so let's use optionals
core/src/main/java/google/registry/config/files/default-config.yaml line 643 at r5 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Add information on expected format here.
Done.
gbrodman
left a comment
There was a problem hiding this comment.
ptal
@gbrodman made 1 comment.
Reviewable status: 0 of 49 files reviewed, 6 unresolved discussions (waiting on CydeWeys).
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys resolved 6 discussions.
Reviewable status: 0 of 49 files reviewed, all discussions resolved.
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys reviewed 49 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on gbrodman).
We add optional Valkey caching of hosts and domains for future use. Eventually, this will allow us to pre-warm large amounts of data in Valkey for quick retrieval during actions like RDAP.
Note: this doesn't actually use the caches yet.
We use Jedis instead of Redisson for speed purposes
(https://www.instaclustr.com/blog/redis-java-clients-and-client-side-caching/)
which means that we have to implement our own multilayer cache but
that's not the worst thing in the world.
Tested on crash with logging and RDAP code that's not included in this
PR -- it behaves as you'd expect, where the local cache works for
immediate re-lookups and the remote cache works after a restart.
This change is