Jedis test plan coverage for CSC (#3918)

* initial changes * cover tests for JedisPooled and functionality * fix javadoc * cover new tests for JedisCluster and JedisSentineled * Fix CSC allow-and-deny-list and rename Cacheable interface * Tag CommandArguments#getKeys() as Internal * cover lruEvictionTest * Address code reviews and more updates * fix format and more minor changes * format Connection * modify WeakReference usage
redis · Aug 15, 2024 · 9bddabd · 9bddabd
1 parent f78fc08
commit 9bddabd
Show file tree

Hide file tree

Showing 30 changed files with 894 additions and 1,073 deletions.
diff --git a/pom.xml b/pom.xml
@@ -76,19 +76,6 @@
 		</dependency>
 
 		<!-- Optional dependencies -->
-		<!-- Client-side caching -->
-		<dependency>
-			<groupId>com.google.guava</groupId>
-			<artifactId>guava</artifactId>
-			<version>33.2.1-jre</version>
-			<optional>true</optional>
-		</dependency>
-		<dependency>
-			<groupId>com.github.ben-manes.caffeine</groupId>
-			<artifactId>caffeine</artifactId>
-			<version>2.9.3</version>
-			<optional>true</optional>
-		</dependency>
 
 		<!-- UNIX socket connection support -->
 		<dependency>

diff --git a/src/main/java/redis/clients/jedis/CommandArguments.java b/src/main/java/redis/clients/jedis/CommandArguments.java
@@ -3,8 +3,11 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Iterator;
+import java.util.List;
 
+import redis.clients.jedis.annots.Internal;
 import redis.clients.jedis.args.Rawable;
 import redis.clients.jedis.args.RawableFactory;
 import redis.clients.jedis.commands.ProtocolCommand;
@@ -14,14 +17,20 @@
 public class CommandArguments implements Iterable<Rawable> {
 
   private final ArrayList<Rawable> args;
-  private final ArrayList<Object> keys;
+
+  private List<Object> keys;
 
   private boolean blocking;
 
+  private CommandArguments() {
+    throw new InstantiationError();
+  }
+
   public CommandArguments(ProtocolCommand command) {
     args = new ArrayList<>();
     args.add(command);
-    keys = new ArrayList<>();
+
+    keys = Collections.emptyList();
   }
 
   public ProtocolCommand getCommand() {
@@ -113,10 +122,25 @@ public CommandArguments key(Object key) {
     } else {
       throw new IllegalArgumentException("\"" + key.toString() + "\" is not a valid argument.");
     }
-    keys.add(key);
+
+    addKeyInKeys(key);
+
     return this;
   }
 
+  private void addKeyInKeys(Object key) {
+    if (keys.isEmpty()) {
+      keys = Collections.singletonList(key);
+    } else if (keys.size() == 1) {
+      List oldKeys = keys;
+      keys = new ArrayList();
+      keys.addAll(oldKeys);
+      keys.add(key);
+    } else {
+      keys.add(key);
+    }
+  }
+
   public final CommandArguments keys(Object... keys) {
     Arrays.stream(keys).forEach(this::key);
     return this;
@@ -133,6 +157,7 @@ public final CommandArguments addParams(IParams params) {
   }
 
   protected CommandArguments processKey(byte[] key) {
+    // do nothing
     return this;
   }
 
@@ -144,6 +169,7 @@ protected final CommandArguments processKeys(byte[]... keys) {
   }
 
   protected CommandArguments processKey(String key) {
+    // do nothing
     return this;
   }
 
@@ -163,8 +189,9 @@ public Iterator<Rawable> iterator() {
     return args.iterator();
   }
 
-  public Object[] getKeys() {
-    return keys.toArray();
+  @Internal
+  public List<Object> getKeys() {
+    return keys;
   }
 
   public boolean isBlocking() {

diff --git a/src/main/java/redis/clients/jedis/Connection.java b/src/main/java/redis/clients/jedis/Connection.java
@@ -102,7 +102,7 @@ public String toIdentityString() {
           .append(id)
           .append(", L:")
           .append(localAddr)
-          .append(broken? " ! " : " - ")
+          .append(broken ? " ! " : " - ")
           .append("R:")
           .append(remoteAddr)
           .append(']');
@@ -455,7 +455,7 @@ public List<Object> getMany(final int count) {
 
   /**
    * Check if the client name libname, libver, characters are legal
-   * 
+   *
    * @param info the name
    * @return Returns true if legal, false throws exception
    * @throws JedisException if characters illegal
@@ -499,8 +499,9 @@ protected void initializeFromClientConfig(final JedisClientConfig config) {
       }
 
       ClientSetInfoConfig setInfoConfig = config.getClientSetInfoConfig();
-      if (setInfoConfig == null)
+      if (setInfoConfig == null) {
         setInfoConfig = ClientSetInfoConfig.DEFAULT;
+      }
 
       if (!setInfoConfig.isDisabled()) {
         String libName = JedisMetaInfo.getArtifactId();

diff --git a/src/main/java/redis/clients/jedis/csc/AbstractCache.java b/src/main/java/redis/clients/jedis/csc/AbstractCache.java
@@ -14,25 +14,23 @@
 import redis.clients.jedis.util.SafeEncoder;
 
 /**
- * The class to manage the client-side caching. User can provide any of implementation of this class
- * to the client object; e.g. {@link redis.clients.jedis.csc.CaffeineClientSideCache
- * CaffeineClientSideCache} or {@link redis.clients.jedis.csc.GuavaClientSideCache
- * GuavaClientSideCache} or a custom implementation of their own.
+ * The class to manage the client-side caching. User can provide an of implementation of this class
+ * to the client object.
  */
 @Experimental
 public abstract class AbstractCache implements Cache {
 
-  private ClientSideCacheable cacheable;
+  private Cacheable cacheable;
   private final Map<ByteBuffer, Set<CacheKey<?>>> redisKeysToCacheKeys = new ConcurrentHashMap<>();
   private final int maximumSize;
   private ReentrantLock lock = new ReentrantLock();
   private volatile CacheStats stats = new CacheStats();
 
   protected AbstractCache(int maximumSize) {
-    this(maximumSize, DefaultClientSideCacheable.INSTANCE);
+    this(maximumSize, DefaultCacheable.INSTANCE);
   }
 
-  protected AbstractCache(int maximumSize, ClientSideCacheable cacheable) {
+  protected AbstractCache(int maximumSize, Cacheable cacheable) {
     this.maximumSize = maximumSize;
     this.cacheable = cacheable;
   }
@@ -89,7 +87,7 @@ public CacheEntry set(CacheKey cacheKey, CacheEntry entry) {
   }
 
   @Override
-  public Boolean delete(CacheKey cacheKey) {
+  public boolean delete(CacheKey cacheKey) {
     lock.lock();
     try {
       boolean removed = removeFromStore(cacheKey);
@@ -171,12 +169,12 @@ public int flush() {
   }
 
   @Override
-  public Boolean isCacheable(CacheKey cacheKey) {
-    return cacheable.isCacheable(cacheKey.getCommand().getArguments().getCommand(), cacheKey.getRedisKeys());
+  public boolean isCacheable(CacheKey cacheKey) {
+    return cacheable.isCacheable(cacheKey.getRedisCommand(), cacheKey.getRedisKeys());
   }
 
   @Override
-  public Boolean hasCacheKey(CacheKey cacheKey) {
+  public boolean hasCacheKey(CacheKey cacheKey) {
     return containsKeyInStore(cacheKey);
   }
 
@@ -202,13 +200,13 @@ public CacheStats getAndResetStats() {
 
   protected abstract CacheEntry putIntoStore(CacheKey cacheKey, CacheEntry entry);
 
-  protected abstract Boolean removeFromStore(CacheKey cacheKey);
+  protected abstract boolean removeFromStore(CacheKey cacheKey);
 
   // protected abstract Collection<CacheKey> remove(Set<CacheKey<?>> commands);
 
   protected abstract void clearStore();
 
-  protected abstract Boolean containsKeyInStore(CacheKey cacheKey);
+  protected abstract boolean containsKeyInStore(CacheKey cacheKey);
 
   // End of abstract methods to be implemented by the concrete classes
 

diff --git a/src/main/java/redis/clients/jedis/csc/Cache.java b/src/main/java/redis/clients/jedis/csc/Cache.java
@@ -45,7 +45,7 @@ public interface Cache {
      * @param cacheKey The cache key of the entry in the cache
      * @return True if the entry could be deleted, false if the entry wasn't found.
      */
-    Boolean delete(CacheKey cacheKey);
+    boolean delete(CacheKey cacheKey);
 
     /**
      * Delete entries by cache key from the cache
@@ -82,14 +82,14 @@ public interface Cache {
      * @param cacheKey The key of the cache entry
      * @return True if the entry is cachable, false otherwise
      */
-    Boolean isCacheable(CacheKey cacheKey);
+    boolean isCacheable(CacheKey cacheKey);
 
     /**
      *
      * @param cacheKey The key of the cache entry
      * @return True if the cache already contains the key
      */
-    Boolean hasCacheKey(CacheKey cacheKey);
+    boolean hasCacheKey(CacheKey cacheKey);
 
     /**
      * @return The eviction policy that is used by the cache
@@ -99,10 +99,10 @@ public interface Cache {
     /**
      * @return The statistics of the cache
      */
-    public CacheStats getStats();
+    CacheStats getStats();
 
     /**
      * @return The statistics of the cache
      */
-    public CacheStats getAndResetStats();
+    CacheStats getAndResetStats();
 }
diff --git a/src/main/java/redis/clients/jedis/csc/CacheConnection.java b/src/main/java/redis/clients/jedis/csc/CacheConnection.java
@@ -1,8 +1,8 @@
 package redis.clients.jedis.csc;
 
-import java.lang.ref.WeakReference;
 import java.util.Objects;
 import java.util.concurrent.locks.ReentrantLock;
+
 import redis.clients.jedis.CommandObject;
 import redis.clients.jedis.Connection;
 import redis.clients.jedis.JedisClientConfig;
@@ -84,7 +84,7 @@ public <T> T executeCommand(final CommandObject<T> commandObject) {
     clientSideCache.getStats().miss();
     T value = super.executeCommand(commandObject);
     if (value != null) {
-      cacheEntry = new CacheEntry<T>(cacheKey, value, new WeakReference(this));
+      cacheEntry = new CacheEntry<>(cacheKey, value, this);
       clientSideCache.set(cacheKey, cacheEntry);
       // this line actually provides a deep copy of cached object instance 
       value = cacheEntry.getValue();
@@ -101,7 +101,7 @@ private void initializeClientSideCache() {
   }
 
   private CacheEntry validateEntry(CacheEntry cacheEntry) {
-    CacheConnection cacheOwner = (CacheConnection) cacheEntry.getConnection().get();
+    CacheConnection cacheOwner = cacheEntry.getConnection();
     if (cacheOwner == null || cacheOwner.isBroken() || !cacheOwner.isConnected()) {
       clientSideCache.delete(cacheEntry.getCacheKey());
       return null;

diff --git a/src/main/java/redis/clients/jedis/csc/CacheEntry.java b/src/main/java/redis/clients/jedis/csc/CacheEntry.java
@@ -7,20 +7,17 @@
 import java.io.ObjectOutputStream;
 import java.lang.ref.WeakReference;
 
-import redis.clients.jedis.Connection;
-import redis.clients.jedis.annots.Internal;
 import redis.clients.jedis.exceptions.JedisCacheException;
 
-@Internal
 public class CacheEntry<T> {
 
   private final CacheKey<T> cacheKey;
-  private final WeakReference<Connection> connection;
+  private final WeakReference<CacheConnection> connection;
   private final byte[] bytes;
 
-  public CacheEntry(CacheKey<T> cacheKey, T value, WeakReference<Connection> connection) {
+  public CacheEntry(CacheKey<T> cacheKey, T value, CacheConnection connection) {
     this.cacheKey = cacheKey;
-    this.connection = connection;
+    this.connection = new WeakReference<>(connection);
     this.bytes = toBytes(value);
   }
 
@@ -32,8 +29,8 @@ public T getValue() {
     return toObject(bytes);
   }
 
-  public WeakReference<Connection> getConnection() {
-    return connection;
+  public CacheConnection getConnection() {
+    return connection.get();
   }
 
   private static byte[] toBytes(Object object) {
@@ -52,9 +49,7 @@ private T toObject(byte[] data) {
     try (ByteArrayInputStream bais = new ByteArrayInputStream(data);
         ObjectInputStream ois = new ObjectInputStream(bais)) {
       return (T) ois.readObject();
-    } catch (IOException e) {
-      throw new JedisCacheException("Failed to deserialize object", e);
-    } catch (ClassNotFoundException e) {
+    } catch (IOException | ClassNotFoundException e) {
       throw new JedisCacheException("Failed to deserialize object", e);
     }
   }

diff --git a/src/main/java/redis/clients/jedis/csc/CacheKey.java b/src/main/java/redis/clients/jedis/csc/CacheKey.java
@@ -1,10 +1,11 @@
 package redis.clients.jedis.csc;
 
+import java.util.List;
 import java.util.Objects;
+
 import redis.clients.jedis.CommandObject;
-import redis.clients.jedis.annots.Internal;
+import redis.clients.jedis.commands.ProtocolCommand;
 
-@Internal
 public class CacheKey<T> {
 
   private final CommandObject<T> command;
@@ -26,11 +27,11 @@ public boolean equals(Object obj) {
     return Objects.equals(this.command, other.command);
   }
 
-  public Object[] getRedisKeys() {
+  public List<Object> getRedisKeys() {
     return command.getArguments().getKeys();
   }
 
-  public CommandObject<T> getCommand() {
-    return command;
+  public ProtocolCommand getRedisCommand() {
+    return command.getArguments().getCommand();
   }
 }
diff --git a/src/main/java/redis/clients/jedis/csc/Cacheable.java b/src/main/java/redis/clients/jedis/csc/Cacheable.java
@@ -0,0 +1,9 @@
+package redis.clients.jedis.csc;
+
+import java.util.List;
+import redis.clients.jedis.commands.ProtocolCommand;
+
+public interface Cacheable {
+
+  boolean isCacheable(ProtocolCommand command, List<Object> keys);
+}