From 80eefc6498bccc81b4a9bd929c61d5254a88abe8 Mon Sep 17 00:00:00 2001 From: gbrodman Date: Thu, 7 May 2026 16:28:56 -0400 Subject: [PATCH] Fix remote cache prefixing by handling it all in the Jedis client (#3035) Found this out while testing metrics. In hindsight, not the best idea to handle prefixing outside of the client itself. Instead, we'll enforce the prefixing closer to Valkey, all in one place. --- .../registry/batch/SyncRemoteCacheAction.java | 23 ++--- .../google/registry/cache/CacheModule.java | 16 +--- .../registry/cache/MultilayerDomainCache.java | 9 +- .../cache/MultilayerEppResourceCache.java | 13 ++- .../registry/cache/MultilayerHostCache.java | 9 +- .../registry/cache/SimplifiedJedisClient.java | 65 +++++++++----- .../batch/SyncRemoteCacheActionTest.java | 62 ++++++-------- .../cache/MultilayerDomainCacheTest.java | 11 ++- .../cache/MultilayerHostCacheTest.java | 10 +-- .../cache/SimplifiedJedisClientTest.java | 85 +++++++++---------- 10 files changed, 138 insertions(+), 165 deletions(-) diff --git a/core/src/main/java/google/registry/batch/SyncRemoteCacheAction.java b/core/src/main/java/google/registry/batch/SyncRemoteCacheAction.java index 8877ac482..6ba502b24 100644 --- a/core/src/main/java/google/registry/batch/SyncRemoteCacheAction.java +++ b/core/src/main/java/google/registry/batch/SyncRemoteCacheAction.java @@ -63,25 +63,20 @@ public class SyncRemoteCacheAction implements Runnable { private final LockHandler lockHandler; private final Response response; - private final Optional> domainJedisClient; - private final Optional> hostJedisClient; + private final Optional jedisClient; @Inject public SyncRemoteCacheAction( - LockHandler lockHandler, - Response response, - Optional> domainJedisClient, - Optional> hostJedisClient) { + LockHandler lockHandler, Response response, Optional jedisClient) { this.lockHandler = lockHandler; this.response = response; - this.domainJedisClient = domainJedisClient; - this.hostJedisClient = hostJedisClient; + this.jedisClient = jedisClient; } @Override public void run() { response.setContentType(MediaType.PLAIN_TEXT_UTF_8); - if (domainJedisClient.isEmpty() || hostJedisClient.isEmpty()) { + if (jedisClient.isEmpty()) { response.setStatus(SC_NO_CONTENT); response.setPayload("No Jedis/Valkey configuration found"); return; @@ -134,7 +129,7 @@ public class SyncRemoteCacheAction implements Runnable { return 0; } logger.atInfo().log("Processing %d domains", domains.size()); - processResources(domainJedisClient.get(), domains, Domain::getDomainName); + processResources(Domain.class, domains, Domain::getDomainName); setNewCursorTime(domains, REMOTE_CACHE_DOMAIN_SYNC); return domains.size(); } @@ -154,13 +149,13 @@ public class SyncRemoteCacheAction implements Runnable { return 0; } logger.atInfo().log("Processing %d hosts", hosts.size()); - processResources(hostJedisClient.get(), hosts, Host::getRepoId); + processResources(Host.class, hosts, Host::getRepoId); setNewCursorTime(hosts, REMOTE_CACHE_HOST_SYNC); return hosts.size(); } private void processResources( - SimplifiedJedisClient jedisClient, List resources, Function getKeyFunction) { + Class clazz, List resources, Function getKeyFunction) { ImmutableList.Builder toDeleteBuilder = new ImmutableList.Builder<>(); ImmutableList.Builder> toSaveBuilder = new ImmutableList.Builder<>(); @@ -176,9 +171,9 @@ public class SyncRemoteCacheAction implements Runnable { ImmutableList toDelete = toDeleteBuilder.build(); ImmutableList> toSave = toSaveBuilder.build(); - jedisClient.deleteAll(toDelete); + jedisClient.get().deleteAll(clazz, toDelete); logger.atInfo().log("Invalidated %d from the remote cache", toDelete.size()); - jedisClient.setAll(toSave); + jedisClient.get().setAll(toSave); logger.atInfo().log("Set %d in the remote cache", toSave.size()); } diff --git a/core/src/main/java/google/registry/cache/CacheModule.java b/core/src/main/java/google/registry/cache/CacheModule.java index 3aee2ee8e..cd8e5d46e 100644 --- a/core/src/main/java/google/registry/cache/CacheModule.java +++ b/core/src/main/java/google/registry/cache/CacheModule.java @@ -85,22 +85,14 @@ public final class CacheModule { @Provides @Singleton - public static Optional> provideDomainJedisClient( - Optional jedis) { - return jedis.map(j -> SimplifiedJedisClient.create(Domain.class, j)); - } - - @Provides - @Singleton - public static Optional> provideHostJedisClient( - Optional jedis) { - return jedis.map(j -> SimplifiedJedisClient.create(Host.class, j)); + public static Optional provideJedisClient(Optional jedis) { + return jedis.map(SimplifiedJedisClient::new); } @Provides @Singleton public static DomainCache provideDomainCache( - Optional> domainJedisClient, Clock clock) { + Optional domainJedisClient, Clock clock) { if (domainJedisClient.isEmpty()) { return domainName -> ForeignKeyUtils.loadResourceByCache(Domain.class, domainName, clock.now()); @@ -110,7 +102,7 @@ public final class CacheModule { @Provides @Singleton - public static HostCache provideHostCache(Optional> hostJedisClient) { + public static HostCache provideHostCache(Optional hostJedisClient) { if (hostJedisClient.isEmpty()) { return repoId -> Optional.ofNullable(EppResource.loadByCache(VKey.create(Host.class, repoId))); diff --git a/core/src/main/java/google/registry/cache/MultilayerDomainCache.java b/core/src/main/java/google/registry/cache/MultilayerDomainCache.java index 5a364ba04..024c95102 100644 --- a/core/src/main/java/google/registry/cache/MultilayerDomainCache.java +++ b/core/src/main/java/google/registry/cache/MultilayerDomainCache.java @@ -32,14 +32,14 @@ public class MultilayerDomainCache extends MultilayerEppResourceCache private final Clock clock; - public MultilayerDomainCache(SimplifiedJedisClient jedisClient, Clock clock) { + public MultilayerDomainCache(SimplifiedJedisClient jedisClient, Clock clock) { super(jedisClient); this.clock = clock; } @Override public Optional loadByDomainName(String domainName) { - return loadFromCaches(domainName); + return loadFromCaches(Domain.class, domainName); } @Override @@ -56,11 +56,6 @@ public class MultilayerDomainCache extends MultilayerEppResourceCache .map(domain -> domain.cloneProjectedAtTime(now)); } - @Override - protected String getJedisPrefix() { - return "d_"; - } - @Override protected boolean shouldPersistToRemoteCache(Domain domain) { return Tld.get(domain.getTld()).getTldType().equals(Tld.TldType.REAL); diff --git a/core/src/main/java/google/registry/cache/MultilayerEppResourceCache.java b/core/src/main/java/google/registry/cache/MultilayerEppResourceCache.java index c6a314892..de53f60e4 100644 --- a/core/src/main/java/google/registry/cache/MultilayerEppResourceCache.java +++ b/core/src/main/java/google/registry/cache/MultilayerEppResourceCache.java @@ -35,21 +35,19 @@ public abstract class MultilayerEppResourceCache { .maximumSize(RegistryConfig.getEppResourceMaxCachedEntries()) .build(); - private final SimplifiedJedisClient jedisClient; + private final SimplifiedJedisClient jedisClient; - protected MultilayerEppResourceCache(SimplifiedJedisClient jedisClient) { + protected MultilayerEppResourceCache(SimplifiedJedisClient jedisClient) { this.jedisClient = jedisClient; } protected abstract Optional loadFromDatabase(String key); - protected abstract String getJedisPrefix(); - protected boolean shouldPersistToRemoteCache(V value) { return true; } - protected Optional loadFromCaches(String key) { + protected Optional loadFromCaches(Class clazz, String key) { // hopefully the resource is in the local cache Optional possibleValue = Optional.ofNullable(localCache.getIfPresent(key)); if (possibleValue.isPresent()) { @@ -57,8 +55,7 @@ public abstract class MultilayerEppResourceCache { } // if not, try the remote cache - String jedisKey = getJedisPrefix() + key; - possibleValue = jedisClient.get(jedisKey); + possibleValue = jedisClient.get(clazz, key); if (possibleValue.isPresent()) { localCache.put(key, possibleValue.get()); return possibleValue; @@ -70,7 +67,7 @@ public abstract class MultilayerEppResourceCache { v -> { // Optional has no direct "peek" functionality to fill the caches if (shouldPersistToRemoteCache(v)) { - jedisClient.set(new SimplifiedJedisClient.JedisResource<>(jedisKey, v)); + jedisClient.set(new SimplifiedJedisClient.JedisResource<>(key, v)); } localCache.put(key, v); return v; diff --git a/core/src/main/java/google/registry/cache/MultilayerHostCache.java b/core/src/main/java/google/registry/cache/MultilayerHostCache.java index 87660d238..8bb21be7d 100644 --- a/core/src/main/java/google/registry/cache/MultilayerHostCache.java +++ b/core/src/main/java/google/registry/cache/MultilayerHostCache.java @@ -27,13 +27,13 @@ import java.util.Optional; */ public class MultilayerHostCache extends MultilayerEppResourceCache implements HostCache { - public MultilayerHostCache(SimplifiedJedisClient jedisClient) { + public MultilayerHostCache(SimplifiedJedisClient jedisClient) { super(jedisClient); } @Override public Optional loadByRepoId(String repoId) { - return loadFromCaches(repoId); + return loadFromCaches(Host.class, repoId); } @Override @@ -41,9 +41,4 @@ public class MultilayerHostCache extends MultilayerEppResourceCache implem return replicaTm() .transact(() -> replicaTm().loadByKeyIfPresent(VKey.create(Host.class, repoId))); } - - @Override - protected String getJedisPrefix() { - return "h_"; - } } diff --git a/core/src/main/java/google/registry/cache/SimplifiedJedisClient.java b/core/src/main/java/google/registry/cache/SimplifiedJedisClient.java index 7c09b7003..2f2acc042 100644 --- a/core/src/main/java/google/registry/cache/SimplifiedJedisClient.java +++ b/core/src/main/java/google/registry/cache/SimplifiedJedisClient.java @@ -14,13 +14,17 @@ package google.registry.cache; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import com.google.common.collect.ImmutableCollection; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.collect.Streams; import com.google.common.flogger.FluentLogger; import google.registry.model.EppResource; +import google.registry.model.domain.Domain; +import google.registry.model.host.Host; import io.protostuff.LinkedBuffer; import io.protostuff.ProtostuffIOUtil; import io.protostuff.Schema; @@ -39,54 +43,57 @@ import redis.clients.jedis.params.SetParams; *

{@link UnifiedJedis} pairs key-value types, so we need the key to be serialized to a byte * array as well. */ -public class SimplifiedJedisClient { +public class SimplifiedJedisClient { public record JedisResource(String key, V value) {} + private static final ImmutableMap, String> TYPE_PREFIXES = + ImmutableMap.of( + Domain.class, "d_", + Host.class, "h_"); + + private static final ImmutableMap, Schema> + VALUE_SCHEMAS = + ImmutableMap.of( + Domain.class, RuntimeSchema.getSchema(Domain.class), + Host.class, RuntimeSchema.getSchema(Host.class)); + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private static final int BATCH_SIZE = 500; - private final Schema valueSchema; private final UnifiedJedis jedis; - public static SimplifiedJedisClient create( - Class valueClass, UnifiedJedis jedis) { - Schema valueSchema = RuntimeSchema.getSchema(valueClass); - return new SimplifiedJedisClient<>(valueSchema, jedis); - } - - private SimplifiedJedisClient(Schema valueSchema, UnifiedJedis jedis) { - this.valueSchema = valueSchema; + SimplifiedJedisClient(UnifiedJedis jedis) { this.jedis = jedis; } /** Gets the value from the remote cache. Returns null if it does not exist. */ - public Optional get(String key) { + public Optional get(Class clazz, String key) { checkNotNull(key, "Key cannot be null"); - byte[] data = jedis.get(key.getBytes(StandardCharsets.UTF_8)); - return Optional.ofNullable(data).map(this::deserialize); + byte[] data = jedis.get(convertKey(clazz, key)); + return Optional.ofNullable(data).map(d -> deserialize(clazz, d)); } /** Sets the value in the remote cache. */ - public void set(JedisResource resource) { + public void set(JedisResource resource) { checkNotNull(resource.key, "Key cannot be null"); checkNotNull(resource.value, "Value cannot be null"); jedis.set( - resource.key.getBytes(StandardCharsets.UTF_8), + convertKey(resource.value.getClass(), resource.key), serialize(resource.value), new SetParams().pxAt(resource.value.getDeletionTime().toEpochMilli())); } /** Sets multiple values in the remote cache using a Jedis {@link AbstractPipeline}. */ - public void setAll(ImmutableCollection> resources) { + public void setAll(ImmutableCollection> resources) { logger.atInfo().log("Processing %d resources", resources.size()); for (Iterable> batch : Iterables.partition(resources, BATCH_SIZE)) { try (AbstractPipeline pipeline = jedis.pipelined()) { batch.forEach( resource -> pipeline.set( - resource.key.getBytes(StandardCharsets.UTF_8), + convertKey(resource.value.getClass(), resource.key), serialize(resource.value), new SetParams().pxAt(resource.value.getDeletionTime().toEpochMilli()))); pipeline.sync(); @@ -106,18 +113,18 @@ public class SimplifiedJedisClient { *

This could also be accomplished by using {@link #setAll(ImmutableCollection)} with * expiration times that are in the past, but this is clearer. */ - public void deleteAll(ImmutableCollection keys) { + public void deleteAll(Class valueType, ImmutableCollection keys) { // we use a reasonably small batch size to avoid overwhelming the network for (Iterable batch : Iterables.partition(keys, BATCH_SIZE)) { byte[][] keysToUnlink = - Streams.stream(batch) - .map(key -> key.getBytes(StandardCharsets.UTF_8)) - .toArray(byte[][]::new); + Streams.stream(batch).map(key -> convertKey(valueType, key)).toArray(byte[][]::new); jedis.unlink(keysToUnlink); } } - private byte[] serialize(V value) { + private byte[] serialize(V value) { + @SuppressWarnings("unchecked") + Schema valueSchema = (Schema) getValueSchema(value.getClass()); LinkedBuffer buffer = LinkedBuffer.allocate(LinkedBuffer.DEFAULT_BUFFER_SIZE); try { return ProtostuffIOUtil.toByteArray(value, valueSchema, buffer); @@ -126,10 +133,22 @@ public class SimplifiedJedisClient { } } - private V deserialize(byte[] data) { + private V deserialize(Class clazz, byte[] data) { // We use protobufs because other deserializers don't play nicely with immutable collections + Schema valueSchema = getValueSchema(clazz); V value = valueSchema.newMessage(); ProtostuffIOUtil.mergeFrom(data, value, valueSchema); return value; } + + private byte[] convertKey(Class clazz, String key) { + checkArgument(TYPE_PREFIXES.containsKey(clazz), "Unknown class type %s", clazz); + return (TYPE_PREFIXES.get(clazz) + key).getBytes(StandardCharsets.UTF_8); + } + + @SuppressWarnings("unchecked") + private Schema getValueSchema(Class clazz) { + checkArgument(VALUE_SCHEMAS.containsKey(clazz), "Unknown class type %s", clazz); + return (Schema) VALUE_SCHEMAS.get(clazz); + } } diff --git a/core/src/test/java/google/registry/batch/SyncRemoteCacheActionTest.java b/core/src/test/java/google/registry/batch/SyncRemoteCacheActionTest.java index cd08a4663..ae4d1747d 100644 --- a/core/src/test/java/google/registry/batch/SyncRemoteCacheActionTest.java +++ b/core/src/test/java/google/registry/batch/SyncRemoteCacheActionTest.java @@ -26,7 +26,6 @@ import static jakarta.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR; import static jakarta.servlet.http.HttpServletResponse.SC_NO_CONTENT; import static jakarta.servlet.http.HttpServletResponse.SC_OK; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; @@ -65,8 +64,7 @@ class SyncRemoteCacheActionTest { final JpaIntegrationTestExtension jpa = new JpaTestExtensions.Builder().withClock(clock).buildIntegrationTestExtension(); - @Mock private SimplifiedJedisClient domainJedisClient; - @Mock private SimplifiedJedisClient hostJedisClient; + @Mock private SimplifiedJedisClient jedisClient; private final FakeResponse response = new FakeResponse(); private FakeLockHandler lockHandler = new FakeLockHandler(true); @@ -75,14 +73,12 @@ class SyncRemoteCacheActionTest { @BeforeEach void beforeEach() { createTld("tld"); - action = - new SyncRemoteCacheAction( - lockHandler, response, Optional.of(domainJedisClient), Optional.of(hostJedisClient)); + action = new SyncRemoteCacheAction(lockHandler, response, Optional.of(jedisClient)); } @Test void test_noJedisConfig() { - action = new SyncRemoteCacheAction(lockHandler, response, Optional.empty(), Optional.empty()); + action = new SyncRemoteCacheAction(lockHandler, response, Optional.empty()); action.run(); assertThat(response.getStatus()).isEqualTo(SC_NO_CONTENT); assertThat(response.getPayload()).contains("No Jedis/Valkey configuration found"); @@ -91,9 +87,7 @@ class SyncRemoteCacheActionTest { @Test void test_lockAcquisitionFails() { lockHandler = new FakeLockHandler(false); - action = - new SyncRemoteCacheAction( - lockHandler, response, Optional.of(domainJedisClient), Optional.of(hostJedisClient)); + action = new SyncRemoteCacheAction(lockHandler, response, Optional.of(jedisClient)); action.run(); assertThat(response.getStatus()).isEqualTo(SC_NO_CONTENT); assertThat(response.getPayload()).contains("Could not acquire lock"); @@ -101,7 +95,7 @@ class SyncRemoteCacheActionTest { @Test void test_exceptionThrown() { - doThrow(new RuntimeException("Redis failed")).when(domainJedisClient).deleteAll(any()); + doThrow(new RuntimeException("Redis failed")).when(jedisClient).deleteAll(any(), any()); persistActiveDomain("example.tld"); // So there is something to process action.run(); assertThat(response.getStatus()).isEqualTo(SC_INTERNAL_SERVER_ERROR); @@ -112,7 +106,7 @@ class SyncRemoteCacheActionTest { void test_syncDomains_noDomains() { action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - verifyNoInteractions(domainJedisClient); + verifyNoInteractions(jedisClient); assertThat(DatabaseHelper.loadByKeyIfPresent(Cursor.createGlobalVKey(REMOTE_CACHE_DOMAIN_SYNC))) .isEmpty(); } @@ -126,12 +120,11 @@ class SyncRemoteCacheActionTest { action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - verify(domainJedisClient) + verify(jedisClient) .setAll( - eq( - ImmutableList.of( - new SimplifiedJedisClient.JedisResource<>("example1.tld", domain1), - new SimplifiedJedisClient.JedisResource<>("example2.tld", domain2)))); + ImmutableList.of( + new SimplifiedJedisClient.JedisResource<>("example1.tld", domain1), + new SimplifiedJedisClient.JedisResource<>("example2.tld", domain2))); assertThat( DatabaseHelper.loadByKey(Cursor.createGlobalVKey(REMOTE_CACHE_DOMAIN_SYNC)) @@ -148,12 +141,11 @@ class SyncRemoteCacheActionTest { action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - verify(domainJedisClient) + verify(jedisClient) .setAll( - eq( - ImmutableList.of( - new SimplifiedJedisClient.JedisResource<>("active.tld", activeDomain)))); - verify(domainJedisClient).deleteAll(eq(ImmutableList.of("deleted.tld"))); + ImmutableList.of( + new SimplifiedJedisClient.JedisResource<>("active.tld", activeDomain))); + verify(jedisClient).deleteAll(Domain.class, ImmutableList.of("deleted.tld")); } @Test @@ -171,18 +163,16 @@ class SyncRemoteCacheActionTest { action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - verify(domainJedisClient) + verify(jedisClient) .setAll( - eq( - ImmutableList.of( - new SimplifiedJedisClient.JedisResource<>("example2.tld", domain2)))); + ImmutableList.of(new SimplifiedJedisClient.JedisResource<>("example2.tld", domain2))); } @Test void test_syncHosts_noHosts() { action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - verifyNoInteractions(hostJedisClient); + verifyNoInteractions(jedisClient); assertThat(DatabaseHelper.loadByKeyIfPresent(Cursor.createGlobalVKey(REMOTE_CACHE_HOST_SYNC))) .isEmpty(); } @@ -196,12 +186,11 @@ class SyncRemoteCacheActionTest { action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - verify(hostJedisClient) + verify(jedisClient) .setAll( - eq( - ImmutableList.of( - new SimplifiedJedisClient.JedisResource<>(host1.getRepoId(), host1), - new SimplifiedJedisClient.JedisResource<>(host2.getRepoId(), host2)))); + ImmutableList.of( + new SimplifiedJedisClient.JedisResource<>(host1.getRepoId(), host1), + new SimplifiedJedisClient.JedisResource<>(host2.getRepoId(), host2))); assertThat( DatabaseHelper.loadByKey(Cursor.createGlobalVKey(REMOTE_CACHE_HOST_SYNC)) @@ -218,11 +207,10 @@ class SyncRemoteCacheActionTest { action.run(); assertThat(response.getStatus()).isEqualTo(SC_OK); - verify(hostJedisClient) + verify(jedisClient) .setAll( - eq( - ImmutableList.of( - new SimplifiedJedisClient.JedisResource<>(active.getRepoId(), active)))); - verify(hostJedisClient).deleteAll(eq(ImmutableList.of(deleted.getRepoId()))); + ImmutableList.of( + new SimplifiedJedisClient.JedisResource<>(active.getRepoId(), active))); + verify(jedisClient).deleteAll(Host.class, ImmutableList.of(deleted.getRepoId())); } } diff --git a/core/src/test/java/google/registry/cache/MultilayerDomainCacheTest.java b/core/src/test/java/google/registry/cache/MultilayerDomainCacheTest.java index db79aded8..f09302e18 100644 --- a/core/src/test/java/google/registry/cache/MultilayerDomainCacheTest.java +++ b/core/src/test/java/google/registry/cache/MultilayerDomainCacheTest.java @@ -18,7 +18,6 @@ import static com.google.common.truth.Truth.assertThat; import static google.registry.testing.DatabaseHelper.createTld; import static google.registry.testing.DatabaseHelper.persistActiveDomain; import static google.registry.testing.DatabaseHelper.persistResource; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -42,7 +41,7 @@ public class MultilayerDomainCacheTest { final JpaIntegrationTestExtension jpa = new JpaTestExtensions.Builder().buildIntegrationTestExtension(); - private final SimplifiedJedisClient jedisClient = mock(SimplifiedJedisClient.class); + private final SimplifiedJedisClient jedisClient = mock(SimplifiedJedisClient.class); private final FakeClock clock = new FakeClock(); private MultilayerDomainCache cache; @@ -58,8 +57,8 @@ public class MultilayerDomainCacheTest { assertThat(cache.loadByDomainName("example.tld")).hasValue(domain); // We should have filled the caches after one attempt to load from Valkey - verify(jedisClient).get("d_example.tld"); - verify(jedisClient).set(new SimplifiedJedisClient.JedisResource<>("d_example.tld", domain)); + verify(jedisClient).get(Domain.class, "example.tld"); + verify(jedisClient).set(new SimplifiedJedisClient.JedisResource<>("example.tld", domain)); // Further loads hit the local cache assertThat(cache.loadByDomainName("example.tld")).hasValue(domain); @@ -71,7 +70,7 @@ public class MultilayerDomainCacheTest { // Note: we don't save the domain to SQL Domain domain = DatabaseHelper.newDomain("example.tld"); // We hit the Valkey cache first - when(jedisClient.get(eq("d_example.tld"))).thenReturn(Optional.of(domain)); + when(jedisClient.get(Domain.class, "example.tld")).thenReturn(Optional.of(domain)); assertThat(cache.loadByDomainName("example.tld")).hasValue(domain); } @@ -83,7 +82,7 @@ public class MultilayerDomainCacheTest { assertThat(cache.loadByDomainName("example.tld")).hasValue(domain); // This time, we don't populate the remote cache because it's prober data - verify(jedisClient).get("d_example.tld"); + verify(jedisClient).get(Domain.class, "example.tld"); verifyNoMoreInteractions(jedisClient); } diff --git a/core/src/test/java/google/registry/cache/MultilayerHostCacheTest.java b/core/src/test/java/google/registry/cache/MultilayerHostCacheTest.java index db0f11b52..3a2dedf38 100644 --- a/core/src/test/java/google/registry/cache/MultilayerHostCacheTest.java +++ b/core/src/test/java/google/registry/cache/MultilayerHostCacheTest.java @@ -16,7 +16,6 @@ package google.registry.cache; import static com.google.common.truth.Truth.assertThat; import static google.registry.testing.DatabaseHelper.persistActiveHost; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -38,7 +37,7 @@ public class MultilayerHostCacheTest { final JpaIntegrationTestExtension jpa = new JpaTestExtensions.Builder().buildIntegrationTestExtension(); - private final SimplifiedJedisClient jedisClient = mock(SimplifiedJedisClient.class); + private final SimplifiedJedisClient jedisClient = mock(SimplifiedJedisClient.class); private MultilayerHostCache cache; @BeforeEach @@ -52,9 +51,8 @@ public class MultilayerHostCacheTest { assertThat(cache.loadByRepoId(host.getRepoId())).hasValue(host); // We should have filled the caches after one attempt to load from Valkey - verify(jedisClient).get("h_" + host.getRepoId()); - verify(jedisClient) - .set(new SimplifiedJedisClient.JedisResource<>("h_" + host.getRepoId(), host)); + verify(jedisClient).get(Host.class, host.getRepoId()); + verify(jedisClient).set(new SimplifiedJedisClient.JedisResource<>(host.getRepoId(), host)); // Further loads hit the local cache assertThat(cache.loadByRepoId(host.getRepoId())).hasValue(host); @@ -66,7 +64,7 @@ public class MultilayerHostCacheTest { // Note: we don't save the host to SQL Host host = DatabaseHelper.newHost("ns1.example.tld"); // We hit the Valkey cache first - when(jedisClient.get(eq("h_" + host.getRepoId()))).thenReturn(Optional.of(host)); + when(jedisClient.get(Host.class, host.getRepoId())).thenReturn(Optional.of(host)); assertThat(cache.loadByRepoId(host.getRepoId())).hasValue(host); } diff --git a/core/src/test/java/google/registry/cache/SimplifiedJedisClientTest.java b/core/src/test/java/google/registry/cache/SimplifiedJedisClientTest.java index 67e5df3bb..0811e1f20 100644 --- a/core/src/test/java/google/registry/cache/SimplifiedJedisClientTest.java +++ b/core/src/test/java/google/registry/cache/SimplifiedJedisClientTest.java @@ -23,7 +23,6 @@ import static google.registry.testing.DatabaseHelper.persistDeletedDomain; import static org.joda.time.DateTimeZone.UTC; import com.google.common.collect.ImmutableList; -import google.registry.model.EppResource; import google.registry.model.domain.Domain; import google.registry.model.host.Host; import google.registry.persistence.transaction.JpaTestExtensions; @@ -38,7 +37,6 @@ import org.testcontainers.junit.jupiter.Container; import org.testcontainers.junit.jupiter.Testcontainers; import redis.clients.jedis.HostAndPort; import redis.clients.jedis.RedisClient; -import redis.clients.jedis.UnifiedJedis; /** Tests for {@link SimplifiedJedisClient}. */ @Testcontainers @@ -60,29 +58,29 @@ public class SimplifiedJedisClientTest { @Test void testClient_roundTrip_domain() { Domain domain = persistActiveDomain("example.tld"); - SimplifiedJedisClient client = createSimplifiedClient(Domain.class); - client.set(new SimplifiedJedisClient.JedisResource<>("d_example.tld", domain)); + SimplifiedJedisClient client = createJedisClient(); + client.set(new SimplifiedJedisClient.JedisResource<>("example.tld", domain)); // dsData and gracePeriods get serialized as null instead of the empty set, which is fine assertAboutImmutableObjects() - .that(client.get("d_example.tld").get()) + .that(client.get(Domain.class, "example.tld").get()) .isEqualExceptFields(domain, "dsData", "gracePeriods"); } @Test void testClient_roundTrip_host() { Host host = persistActiveHost("ns1.example.tld"); - SimplifiedJedisClient client = createSimplifiedClient(Host.class); - client.set(new SimplifiedJedisClient.JedisResource<>("h_repoId1", host)); - assertThat(client.get("h_repoId1")).hasValue(host); + SimplifiedJedisClient client = createJedisClient(); + client.set(new SimplifiedJedisClient.JedisResource<>("repoId1", host)); + assertThat(client.get(Host.class, "repoId1")).hasValue(host); } @Test void testSet_withExpiration() throws Exception { - SimplifiedJedisClient client = createSimplifiedClient(Domain.class); + SimplifiedJedisClient client = createJedisClient(); Domain pendingDelete = persistDeletedDomain("example.tld", DateTime.now(UTC).plusMillis(100)); - client.set(new SimplifiedJedisClient.JedisResource<>("d_example1.tld", pendingDelete)); + client.set(new SimplifiedJedisClient.JedisResource<>("example1.tld", pendingDelete)); Thread.sleep(101); - assertThat(client.get("d_example1.tld")).isEmpty(); + assertThat(client.get(Domain.class, "example1.tld")).isEmpty(); } @Test @@ -90,22 +88,22 @@ public class SimplifiedJedisClientTest { Domain domain1 = persistActiveDomain("example1.tld"); Domain domain2 = persistActiveDomain("example2.tld"); Domain domain3 = persistActiveDomain("example3.tld"); - SimplifiedJedisClient client = createSimplifiedClient(Domain.class); + SimplifiedJedisClient client = createJedisClient(); client.setAll( ImmutableList.of( - new SimplifiedJedisClient.JedisResource<>("d_example1.tld", domain1), - new SimplifiedJedisClient.JedisResource<>("d_example2.tld", domain2), - new SimplifiedJedisClient.JedisResource<>("d_example3.tld", domain3))); + new SimplifiedJedisClient.JedisResource<>("example1.tld", domain1), + new SimplifiedJedisClient.JedisResource<>("example2.tld", domain2), + new SimplifiedJedisClient.JedisResource<>("example3.tld", domain3))); assertAboutImmutableObjects() - .that(client.get("d_example1.tld").get()) + .that(client.get(Domain.class, "example1.tld").get()) .isEqualExceptFields(domain1, "dsData", "gracePeriods"); assertAboutImmutableObjects() - .that(client.get("d_example2.tld").get()) + .that(client.get(Domain.class, "example2.tld").get()) .isEqualExceptFields(domain2, "dsData", "gracePeriods"); assertAboutImmutableObjects() - .that(client.get("d_example3.tld").get()) + .that(client.get(Domain.class, "example3.tld").get()) .isEqualExceptFields(domain3, "dsData", "gracePeriods"); } @@ -114,17 +112,17 @@ public class SimplifiedJedisClientTest { Host host1 = persistActiveHost("ns1.example.tld"); Host host2 = persistActiveHost("ns2.example.tld"); Host host3 = persistActiveHost("ns3.example.tld"); - SimplifiedJedisClient client = createSimplifiedClient(Host.class); + SimplifiedJedisClient client = createJedisClient(); client.setAll( ImmutableList.of( - new SimplifiedJedisClient.JedisResource<>("h_repoId1", host1), - new SimplifiedJedisClient.JedisResource<>("h_repoId2", host2), - new SimplifiedJedisClient.JedisResource<>("h_repoId3", host3))); + new SimplifiedJedisClient.JedisResource<>("repoId1", host1), + new SimplifiedJedisClient.JedisResource<>("repoId2", host2), + new SimplifiedJedisClient.JedisResource<>("repoId3", host3))); - assertThat(client.get("h_repoId1")).hasValue(host1); - assertThat(client.get("h_repoId2")).hasValue(host2); - assertThat(client.get("h_repoId3")).hasValue(host3); + assertThat(client.get(Host.class, "repoId1")).hasValue(host1); + assertThat(client.get(Host.class, "repoId2")).hasValue(host2); + assertThat(client.get(Host.class, "repoId3")).hasValue(host3); } @Test @@ -132,35 +130,32 @@ public class SimplifiedJedisClientTest { Host host1 = persistActiveHost("ns1.example.tld"); Host host2 = persistActiveHost("ns2.example.tld"); Host host3 = persistActiveHost("ns3.example.tld"); - SimplifiedJedisClient client = createSimplifiedClient(Host.class); + SimplifiedJedisClient client = createJedisClient(); client.setAll( ImmutableList.of( - new SimplifiedJedisClient.JedisResource<>("h_repoId1", host1), - new SimplifiedJedisClient.JedisResource<>("h_repoId2", host2), - new SimplifiedJedisClient.JedisResource<>("h_repoId3", host3))); + new SimplifiedJedisClient.JedisResource<>("repoId1", host1), + new SimplifiedJedisClient.JedisResource<>("repoId2", host2), + new SimplifiedJedisClient.JedisResource<>("repoId3", host3))); - client.deleteAll(ImmutableList.of("h_repoId1", "h_repoId2", "h_nonexistent")); - assertThat(client.get("h_repoId1")).isEmpty(); - assertThat(client.get("h_repoId2")).isEmpty(); - assertThat(client.get("h_repoId3")).hasValue(host3); + client.deleteAll(Host.class, ImmutableList.of("repoId1", "repoId2", "nonexistent")); + assertThat(client.get(Host.class, "repoId1")).isEmpty(); + assertThat(client.get(Host.class, "repoId2")).isEmpty(); + assertThat(client.get(Host.class, "repoId3")).hasValue(host3); } @Test void testClient_nonexistent() { - SimplifiedJedisClient domainClient = createSimplifiedClient(Domain.class); - SimplifiedJedisClient hostClient = createSimplifiedClient(Host.class); - assertThat(domainClient.get("d_nonexistent.tld")).isEmpty(); - assertThat(hostClient.get("h_ns1.nonexistent.tld")).isEmpty(); + SimplifiedJedisClient domainClient = createJedisClient(); + SimplifiedJedisClient hostClient = createJedisClient(); + assertThat(domainClient.get(Domain.class, "nonexistent.tld")).isEmpty(); + assertThat(hostClient.get(Host.class, "ns1.nonexistent.tld")).isEmpty(); } - private SimplifiedJedisClient createSimplifiedClient(Class clazz) { - return SimplifiedJedisClient.create(clazz, createJedisClient()); - } - - private UnifiedJedis createJedisClient() { - return RedisClient.builder() - .hostAndPort(new HostAndPort(valkey.getHost(), valkey.getFirstMappedPort())) - .build(); + private SimplifiedJedisClient createJedisClient() { + return new SimplifiedJedisClient( + RedisClient.builder() + .hostAndPort(new HostAndPort(valkey.getHost(), valkey.getFirstMappedPort())) + .build()); } }