[GitHub] [geode] kirklund commented on a change in pull request #6831: GEODE-9519: Implement Radish ZSCAN command

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

kirklund commented on a change in pull request #6831:
URL: https://github.com/apache/geode/pull/6831#discussion_r702186251



##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -568,6 +591,19 @@ private int getMaxElementsToReturn(AbstractSortedSetRangeOptions<?> rangeOptions
     return result;
   }
 
+  private void addIfMatching(Pattern matchPattern, List<byte[]> resultList, byte[] key,
+      double score) {
+    if (matchPattern != null) {
+      if (matchPattern.matcher(bytesToString(key)).matches()) {
+        resultList.add(key);
+        resultList.add(doubleToBytes(score));
+      }
+    } else {
+      resultList.add(key);
+      resultList.add(doubleToBytes(score));
+    }
+  }

Review comment:
       This change should be unit tested.

##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSortedSet.java
##########
@@ -377,6 +382,24 @@ long zrevrank(byte[] member) {
     return scoreSet.size() - scoreSet.indexOf(orderedSetEntry) - 1;
   }
 
+  ImmutablePair<Integer, List<byte[]>> zscan(Pattern matchPattern, int count, int cursor) {
+    // No need to allocate more space than it's possible to use given the size of the sorted set. We
+    // need to add 1 to zcard() to ensure that if count > members.size(), we return a cursor of 0
+    long maximumCapacity = 2L * Math.min(count, zcard() + 1);
+    if (maximumCapacity > Integer.MAX_VALUE) {
+      LogService.getLogger().info(

Review comment:
       `LogService.getLogger()` constructs a new instance of `FastLogger` every time it's called. You should always store it in a `static final` reference instead of inlining calls like this.
   ```
   private static final Logger logger = LogService.getLogger();
   ```

##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/key/AbstractScanExecutor.java
##########
@@ -15,17 +15,120 @@
  */
 package org.apache.geode.redis.internal.executor.key;
 
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_CURSOR;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NOT_INTEGER;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_SYNTAX;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_TYPE;
+import static org.apache.geode.redis.internal.netty.Coder.bytesToLong;
+import static org.apache.geode.redis.internal.netty.Coder.bytesToString;
+import static org.apache.geode.redis.internal.netty.Coder.equalsIgnoreCaseBytes;
+import static org.apache.geode.redis.internal.netty.Coder.narrowLongToInt;
+import static org.apache.geode.redis.internal.netty.StringBytesGlossary.bCOUNT;
+import static org.apache.geode.redis.internal.netty.StringBytesGlossary.bMATCH;
+
 import java.math.BigInteger;
+import java.util.List;
 import java.util.regex.Pattern;
+import java.util.regex.PatternSyntaxException;
+
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.logging.log4j.Logger;
 
+import org.apache.geode.logging.internal.log4j.api.LogService;
+import org.apache.geode.redis.internal.RedisException;
+import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.data.RedisDataType;
+import org.apache.geode.redis.internal.data.RedisDataTypeMismatchException;
+import org.apache.geode.redis.internal.data.RedisKey;
 import org.apache.geode.redis.internal.executor.AbstractExecutor;
 import org.apache.geode.redis.internal.executor.GlobPattern;
+import org.apache.geode.redis.internal.executor.RedisResponse;
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
 
 public abstract class AbstractScanExecutor extends AbstractExecutor {
-
+  private static final Logger logger = LogService.getLogger();
   protected final BigInteger UNSIGNED_LONG_CAPACITY = new BigInteger("18446744073709551615");
   protected final int DEFAULT_COUNT = 10;
 
+  @Override
+  public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {

Review comment:
       There are a lot of changes here that should be unit tested.




-- 
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: notifications-unsubscribe@geode.apache.org

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