You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by do...@apache.org on 2022/02/02 23:23:47 UTC
[geode] branch support/1.15 updated: GEODE-9833: SPOP Command Support (#7319)
This is an automated email from the ASF dual-hosted git repository.
donalevans pushed a commit to branch support/1.15
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/support/1.15 by this push:
new 64bf652 GEODE-9833: SPOP Command Support (#7319)
64bf652 is described below
commit 64bf652dd057ff48bb0ce4405a20832f7e4bda48
Author: Kris10 <ko...@vmware.com>
AuthorDate: Wed Feb 2 14:51:19 2022 -0800
GEODE-9833: SPOP Command Support (#7319)
- Modified implementation of retrieving a random element from SizeableObjectOpenCustomHashSetWithCursor backing array
(cherry picked from commit afa17f65ec57052b0ee110326385568df784ba1a)
---
geode-for-redis/README.md | 1 +
.../server/AbstractHitsMissesIntegrationTest.java | 15 +-
.../executor/set/AbstractSPopIntegrationTest.java | 256 ++++++++-------------
.../redis/internal/commands/RedisCommandType.java | 7 +-
.../commands/executor/set/SPopExecutor.java | 53 ++---
.../commands/executor/set/SRandMemberExecutor.java | 19 +-
.../commands/executor/set/SetRandomExecutor.java | 23 +-
.../geode/redis/internal/data/NullRedisSet.java | 3 +-
.../apache/geode/redis/internal/data/RedisSet.java | 131 +++++++----
.../SizeableObjectOpenCustomHashSetWithCursor.java | 23 +-
10 files changed, 252 insertions(+), 279 deletions(-)
diff --git a/geode-for-redis/README.md b/geode-for-redis/README.md
index 183d148..f0222fd 100644
--- a/geode-for-redis/README.md
+++ b/geode-for-redis/README.md
@@ -212,6 +212,7 @@ Geode for Redis implements a subset of the full Redis command set.
- SLOWLOG <sup>3</sup>
- SMEMBERS
- SMOVE
+- SPOP
- SRANDMEMBER
- SREM
- STRLEN
diff --git a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/server/AbstractHitsMissesIntegrationTest.java b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/server/AbstractHitsMissesIntegrationTest.java
index 4b07ff9..a123987 100644
--- a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/server/AbstractHitsMissesIntegrationTest.java
+++ b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/server/AbstractHitsMissesIntegrationTest.java
@@ -415,6 +415,13 @@ public abstract class AbstractHitsMissesIntegrationTest implements RedisIntegrat
runMultiKeyCommandAndAssertNoStatUpdates(SET_KEY, (k1, k2) -> jedis.smove(k1, k2, "member"));
}
+ // FYI - In Redis 5.x SPOP produces inconsistent results depending on whether a count was given
+ // or not. In Redis 6.x SPOP does not update any stats.
+ @Test
+ public void testSpop() {
+ runCommandAndAssertNoStatUpdates(SET_KEY, k -> jedis.spop(k));
+ }
+
@Test
public void testSrandmember() {
runCommandAndAssertHitsAndMisses(SET_KEY, k -> jedis.srandmember(k));
@@ -563,14 +570,6 @@ public abstract class AbstractHitsMissesIntegrationTest implements RedisIntegrat
runCommandAndAssertNoStatUpdates(STRING_INT_KEY, k -> jedis.setbit(k, 0L, "1"));
}
- /************* Set related commands *************/
- // FYI - In Redis 5.x SPOP produces inconsistent results depending on whether a count was given
- // or not. In Redis 6.x SPOP does not update any stats.
- @Test
- public void testSpop() {
- runCommandAndAssertNoStatUpdates(SET_KEY, k -> jedis.spop(k));
- }
-
/************* Helper Methods *************/
private void runCommandAndAssertHitsAndMisses(String key, Consumer<String> command) {
Map<String, String> info = RedisTestHelper.getInfo(jedis);
diff --git a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSPopIntegrationTest.java b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSPopIntegrationTest.java
index c85e339..20ad60d 100755
--- a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSPopIntegrationTest.java
+++ b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSPopIntegrationTest.java
@@ -14,16 +14,20 @@
*/
package org.apache.geode.redis.internal.commands.executor.set;
+import static org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertAtLeastNArgs;
import static org.apache.geode.redis.internal.RedisConstants.ERROR_SYNTAX;
import static org.apache.geode.redis.internal.RedisConstants.ERROR_VALUE_MUST_BE_POSITIVE;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_TYPE;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
+import static org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
-import java.net.Socket;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
import org.junit.After;
import org.junit.Before;
@@ -32,17 +36,19 @@ import redis.clients.jedis.HostAndPort;
import redis.clients.jedis.JedisCluster;
import redis.clients.jedis.Protocol;
+import org.apache.geode.redis.ConcurrentLoopingThreads;
import org.apache.geode.redis.RedisIntegrationTest;
-import org.apache.geode.test.awaitility.GeodeAwaitility;
public abstract class AbstractSPopIntegrationTest implements RedisIntegrationTest {
private JedisCluster jedis;
- private static final int REDIS_CLIENT_TIMEOUT =
- Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+ private static final String NON_EXISTENT_SET_KEY = "{tag1}nonExistentSet";
+ private static final String SET_KEY = "{tag1}setKey";
+ private static final String[] SET_MEMBERS =
+ {"one", "two", "three", "four", "five", "six", "seven", "eight"};
@Before
public void setUp() {
- jedis = new JedisCluster(new HostAndPort("localhost", getPort()), REDIS_CLIENT_TIMEOUT);
+ jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT);
}
@After
@@ -52,213 +58,141 @@ public abstract class AbstractSPopIntegrationTest implements RedisIntegrationTes
}
@Test
- public void givenKeyNotProvided_returnsWrongNumberOfArgumentsError() {
- assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SPOP))
- .hasMessageContaining("ERR wrong number of arguments for 'spop' command");
+ public void spopTooFewArgs_returnsError() {
+ assertAtLeastNArgs(jedis, Protocol.Command.SPOP, 1);
}
@Test
- public void givenMoreThanThreeArguments_returnsSyntaxError() {
+ public void spopTooManyArgs_returnsError() {
assertThatThrownBy(
- () -> jedis.sendCommand("key", Protocol.Command.SPOP, "key", "NaN", "extraArg"))
+ () -> jedis.sendCommand(SET_KEY, Protocol.Command.SPOP, SET_KEY, "5", "5"))
.hasMessageContaining(ERROR_SYNTAX);
}
@Test
- public void givenCountIsNotAnInteger_returnsNotIntegerError() {
- assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SPOP, "key", "NaN"))
+ public void spop_withNonNumericCount_returnsError() {
+ assertThatThrownBy(() -> jedis.sendCommand(SET_KEY, Protocol.Command.SPOP, SET_KEY, "b"))
.hasMessageContaining(ERROR_VALUE_MUST_BE_POSITIVE);
}
@Test
- public void testSPop() {
- int ENTRIES = 10;
-
- List<String> masterSet = new ArrayList<>();
- for (int i = 0; i < ENTRIES; i++) {
- masterSet.add("master-" + i);
- }
-
- jedis.sadd("master", masterSet.toArray(new String[] {}));
- String poppy = jedis.spop("master");
-
- masterSet.remove(poppy);
- assertThat(jedis.smembers("master").toArray()).containsExactlyInAnyOrder(masterSet.toArray());
-
- assertThat(jedis.spop("spopnonexistent")).isNull();
+ public void spop_withNegativeCount_returnsError() {
+ assertThatThrownBy(() -> jedis.spop(SET_KEY, -1))
+ .hasMessageContaining(ERROR_VALUE_MUST_BE_POSITIVE);
}
@Test
- public void testSPopAll() {
- int ENTRIES = 10;
-
- List<String> masterSet = new ArrayList<>();
- for (int i = 0; i < ENTRIES; i++) {
- masterSet.add("master-" + i);
- }
-
- jedis.sadd("master", masterSet.toArray(new String[] {}));
- Set<String> popped = jedis.spop("master", ENTRIES);
-
- assertThat(jedis.smembers("master").toArray()).isEmpty();
- assertThat(popped.toArray()).containsExactlyInAnyOrder(masterSet.toArray());
+ public void spop_withoutCount_withNonExistentSet_returnsNull_setNotCreated() {
+ assertThat(jedis.spop(NON_EXISTENT_SET_KEY)).isNull();
+ assertThat(jedis.exists(NON_EXISTENT_SET_KEY)).isFalse();
}
@Test
- public void testSPopAllPlusOne() {
- int ENTRIES = 10;
-
- List<String> masterSet = new ArrayList<>();
- for (int i = 0; i < ENTRIES; i++) {
- masterSet.add("master-" + i);
- }
+ public void spop_withoutCount_withExistentSet_returnsOneMember_removesReturnedMemberFromSet() {
+ jedis.sadd(SET_KEY, SET_MEMBERS);
- jedis.sadd("master", masterSet.toArray(new String[] {}));
- Set<String> popped = jedis.spop("master", ENTRIES + 1);
+ List<String> setMembersList = new ArrayList<>(Arrays.asList(SET_MEMBERS));
+ String result = jedis.spop(SET_KEY);
+ assertThat(result).isIn(setMembersList);
- assertThat(jedis.smembers("master").toArray()).isEmpty();
- assertThat(popped.toArray()).containsExactlyInAnyOrder(masterSet.toArray());
+ setMembersList.remove(result);
+ assertThat(jedis.smembers(SET_KEY)).containsExactlyInAnyOrderElementsOf(setMembersList);
}
@Test
- public void testSPopAllMinusOne() {
- int ENTRIES = 10;
-
- List<String> masterSet = new ArrayList<>();
- for (int i = 0; i < ENTRIES; i++) {
- masterSet.add("master-" + i);
- }
-
- jedis.sadd("master", masterSet.toArray(new String[] {}));
- Set<String> popped = jedis.spop("master", ENTRIES - 1);
-
- assertThat(jedis.smembers("master").toArray()).hasSize(1);
- assertThat(popped).hasSize(ENTRIES - 1);
- assertThat(masterSet).containsAll(popped);
+ public void spop_withCountAsZero_withExistentSet_returnsEmptyList_setNotModified() {
+ jedis.sadd(SET_KEY, SET_MEMBERS);
+ assertThat(jedis.spop(SET_KEY, 0)).isEmpty();
+ assertThat(jedis.smembers(SET_KEY)).containsExactlyInAnyOrder(SET_MEMBERS);
}
@Test
- public void testManySPops() {
- int ENTRIES = 100;
-
- List<String> masterSet = new ArrayList<>();
- for (int i = 0; i < ENTRIES; i++) {
- masterSet.add("master-" + i);
- }
-
- jedis.sadd("master", masterSet.toArray(new String[] {}));
-
- List<String> popped = new ArrayList<>();
- for (int i = 0; i < ENTRIES; i++) {
- popped.add(jedis.spop("master"));
- }
-
- assertThat(jedis.smembers("master")).isEmpty();
- assertThat(popped.toArray()).containsExactlyInAnyOrder(masterSet.toArray());
-
- assertThat(jedis.spop("master")).isNull();
+ public void spop_withCount_withNonExistentSet_returnsEmptyList_setNotCreated() {
+ assertThat(jedis.spop(NON_EXISTENT_SET_KEY, 1)).isEmpty();
+ assertThat(jedis.exists(NON_EXISTENT_SET_KEY)).isFalse();
}
@Test
- public void testConcurrentSPops() throws InterruptedException {
- int ENTRIES = 1000;
-
- List<String> masterSet = new ArrayList<>();
- for (int i = 0; i < ENTRIES; i++) {
- masterSet.add("master-" + i);
- }
+ public void spop_withSmallCount_withExistentSet_returnsCorrectNumberOfMembers_removesReturnedMembersFromSet() {
+ jedis.sadd(SET_KEY, SET_MEMBERS);
+ int count = 2;
- jedis.sadd("master", masterSet.toArray(new String[] {}));
+ List<String> setMembersList = new ArrayList<>(Arrays.asList(SET_MEMBERS));
+ Set<String> result = jedis.spop(SET_KEY, count);
+ assertThat(result.size()).isEqualTo(count);
+ assertThat(result).isSubsetOf(setMembersList);
- List<String> popped1 = new ArrayList<>();
- Runnable runnable1 = () -> {
- for (int i = 0; i < ENTRIES / 2; i++) {
- popped1.add(jedis.spop("master"));
- }
- };
-
- List<String> popped2 = new ArrayList<>();
- Runnable runnable2 = () -> {
- for (int i = 0; i < ENTRIES / 2; i++) {
- popped2.add(jedis.spop("master"));
- }
- };
-
- Thread thread1 = new Thread(runnable1);
- Thread thread2 = new Thread(runnable2);
-
- thread1.start();
- thread2.start();
- thread1.join();
- thread2.join();
-
- assertThat(jedis.smembers("master")).isEmpty();
-
- popped1.addAll(popped2);
- assertThat(popped1.toArray()).containsExactlyInAnyOrder(masterSet.toArray());
+ setMembersList.removeAll(result);
+ assertThat(jedis.smembers(SET_KEY)).containsExactlyInAnyOrderElementsOf(setMembersList);
}
@Test
- public void testSPopWithOutCount_shouldReturnNil_givenEmptySet() {
- Object result = jedis.sendCommand("noneSuch", Protocol.Command.SPOP, "noneSuch");
+ public void spop_withLargeCount_withExistentSet_returnsCorrectNumberOfMembers_removesReturnedMembersFromSet() {
+ jedis.sadd(SET_KEY, SET_MEMBERS);
+ int count = 6;
- assertThat(result).isNull();
+ List<String> setMembersList = new ArrayList<>(Arrays.asList(SET_MEMBERS));
+ Set<String> result = jedis.spop(SET_KEY, count);
+ assertThat(result.size()).isEqualTo(count);
+ assertThat(result).isSubsetOf(setMembersList);
+
+ setMembersList.removeAll(result);
+ assertThat(jedis.smembers(SET_KEY)).containsExactlyInAnyOrderElementsOf(setMembersList);
}
@Test
- public void testSPopWithCount_shouldReturnEmptyList_givenEmptySet() {
- Set<String> result = jedis.spop("noneSuch", 2);
-
- assertThat(result).isEmpty();
+ public void spop_withCountAsSetSize_withExistentSet_returnsAllMembers_setKeyIsDeleted() {
+ jedis.sadd(SET_KEY, SET_MEMBERS);
+ assertThat(jedis.spop(SET_KEY, SET_MEMBERS.length)).containsExactlyInAnyOrder(SET_MEMBERS);
+ assertThat(jedis.exists(SET_KEY)).isFalse();
}
@Test
- public void testSPopWithCountOfOne_shouldReturnList() {
- jedis.sadd("set", "one");
-
- Object actual = jedis.sendCommand("set", Protocol.Command.SPOP, "set", "1");
-
- assertThat(actual).isInstanceOf(List.class);
+ public void spop_withCountGreaterThanSetSize_withExistentSet_returnsAllMembers_setKeyIsDeleted() {
+ jedis.sadd(SET_KEY, SET_MEMBERS);
+ assertThat(jedis.spop(SET_KEY, SET_MEMBERS.length * 2)).containsExactlyInAnyOrder(SET_MEMBERS);
+ assertThat(jedis.exists(SET_KEY)).isFalse();
}
@Test
- public void testSPopWithoutCount_shouldNotReturnList() {
- jedis.sadd("set", "one");
+ public void spop_withoutCount_withWrongKeyType_returnsWrongTypeError() {
+ String key = "ding";
+ jedis.set(key, "dong");
+ assertThatThrownBy(() -> jedis.spop(key)).hasMessageContaining(ERROR_WRONG_TYPE);
+ }
- Object actual = jedis.sendCommand("set", Protocol.Command.SPOP, "set");
- assertThat(actual).isNotInstanceOf(List.class);
+ @Test
+ public void spop_withCountAsZero_withWrongKeyType_returnsWrongTypeError() {
+ String key = "ding";
+ jedis.set(key, "dong");
+ assertThatThrownBy(() -> jedis.spop(key, 0)).hasMessageContaining(ERROR_WRONG_TYPE);
}
@Test
- public void testSPopWithCountZero_shouldReturnEmptyList() {
- jedis.sadd("set", "one");
-
- Set<String> result = jedis.spop("set", 0);
-
- assertThat(result).isEmpty();
+ public void spop_withCountAsNegative_withWrongKeyType_returnsWrongTypeError() {
+ String key = "ding";
+ jedis.set(key, "dong");
+ assertThatThrownBy(() -> jedis.spop(key, -1))
+ .hasMessageContaining(ERROR_VALUE_MUST_BE_POSITIVE);
}
@Test
- public void testSPopWithoutArg_shouldReturnBulkString() throws Exception {
- jedis.sadd("set", "one");
-
- try (Socket redisSocket = new Socket("localhost", getPort())) {
- byte[] rawBytes = new byte[] {
- '*', '2', 0x0d, 0x0a,
- '$', '4', 0x0d, 0x0a,
- 'S', 'P', 'O', 'P', 0x0d, 0x0a,
- '$', '3', 0x0d, 0x0a,
- 's', 'e', 't', 0x0d, 0x0a,
- };
-
- redisSocket.getOutputStream().write(rawBytes);
-
- byte[] inputBuffer = new byte[1024];
- int n = redisSocket.getInputStream().read(inputBuffer);
- String result = new String(Arrays.copyOfRange(inputBuffer, 0, n));
-
- assertThat(result).isEqualTo("$3\r\none\r\n");
- }
+ public void ensureSetConsistency_whenRunningConcurrently() {
+ final AtomicReference<String> spopResultReference = new AtomicReference<>();
+ new ConcurrentLoopingThreads(1000,
+ i -> jedis.sadd(SET_KEY, SET_MEMBERS),
+ i -> spopResultReference.set(jedis.spop(SET_KEY)))
+ .runWithAction(() -> {
+ assertThat(spopResultReference).satisfiesAnyOf(
+ spopResult -> assertThat(spopResult.get()).isNull(),
+ spopResult -> assertThat(spopResult.get()).isIn(Arrays.asList(SET_MEMBERS)));
+ assertThat(SET_KEY).satisfiesAnyOf(
+ key -> assertThat(jedis.exists(key)).isFalse(),
+ key -> assertThat(jedis.smembers(key))
+ .doesNotContain(spopResultReference.get()).isSubsetOf(SET_MEMBERS)
+ .doesNotHaveDuplicates());
+ jedis.srem(SET_KEY, SET_MEMBERS);
+ });
}
}
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/RedisCommandType.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/RedisCommandType.java
index 856ea9a..551bc8d 100755
--- a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/RedisCommandType.java
+++ b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/RedisCommandType.java
@@ -252,6 +252,8 @@ public enum RedisCommandType {
SMEMBERS(new SMembersExecutor(), SUPPORTED,
new Parameter().exact(2).flags(READONLY, SORT_FOR_SCRIPT)),
SMOVE(new SMoveExecutor(), SUPPORTED, new Parameter().exact(4).lastKey(2).flags(WRITE, FAST)),
+ SPOP(new SPopExecutor(), SUPPORTED,
+ new Parameter().min(2).max(3, ERROR_SYNTAX).flags(WRITE, RANDOM, FAST)),
SRANDMEMBER(new SRandMemberExecutor(), SUPPORTED,
new Parameter().min(2).max(3, ERROR_SYNTAX).flags(READONLY, RANDOM)),
SREM(new SRemExecutor(), SUPPORTED, new Parameter().min(3).flags(WRITE, FAST)),
@@ -355,11 +357,6 @@ public enum RedisCommandType {
GETBIT(new GetBitExecutor(), UNSUPPORTED, new Parameter().exact(3).flags(READONLY, FAST)),
SETBIT(new SetBitExecutor(), UNSUPPORTED, new Parameter().exact(4).flags(WRITE, DENYOOM)),
- /**************** Sets *****************/
-
- SPOP(new SPopExecutor(), UNSUPPORTED,
- new Parameter().min(2).max(3, ERROR_SYNTAX).flags(WRITE, RANDOM, FAST)),
-
/*************** Server ****************/
DBSIZE(new DBSizeExecutor(), UNSUPPORTED, new Parameter().exact(1).firstKey(0).flags(READONLY,
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SPopExecutor.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SPopExecutor.java
index cf3ac82..a6cfd05 100755
--- a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SPopExecutor.java
+++ b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SPopExecutor.java
@@ -16,52 +16,35 @@ package org.apache.geode.redis.internal.commands.executor.set;
import static org.apache.geode.redis.internal.RedisConstants.ERROR_VALUE_MUST_BE_POSITIVE;
-import static org.apache.geode.redis.internal.netty.Coder.bytesToLong;
-import static org.apache.geode.redis.internal.netty.Coder.narrowLongToInt;
+import static org.apache.geode.redis.internal.data.RedisDataType.REDIS_SET;
-import java.util.Collection;
+import java.util.Collections;
import java.util.List;
-import org.apache.geode.cache.Region;
-import org.apache.geode.redis.internal.commands.Command;
-import org.apache.geode.redis.internal.commands.executor.CommandExecutor;
-import org.apache.geode.redis.internal.commands.executor.RedisResponse;
-import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.RedisException;
import org.apache.geode.redis.internal.data.RedisKey;
-import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+import org.apache.geode.redis.internal.data.RedisSet;
+import org.apache.geode.redis.internal.services.RegionProvider;
-public class SPopExecutor implements CommandExecutor {
+public class SPopExecutor extends SetRandomExecutor {
@Override
- public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
- List<byte[]> commandElems = command.getProcessedCommand();
- boolean isCountPassed = false;
- int popCount;
-
- if (commandElems.size() == 3) {
- isCountPassed = true;
- try {
- popCount = narrowLongToInt(bytesToLong(commandElems.get(2)));
- } catch (NumberFormatException nex) {
- return RedisResponse.error(ERROR_VALUE_MUST_BE_POSITIVE);
- }
- } else {
- popCount = 1;
+ protected List<byte[]> performCommand(int count, RegionProvider regionProvider, RedisKey key) {
+ if (count < 0) {
+ throw new RedisException(getError());
}
- Region<RedisKey, RedisData> region = context.getRegion();
- RedisKey key = command.getKey();
- Collection<byte[]> popped = context.setLockedExecute(key, false,
- set -> set.spop(region, key, popCount));
-
- if (popped.isEmpty() && !isCountPassed) {
- return RedisResponse.nil();
+ RedisSet set =
+ regionProvider.getTypedRedisData(REDIS_SET, key, false);
+ if (count == 0 || set.isNull()) {
+ return Collections.emptyList();
}
- if (!isCountPassed) {
- return RedisResponse.bulkString(popped.iterator().next());
- }
+ return set.spop(count, regionProvider.getDataRegion(), key);
+ }
- return RedisResponse.array(popped, true);
+ @Override
+ protected String getError() {
+ return ERROR_VALUE_MUST_BE_POSITIVE;
}
}
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SRandMemberExecutor.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SRandMemberExecutor.java
index a60d1c5..4a5677c 100755
--- a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SRandMemberExecutor.java
+++ b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SRandMemberExecutor.java
@@ -14,13 +14,30 @@
*/
package org.apache.geode.redis.internal.commands.executor.set;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NOT_INTEGER;
+import static org.apache.geode.redis.internal.data.RedisDataType.REDIS_SET;
+
+import java.util.Collections;
import java.util.List;
+import org.apache.geode.redis.internal.data.RedisKey;
import org.apache.geode.redis.internal.data.RedisSet;
+import org.apache.geode.redis.internal.services.RegionProvider;
public class SRandMemberExecutor extends SetRandomExecutor {
@Override
- protected List<byte[]> performCommand(RedisSet set, int count) {
+ protected List<byte[]> performCommand(int count, RegionProvider regionProvider, RedisKey key) {
+ RedisSet set =
+ regionProvider.getTypedRedisData(REDIS_SET, key, true);
+ if (count == 0 || set.isNull()) {
+ return Collections.emptyList();
+ }
+
return set.srandmember(count);
}
+
+ @Override
+ protected String getError() {
+ return ERROR_NOT_INTEGER;
+ }
}
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetRandomExecutor.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetRandomExecutor.java
index 56e4254..ca5f1b5 100644
--- a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetRandomExecutor.java
+++ b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetRandomExecutor.java
@@ -14,20 +14,15 @@
*/
package org.apache.geode.redis.internal.commands.executor.set;
-import static org.apache.geode.redis.internal.RedisConstants.ERROR_NOT_INTEGER;
-import static org.apache.geode.redis.internal.data.NullRedisDataStructures.NULL_REDIS_SET;
-import static org.apache.geode.redis.internal.data.RedisDataType.REDIS_SET;
import static org.apache.geode.redis.internal.netty.Coder.bytesToLong;
import static org.apache.geode.redis.internal.netty.Coder.narrowLongToInt;
-import java.util.Collections;
import java.util.List;
import org.apache.geode.redis.internal.commands.Command;
import org.apache.geode.redis.internal.commands.executor.CommandExecutor;
import org.apache.geode.redis.internal.commands.executor.RedisResponse;
import org.apache.geode.redis.internal.data.RedisKey;
-import org.apache.geode.redis.internal.data.RedisSet;
import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
import org.apache.geode.redis.internal.services.RegionProvider;
@@ -43,14 +38,15 @@ public abstract class SetRandomExecutor implements CommandExecutor {
try {
count = narrowLongToInt(bytesToLong(commandElems.get(2)));
} catch (NumberFormatException e) {
- return RedisResponse.error(ERROR_NOT_INTEGER);
+ return RedisResponse.error(getError());
}
} else {
count = 1;
}
List<byte[]> results =
- context.lockedExecute(key, () -> getResult(count, context.getRegionProvider(), key));
+ context.lockedExecute(key, () -> performCommand(count, context.getRegionProvider(), key));
+
if (hasCount) {
return RedisResponse.array(results, true);
} else {
@@ -62,15 +58,8 @@ public abstract class SetRandomExecutor implements CommandExecutor {
}
}
- private List<byte[]> getResult(int count, RegionProvider regionProvider, RedisKey key) {
- RedisSet set =
- regionProvider.getTypedRedisData(REDIS_SET, key, true);
- if (count == 0 || set == NULL_REDIS_SET) {
- return Collections.emptyList();
- }
-
- return performCommand(set, count);
- }
+ protected abstract List<byte[]> performCommand(int count, RegionProvider regionProvider,
+ RedisKey key);
- protected abstract List<byte[]> performCommand(RedisSet set, int count);
+ protected abstract String getError();
}
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisSet.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisSet.java
index 13dbed8..bbb2026 100644
--- a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisSet.java
+++ b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisSet.java
@@ -18,7 +18,6 @@ package org.apache.geode.redis.internal.data;
import static java.util.Collections.emptyList;
-import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
@@ -38,7 +37,7 @@ class NullRedisSet extends RedisSet {
}
@Override
- public Collection<byte[]> spop(Region<RedisKey, RedisData> region, RedisKey key, int popCount) {
+ public List<byte[]> spop(int popCount, Region<RedisKey, RedisData> region, RedisKey key) {
return emptyList();
}
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
index 23dfd0d..55c2f1b 100644
--- a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
+++ b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
@@ -16,7 +16,6 @@
package org.apache.geode.redis.internal.data;
-import static java.util.Collections.emptyList;
import static org.apache.geode.internal.JvmSizeUtils.memoryOverhead;
import static org.apache.geode.redis.internal.data.NullRedisDataStructures.NULL_REDIS_SET;
import static org.apache.geode.redis.internal.data.RedisDataType.REDIS_SET;
@@ -224,45 +223,91 @@ public class RedisSet extends AbstractRedisData {
}
}
- public Collection<byte[]> spop(Region<RedisKey, RedisData> region, RedisKey key, int popCount) {
- int originalSize = scard();
- if (originalSize == 0) {
- return emptyList();
+ public List<byte[]> spop(int count, Region<RedisKey, RedisData> region, RedisKey key) {
+ final int popMethodRatio = 5; // The ratio is based off command documentation
+ List<byte[]> result = new ArrayList<>(Math.min(count, members.size()));
+ if (count * popMethodRatio < members.size()) {
+ /*
+ * Count is small enough to add random elements to result.
+ * Random indexes are generated to get members that are stored in the backing array of the
+ * MemberSet. The members are then removed from the set, allowing for a unique random
+ * number to be generated every time.
+ */
+ spopWithSmallCount(count, result);
+ } else {
+ /*
+ * The count is close to the number of members in the set.
+ * Since members are being removed from the set, there is a possibility of rehashing. With a
+ * large count, there is a chance that it can rehash multiple times, copying
+ * the set to the result and removing members from that help limit the amount of
+ * rehashes that need to be preformed.
+ */
+ spopWithLargeCount(count, result);
+ }
+ storeChanges(region, key, new RemoveByteArrays(result));
+ return result;
+ }
+
+ private void spopWithSmallCount(int count, List<byte[]> result) {
+ Random rand = new Random();
+ while (result.size() != count) {
+ byte[] member = members.getRandomMemberFromBackingArray(rand);
+ result.add(member);
+ members.remove(member);
}
+ }
- if (popCount >= originalSize) {
- region.remove(key, this);
- return this.members;
+ private void spopWithLargeCount(int count, List<byte[]> result) {
+ if (count >= members.size()) {
+ members.toList(result);
+ members.clear();
+ return;
}
- List<byte[]> popped = new ArrayList<>();
- byte[][] setMembers = members.toArray(new byte[originalSize][]);
Random rand = new Random();
- while (popped.size() < popCount) {
- int idx = rand.nextInt(originalSize);
- byte[] memberToPop = setMembers[idx];
- if (memberToPop != null) {
- setMembers[idx] = null;
- popped.add(memberToPop);
- membersRemove(memberToPop);
- }
+ MemberSet remainingMembers = new MemberSet(members.size() - count);
+ while (members.size() != count) {
+ byte[] member = members.getRandomMemberFromBackingArray(rand);
+ remainingMembers.add(member);
+ members.remove(member);
}
- if (!popped.isEmpty()) {
- storeChanges(region, key, new RemoveByteArrays(popped));
- }
- return popped;
+
+ members.toList(result);
+ members = remainingMembers;
}
public List<byte[]> srandmember(int count) {
- List<byte[]> result = new ArrayList<>();
- int randMethodRatio = 3;
+ final int randMethodRatio = 3; // The ratio is based off command documentation
+ List<byte[]> result;
if (count < 0) {
+ result = new ArrayList<>(-count);
srandomDuplicateList(-count, result);
} else if (count * randMethodRatio < members.size()) {
- // Count is small enough to add random elements to result
+ /*
+ * Count is small enough to add random elements to result.
+ * Random indexes are generated to get members that are stored in the backing array of the
+ * MemberSet.
+ *
+ * Since the MemberSet is not being modified, the members previously found are tracked.
+ */
+ result = new ArrayList<>(count);
srandomUniqueListWithSmallCount(count, result);
} else {
- // Count either equal or greater to member size or close to the member size
+ /*
+ * The count is close to the number of members in the set.
+ * If the same method were used as srandomUniqueListWithSmallCount, when the result size
+ * approaches the count, it gets to a point where a specific random index would need
+ * to be generated to get a valid member (A member that has not been added to the result).
+ *
+ * Copying the members to the result and generating a random index of members we want to
+ * remove from the result solves that issue.
+ *
+ * For example, if you have a set with 100 members, and the backing array has a length of 100.
+ * If we want to get 99 random members, it would take more effort to generate 99 unique
+ * random indexes to add unique members to the list than it would be generate 1 random
+ * index for a 1 member to be removed.
+ */
+ result = new ArrayList<>(Math.min(count, members.size()));
srandomUniqueListWithLargeCount(count, result);
}
return result;
@@ -271,46 +316,38 @@ public class RedisSet extends AbstractRedisData {
private void srandomDuplicateList(int count, List<byte[]> result) {
Random rand = new Random();
while (result.size() != count) {
- int randIndex = rand.nextInt(members.getBackingArrayLength());
- byte[] member = members.getFromBackingArray(randIndex);
- if (member != null) {
- result.add(member);
- }
+ byte[] member = members.getRandomMemberFromBackingArray(rand);
+ result.add(member);
}
}
private void srandomUniqueListWithSmallCount(int count, List<byte[]> result) {
Random rand = new Random();
- Set<Integer> indexesUsed = new HashSet<>();
+ Set<byte[]> membersUsed = new HashSet<>();
while (result.size() != count) {
- int randIndex = rand.nextInt(members.getBackingArrayLength());
- byte[] member = members.getFromBackingArray(randIndex);
- if (member != null && !indexesUsed.contains(randIndex)) {
+ byte[] member = members.getRandomMemberFromBackingArray(rand);
+ if (!membersUsed.contains(member)) {
result.add(member);
- indexesUsed.add(randIndex);
+ membersUsed.add(member);
}
}
}
private void srandomUniqueListWithLargeCount(int count, List<byte[]> result) {
if (count >= members.size()) {
- result.addAll(members);
+ members.toList(result);
return;
}
Random rand = new Random();
MemberSet duplicateSet = new MemberSet(members);
-
while (duplicateSet.size() != count) {
- int randIndex = rand.nextInt(duplicateSet.getBackingArrayLength());
- byte[] member = duplicateSet.getFromBackingArray(randIndex);
- if (member != null) {
- duplicateSet.remove(member);
- }
+ byte[] member = members.getRandomMemberFromBackingArray(rand);
+ duplicateSet.remove(member);
}
- result.addAll(duplicateSet);
+ duplicateSet.toList(result);
}
public boolean sismember(byte[] member) {
@@ -503,6 +540,12 @@ public class RedisSet extends AbstractRedisData {
protected int sizeElement(byte[] element) {
return memoryOverhead(element);
}
+
+ private void toList(List<byte[]> list) {
+ for (byte[] member : this) {
+ list.add(member);
+ }
+ }
}
}
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/collections/SizeableObjectOpenCustomHashSetWithCursor.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/collections/SizeableObjectOpenCustomHashSetWithCursor.java
index dfb077b..213b1f3 100644
--- a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/collections/SizeableObjectOpenCustomHashSetWithCursor.java
+++ b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/collections/SizeableObjectOpenCustomHashSetWithCursor.java
@@ -19,6 +19,7 @@ import static it.unimi.dsi.fastutil.HashCommon.mix;
import static org.apache.geode.internal.JvmSizeUtils.memoryOverhead;
import java.util.Collection;
+import java.util.Random;
import it.unimi.dsi.fastutil.objects.ObjectOpenCustomHashSet;
@@ -66,12 +67,22 @@ public abstract class SizeableObjectOpenCustomHashSetWithCursor<E>
return removed;
}
- public E getFromBackingArray(final int pos) {
- return key[pos];
- }
-
- public int getBackingArrayLength() {
- return key.length;
+ /*
+ * Gets a random member given an index.
+ * If member does not exist at that index, then goes to closest member that is right of it.
+ */
+ public E getRandomMemberFromBackingArray(Random rand) {
+ final int backingArrayLength = key.length;
+ E member;
+ int index = rand.nextInt(backingArrayLength);
+ // ADD CHECK FOR NULLLLL
+ while ((member = key[index]) == null) {
+ ++index;
+ if (index >= backingArrayLength) {
+ index = 0;
+ }
+ }
+ return member;
}
@Override