Add expiration/deletion to Valkey cache#3024
Conversation
ptkach
left a comment
There was a problem hiding this comment.
@ptkach reviewed 5 files and all commit messages, and made 3 comments.
Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on CydeWeys and gbrodman).
core/src/main/java/google/registry/cache/MultilayerDomainCache.java line 61 at r1 (raw file):
@Override protected String getJedisPrefix() { return "Domain__";
I'd missed the review on this and it's not major, but did we consider a smaller prefix for domains, like "d_" for instance? I mean these 8 bytes will add up
core/src/main/java/google/registry/cache/SimplifiedJedisClient.java line 75 at r1 (raw file):
resource.key.getBytes(StandardCharsets.UTF_8), serialize(resource.value), new SetParams().pxAt(resource.value.getDeletionTime().toEpochMilli()));
What's your plan on deletion? I'm concerned with majority of domains being set as deleted at the end of time these will never get expired on its own .
core/src/main/java/google/registry/cache/SimplifiedJedisClient.java line 111 at r1 (raw file):
.map(key -> key.getBytes(StandardCharsets.UTF_8)) .toArray(byte[][]::new); jedis.unlink(keysToUnlink);
unlink - good choice!
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman made 2 comments.
Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on CydeWeys and ptkach).
core/src/main/java/google/registry/cache/MultilayerDomainCache.java line 61 at r1 (raw file):
Previously, ptkach (Pavlo Tkach) wrote…
I'd missed the review on this and it's not major, but did we consider a smaller prefix for domains, like "d_" for instance? I mean these 8 bytes will add up
I wasn't sure what other types of data we'd be storing so I was being overly cautious. Messaging you offline to chat about this
core/src/main/java/google/registry/cache/SimplifiedJedisClient.java line 75 at r1 (raw file):
Previously, ptkach (Pavlo Tkach) wrote…
What's your plan on deletion? I'm concerned with majority of domains being set as deleted at the end of time these will never get expired on its own .
Yep, that's the thinking. The batch job to sync up the cache will update the deletion time when the domain does eventually get deleted
gbrodman
left a comment
There was a problem hiding this comment.
@gbrodman made 1 comment.
Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on CydeWeys and ptkach).
core/src/main/java/google/registry/cache/MultilayerDomainCache.java line 61 at r1 (raw file):
Previously, gbrodman wrote…
I wasn't sure what other types of data we'd be storing so I was being overly cautious. Messaging you offline to chat about this
From offline discussion: moving to e.g. d_ in order to save a bit of space
047ef22 to
ad16afe
Compare
ptkach
left a comment
There was a problem hiding this comment.
@ptkach reviewed 6 files and all commit messages, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on CydeWeys).
This will be necessary for the batch job that runs however often to "catch up" to the existing state. We also only store "real" domains in Valkey.
ad16afe to
8f92452
Compare
This will be necessary for the batch job that runs however often to "catch up" to the existing state.
Also we also only store "real" domains in Valkey.
This change is