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

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

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



##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -243,10 +243,9 @@ public int hstrlen(byte[] field) {
       throw new IllegalArgumentException("Requested array size exceeds VM limit");
     }
     List<byte[]> resultList = new ArrayList<>((int) maximumCapacity);
-    do {
-      cursor = hash.scan(cursor, 1,
-          (list, key, value) -> addIfMatching(matchPattern, list, key, value), resultList);
-    } while (cursor != 0 && resultList.size() < maximumCapacity);
+
+    cursor = hash.scan(cursor, count,
+        (list, key, value) -> addIfMatching(matchPattern, list, key, value), resultList);

Review comment:
       I think this is going to underpopulate the resultList if there is a matchPattern. The scan method is only going to call `addIfMatching` for `maximumCapacity` iterations, rather than waiting until resultList has reached `maximumCapacity`.

##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/key/AbstractScanExecutor.java
##########
@@ -15,25 +15,158 @@
  */
 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.annotations.VisibleForTesting;
+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 {
-
-  protected final BigInteger UNSIGNED_LONG_CAPACITY = new BigInteger("18446744073709551615");
+  private static final Logger logger = LogService.getLogger();
+  public static final BigInteger UNSIGNED_LONG_CAPACITY = new BigInteger("18446744073709551615");
   protected final int DEFAULT_COUNT = 10;
 
+  @Override
+  public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
+    List<byte[]> commandElems = command.getProcessedCommand();
+
+    int cursor;
+    try {
+      cursor = getIntCursor(bytesToString(commandElems.get(2)));
+    } catch (RedisException ex) {
+      return RedisResponse.error(ERROR_CURSOR);
+    }
+
+    RedisKey key = command.getKey();
+
+    // Because we're trying to preserve the same semantics of error conditions, with native redis,
+    // the ordering of input validation is reflected here. To that end the first check ends up
+    // being an existence check of the key. That causes a race since the data value needs to be
+    // accessed again when the actual command does its work. If the relevant bucket doesn't get
+    // locked throughout the call, the bucket may move producing inconsistent results.
+    return context.getRegionProvider().execute(key, () -> {
+      RedisData value = context.getRegionProvider().getRedisData(key);
+      if (value.isNull()) {
+        context.getRedisStats().incKeyspaceMisses();
+        return RedisResponse.emptyScan();
+      }
+
+      if (value.getType() != getDataType()) {
+        throw new RedisDataTypeMismatchException(ERROR_WRONG_TYPE);
+      }
+
+      command.getCommandType().checkDeferredParameters(command, context);
+      int count = DEFAULT_COUNT;
+      String globPattern = null;
+
+      for (int i = 3; i < commandElems.size(); i = i + 2) {
+        byte[] commandElemBytes = commandElems.get(i);
+        if (equalsIgnoreCaseBytes(commandElemBytes, bMATCH)) {
+          commandElemBytes = commandElems.get(i + 1);
+          globPattern = bytesToString(commandElemBytes);
+
+        } else if (equalsIgnoreCaseBytes(commandElemBytes, bCOUNT)) {
+          commandElemBytes = commandElems.get(i + 1);
+          try {
+            count = narrowLongToInt(bytesToLong(commandElemBytes));
+          } catch (NumberFormatException e) {
+            return RedisResponse.error(ERROR_NOT_INTEGER);
+          }
+
+          if (count < 1) {
+            return RedisResponse.error(ERROR_SYNTAX);
+          }
+
+        } else {
+          return RedisResponse.error(ERROR_SYNTAX);
+        }
+      }
+
+      Pattern matchPattern;
+      try {
+        matchPattern = convertGlobToRegex(globPattern);
+      } catch (PatternSyntaxException e) {
+        logger.warn(
+            "Could not compile the pattern: '{}' due to the following exception: '{}'. {} will return an empty list.",
+            globPattern, e.getMessage(), command.getCommandType().name());
+
+        return RedisResponse.emptyScan();
+      }
+
+      Pair<Integer, List<byte[]>> scanResult;
+      try {
+        scanResult = executeScan(context, key, matchPattern, count, cursor);
+      } catch (IllegalArgumentException ex) {
+        return RedisResponse.error(ERROR_NOT_INTEGER);
+      }
+
+      return RedisResponse.scan(scanResult.getLeft(), scanResult.getRight());
+    });
+  }
+
   /**
    * @param pattern A glob pattern.
    * @return A regex pattern to recognize the given glob pattern.
    */
-  protected Pattern convertGlobToRegex(String pattern) {
+  @VisibleForTesting
+  public Pattern convertGlobToRegex(String pattern) {
     if (pattern == null) {
       return null;
     }
     return GlobPattern.compile(pattern);
   }
+  // Redis allows values for CURSOR up to UNSIGNED_LONG_CAPACITY, but internally it only makes sense
+  // for us to use values up to Integer.MAX_VALUE, so safely narrow any cursor values to int here
+
+  @VisibleForTesting
+  public int getIntCursor(String cursorString) {

Review comment:
       How about we just use Integer.parseInt here (or Long.parseLong, if you prefer). Using BigInteger is a performance impact. Since the behavior is undetermined if the user passes as a cursor value other than 0 or a number that we previously returned, it seems like we could just treat numbers that are too large as 0 or integer max value or whatever. https://redis.io/commands/scan#calling-scan-with-a-corrupted-cursor




-- 
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