[GitHub] [druid] asdf2014 commented on a change in pull request #11369: Fix an exception when using redis cluster as cache

[GitBox <gi...@apache.org>]

asdf2014 commented on a change in pull request #11369:
URL: https://github.com/apache/druid/pull/11369#discussion_r696605006



##########
File path: extensions-contrib/redis-cache/src/test/java/org/apache/druid/client/cache/RedisClusterCacheTest.java
##########
@@ -130,15 +132,24 @@ public void testCache()
     Assert.assertEquals(0x01020304, Ints.fromByteArray(cache.get(key1)));
     Assert.assertEquals(0x02030405, Ints.fromByteArray(cache.get(key2)));
     Assert.assertEquals(0x03040506, Ints.fromByteArray(cache.get(key3)));
+    Assert.assertEquals(0x03040506, Ints.fromByteArray(cache.get(key3)));
+    Assert.assertEquals(null, cache.get(notExist));

Review comment:
       ```suggestion
       Assert.assertNull(cache.get(notExist));
   ```

##########
File path: extensions-contrib/redis-cache/src/main/java/org/apache/druid/client/cache/AbstractRedisCache.java
##########
@@ -116,9 +105,8 @@ public void put(NamedKey key, byte[] value)
         errorCount.incrementAndGet();
       }
       log.warn(e, "Exception pulling items from cache");
+      return null;

Review comment:
       Since this method will return null now, we should add `@Nullable` annotation to the Cache api. FYI, https://github.com/apache/druid/blob/master/server/src/main/java/org/apache/druid/client/cache/Cache.java#L46

##########
File path: extensions-contrib/redis-cache/src/main/java/org/apache/druid/client/cache/RedisClusterCache.java
##########
@@ -46,10 +52,59 @@ protected void putToRedis(byte[] key, byte[] value, RedisCacheConfig.DurationCon
     cluster.setex(key, (int) expiration.getSeconds(), value);
   }
 
+  static class CachableKey
+  {
+    byte[] keyBytes;
+    NamedKey namedKey;
+
+    public CachableKey(NamedKey namedKey)
+    {
+      this.keyBytes = namedKey.toByteArray();
+      this.namedKey = namedKey;
+    }
+  }
+
+  /**
+   * Jedis does not work if the given keys are distributed among different redis nodes
+   * A simple workaround is to group keys by their slots and mget values for each slot.
+   * <p>
+   * In future, Jedis could be replaced by the Lettuce driver which supports mget operation on a redis cluster
+   */
   @Override
-  protected List<byte[]> mgetFromRedis(byte[]... keys)
+  protected Pair<Integer, Map<NamedKey, byte[]>> mgetFromRedis(Iterable<NamedKey> keys)
   {
-    return cluster.mget(keys);
+    int inputKeyCount = 0;
+
+    // group keys based on their slot

Review comment:
       nit: 
   ```suggestion
       // group keys based on their slots
   ```

##########
File path: extensions-contrib/redis-cache/src/main/java/org/apache/druid/client/cache/AbstractRedisCache.java
##########
@@ -172,7 +160,11 @@ public void doMonitor(ServiceEmitter emitter)
 
   protected abstract void putToRedis(byte[] key, byte[] value, RedisCacheConfig.DurationConfig expiration);
 
-  protected abstract List<byte[]> mgetFromRedis(byte[]... keys);
+  /**
+   * The lhs of the returned pair the count of input keys

Review comment:
       nit:
   
   ```suggestion
      * The lhs of the returned pair is the count of input keys
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org