This is an automated email from the ASF dual-hosted git repository.
sabbey37 pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push:
new bcdf3ca GEODE-8582: Redis SCAN returns internal server error (#5603)
bcdf3ca is described below
commit bcdf3ca64d8aca83cdd48629d8a35be6352d2861
Author: Sarah <[email protected]>
AuthorDate: Thu Oct 8 09:35:05 2020 -0400
GEODE-8582: Redis SCAN returns internal server error (#5603)
Co-authored-by: Darrel Schneider <[email protected]>
---
.../ScanNativeRedisAcceptanceTest.java} | 5 +-
.../pubsub/PubSubNativeRedisAcceptanceTest.java | 2 +
.../executor/key/AbstractScanIntegrationTest.java | 112 +++++++++++++++++++++
.../executor/key/ScanIntegrationTest.java} | 11 +-
.../redis/internal/executor/key/ScanExecutor.java | 37 +++----
5 files changed, 142 insertions(+), 25 deletions(-)
diff --git
a/geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/pubsub/PubSubNativeRedisAcceptanceTest.java
b/geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/key/ScanNativeRedisAcceptanceTest.java
similarity index 87%
copy from
geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/pubsub/PubSubNativeRedisAcceptanceTest.java
copy to
geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/key/ScanNativeRedisAcceptanceTest.java
index ce7778f..237d691 100644
---
a/geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/pubsub/PubSubNativeRedisAcceptanceTest.java
+++
b/geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/key/ScanNativeRedisAcceptanceTest.java
@@ -13,13 +13,14 @@
* the License.
*/
-package org.apache.geode.redis.internal.executor.pubsub;
+package org.apache.geode.redis.internal.executor.key;
import org.junit.ClassRule;
import org.apache.geode.NativeRedisTestRule;
-public class PubSubNativeRedisAcceptanceTest extends
AbstractPubSubIntegrationTest {
+public class ScanNativeRedisAcceptanceTest extends AbstractScanIntegrationTest
{
+
@ClassRule
public static NativeRedisTestRule redis = new NativeRedisTestRule();
diff --git
a/geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/pubsub/PubSubNativeRedisAcceptanceTest.java
b/geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/pubsub/PubSubNativeRedisAcceptanceTest.java
index ce7778f..772ebb1 100644
---
a/geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/pubsub/PubSubNativeRedisAcceptanceTest.java
+++
b/geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/pubsub/PubSubNativeRedisAcceptanceTest.java
@@ -16,9 +16,11 @@
package org.apache.geode.redis.internal.executor.pubsub;
import org.junit.ClassRule;
+import org.junit.Ignore;
import org.apache.geode.NativeRedisTestRule;
+@Ignore("GEODE-8577")
public class PubSubNativeRedisAcceptanceTest extends
AbstractPubSubIntegrationTest {
@ClassRule
public static NativeRedisTestRule redis = new NativeRedisTestRule();
diff --git
a/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/key/AbstractScanIntegrationTest.java
b/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/key/AbstractScanIntegrationTest.java
new file mode 100644
index 0000000..7ab33ba
--- /dev/null
+++
b/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/key/AbstractScanIntegrationTest.java
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.executor.key;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.Jedis;
+import redis.clients.jedis.ScanParams;
+import redis.clients.jedis.ScanResult;
+
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.rules.RedisPortSupplier;
+
+public abstract class AbstractScanIntegrationTest implements RedisPortSupplier
{
+
+ private Jedis jedis;
+ private static final int REDIS_CLIENT_TIMEOUT =
+ Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+
+ @Before
+ public void setUp() {
+ jedis = new Jedis("localhost", getPort(), REDIS_CLIENT_TIMEOUT);
+ }
+
+ @After
+ public void tearDown() {
+ jedis.flushAll();
+ jedis.close();
+ }
+
+ @Test
+ public void givenOneKey_scanReturnsKey() {
+ jedis.set("a", "1");
+ ScanResult<String> result = jedis.scan("0");
+
+ assertThat(result.isCompleteIteration()).isTrue();
+ assertThat(result.getResult()).containsExactly("a");
+ }
+
+ @Test
+ public void givenEmptyDatabase_scanReturnsEmptyResult() {
+ ScanResult<String> result = jedis.scan("0");
+
+ assertThat(result.isCompleteIteration()).isTrue();
+ assertThat(result.getResult()).isEmpty();
+ }
+
+ @Test
+ public void givenMultipleKeys_scanReturnsAllKeys() {
+ jedis.set("a", "1");
+ jedis.sadd("b", "green", "orange");
+ jedis.hset("c", "potato", "sweet");
+ ScanResult<String> result = jedis.scan("0");
+
+ assertThat(result.isCompleteIteration()).isTrue();
+ assertThat(result.getResult()).containsExactlyInAnyOrder("a", "b", "c");
+ }
+
+ @Test
+ public void givenMultipleKeysWithCount_scanReturnsAllKeysWithoutDuplicates()
{
+ jedis.set("a", "1");
+ jedis.sadd("b", "green", "orange");
+ jedis.hset("c", "potato", "sweet");
+ ScanParams scanParams = new ScanParams();
+ scanParams.count(1);
+
+ ScanResult<String> result = jedis.scan("0", scanParams);
+ List<String> allKeysFromScan = new ArrayList<>();
+ allKeysFromScan.addAll(result.getResult());
+
+ while (!result.isCompleteIteration()) {
+ result = jedis.scan(result.getCursor(), scanParams);
+ allKeysFromScan.addAll(result.getResult());
+ }
+
+ assertThat(result.isCompleteIteration()).isTrue();
+ assertThat(allKeysFromScan).containsExactlyInAnyOrder("a", "b", "c");
+ }
+
+ @Test
+ public void
givenMultipleKeysWithMatch_scanReturnsAllMatchingKeysWithoutDuplicates() {
+ jedis.set("a", "1");
+ jedis.sadd("b", "green", "orange");
+ jedis.hset("c", "potato", "sweet");
+ ScanParams scanParams = new ScanParams();
+ scanParams.match("a*");
+
+ ScanResult<String> result = jedis.scan("0", scanParams);
+
+ assertThat(result.isCompleteIteration()).isTrue();
+ assertThat(result.getResult()).containsExactly("a");
+ }
+}
diff --git
a/geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/pubsub/PubSubNativeRedisAcceptanceTest.java
b/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/key/ScanIntegrationTest.java
similarity index 75%
copy from
geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/pubsub/PubSubNativeRedisAcceptanceTest.java
copy to
geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/key/ScanIntegrationTest.java
index ce7778f..bfc9e54 100644
---
a/geode-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/pubsub/PubSubNativeRedisAcceptanceTest.java
+++
b/geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/key/ScanIntegrationTest.java
@@ -13,19 +13,20 @@
* the License.
*/
-package org.apache.geode.redis.internal.executor.pubsub;
+package org.apache.geode.redis.internal.executor.key;
import org.junit.ClassRule;
-import org.apache.geode.NativeRedisTestRule;
+import org.apache.geode.redis.GeodeRedisServerRule;
+
+public class ScanIntegrationTest extends AbstractScanIntegrationTest {
-public class PubSubNativeRedisAcceptanceTest extends
AbstractPubSubIntegrationTest {
@ClassRule
- public static NativeRedisTestRule redis = new NativeRedisTestRule();
+ public static GeodeRedisServerRule server = new GeodeRedisServerRule();
@Override
public int getPort() {
- return redis.getPort();
+ return server.getPort();
}
}
diff --git
a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/key/ScanExecutor.java
b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/key/ScanExecutor.java
index aaf68be..ee2715e 100755
---
a/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/key/ScanExecutor.java
+++
b/geode-redis/src/main/java/org/apache/geode/redis/internal/executor/key/ScanExecutor.java
@@ -24,6 +24,7 @@ import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;
import org.apache.geode.redis.internal.RedisConstants.ArityDef;
+import org.apache.geode.redis.internal.data.ByteArrayWrapper;
import org.apache.geode.redis.internal.executor.RedisResponse;
import org.apache.geode.redis.internal.netty.Coder;
import org.apache.geode.redis.internal.netty.Command;
@@ -32,8 +33,7 @@ import
org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
public class ScanExecutor extends AbstractScanExecutor {
@Override
- public RedisResponse executeCommand(Command command,
- ExecutionHandlerContext context) {
+ public RedisResponse executeCommand(Command command, ExecutionHandlerContext
context) {
List<byte[]> commandElems = command.getProcessedCommand();
if (commandElems.size() < 2) {
@@ -93,36 +93,37 @@ public class ScanExecutor extends AbstractScanExecutor {
return RedisResponse.error(ERROR_ILLEGAL_GLOB);
}
- @SuppressWarnings("unchecked")
- List<String> returnList = (List<String>)
getIteration(getDataRegion(context).keySet(),
- matchPattern, count, cursor);
+ List<String> returnList = getIteration(getDataRegion(context).keySet(),
matchPattern, count,
+ cursor);
return RedisResponse.scan(returnList);
}
- @SuppressWarnings("unchecked")
- private List<?> getIteration(Collection<?> list, Pattern matchPattern, int
count, int cursor) {
- List<String> returnList = new ArrayList<String>();
+ private List<String> getIteration(Collection<ByteArrayWrapper> list, Pattern
matchPattern,
+ int count, int cursor) {
+ List<String> returnList = new ArrayList<>();
int size = list.size();
int beforeCursor = 0;
int numElements = 0;
int i = -1;
- for (String key : (Collection<String>) list) {
+ for (ByteArrayWrapper key : list) {
i++;
if (beforeCursor < cursor) {
beforeCursor++;
continue;
- } else if (numElements < count) {
- if (matchPattern != null) {
- if (matchPattern.matcher(key).matches()) {
- returnList.add(key);
- numElements++;
- }
- } else {
- returnList.add(key);
+ }
+
+ String keyString = key.toString();
+ if (matchPattern != null) {
+ if (matchPattern.matcher(keyString).matches()) {
+ returnList.add(keyString);
numElements++;
}
} else {
+ returnList.add(keyString);
+ numElements++;
+ }
+ if (numElements == count) {
break;
}
}
@@ -130,7 +131,7 @@ public class ScanExecutor extends AbstractScanExecutor {
if (i >= size - 1) {
returnList.add(0, String.valueOf(0));
} else {
- returnList.add(0, String.valueOf(i));
+ returnList.add(0, String.valueOf(i + 1));
}
return returnList;
}
