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 2021/12/02 22:41:46 UTC
[geode] branch develop updated: GEODE-9827: SDIFF command supported (#7157)
This is an automated email from the ASF dual-hosted git repository.
donalevans 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 b13243e GEODE-9827: SDIFF command supported (#7157)
b13243e is described below
commit b13243e091e073c896a66e5d755eb967c1f8c1f3
Author: Kris10 <ko...@vmware.com>
AuthorDate: Thu Dec 2 14:40:04 2021 -0800
GEODE-9827: SDIFF command supported (#7157)
---
.../tools_modules/geode_for_redis.html.md.erb | 1 +
geode-for-redis/README.md | 1 +
.../server/AbstractHitsMissesIntegrationTest.java | 14 +-
.../executor/set/AbstractSDiffIntegrationTest.java | 162 ++++++++++++++++++---
.../redis/internal/commands/RedisCommandType.java | 4 +-
.../commands/executor/set/SetOpExecutor.java | 34 ++++-
.../apache/geode/redis/internal/data/RedisSet.java | 30 ++++
7 files changed, 210 insertions(+), 36 deletions(-)
diff --git a/geode-docs/tools_modules/geode_for_redis.html.md.erb b/geode-docs/tools_modules/geode_for_redis.html.md.erb
index 34952a7..dce1de4 100644
--- a/geode-docs/tools_modules/geode_for_redis.html.md.erb
+++ b/geode-docs/tools_modules/geode_for_redis.html.md.erb
@@ -111,6 +111,7 @@ If the server is functioning properly, you should see a response of `PONG`.
- QUIT <br/>
- RENAME <br/>
- SADD <br/>
+ - SDIFF <br/>
- SET <br/>
- SETNX <br/>
- SLOWLOG **[3]** <br/>
diff --git a/geode-for-redis/README.md b/geode-for-redis/README.md
index 82ed0ed..d22d49b 100644
--- a/geode-for-redis/README.md
+++ b/geode-for-redis/README.md
@@ -197,6 +197,7 @@ Geode for Redis implements a subset of the full Redis command set.
- RENAME
- RENAMENX
- SADD
+- SDIFF
- SET
- SETEX
- SETNX
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 f670b45..cc68ea7 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
@@ -374,8 +374,8 @@ public abstract class AbstractHitsMissesIntegrationTest implements RedisIntegrat
}
@Test
- public void testSrem() {
- runCommandAndAssertNoStatUpdates(SET_KEY, k -> jedis.srem(k, "member"));
+ public void testSdiff() {
+ runMultiKeyCommandAndAssertHitsAndMisses(SET_KEY, (k1, k2) -> jedis.sdiff(k1, k2));
}
@Test
@@ -383,6 +383,11 @@ public abstract class AbstractHitsMissesIntegrationTest implements RedisIntegrat
runCommandAndAssertHitsAndMisses(SET_KEY, k -> jedis.smembers(k));
}
+ @Test
+ public void testSrem() {
+ runCommandAndAssertNoStatUpdates(SET_KEY, k -> jedis.srem(k, "member"));
+ }
+
/************* Hash related commands *************/
@Test
public void testHset() {
@@ -534,11 +539,6 @@ public abstract class AbstractHitsMissesIntegrationTest implements RedisIntegrat
}
@Test
- public void testSdiff() {
- runMultiKeyCommandAndAssertHitsAndMisses(SET_KEY, (k1, k2) -> jedis.sdiff(k1, k2));
- }
-
- @Test
public void testSdiffstore() {
runMultiKeyCommandAndAssertNoStatUpdates(SET_KEY,
(k1, k2) -> jedis.sdiffstore(HASHTAG + "dest", k1, k2));
diff --git a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
index 96e4be6..6fe3db6 100755
--- a/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
+++ b/geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
@@ -15,13 +15,18 @@
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_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.util.ArrayList;
+import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
import org.junit.After;
import org.junit.Before;
@@ -30,17 +35,17 @@ 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 AbstractSDiffIntegrationTest implements RedisIntegrationTest {
private JedisCluster jedis;
- private static final int REDIS_CLIENT_TIMEOUT =
- Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+ private static final String setKey = "{user1}setkey";
+ private static final String nonExistentSetKey = "{user1}nonExistentSet";
@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
@@ -55,33 +60,140 @@ public abstract class AbstractSDiffIntegrationTest implements RedisIntegrationTe
}
@Test
- public void sdiffstoreErrors_givenTooFewArguments() {
- assertAtLeastNArgs(jedis, Protocol.Command.SDIFFSTORE, 2);
+ public void sdiff_returnsAllValuesInSet() {
+ String[] values = createKeyValuesSet();
+ assertThat(jedis.sdiff(setKey)).containsExactlyInAnyOrder(values);
}
@Test
- public void testSDiff() {
- String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
- String[] secondSet = new String[] {"apple", "microsoft", "linux"};
- String[] thirdSet = new String[] {"luigi", "bowser", "peach", "mario"};
- jedis.sadd("{user1}set1", firstSet);
- jedis.sadd("{user1}set2", secondSet);
- jedis.sadd("{user1}set3", thirdSet);
+ public void sdiffWithNonExistentSet_returnsEmptySet() {
+ assertThat(jedis.sdiff(nonExistentSetKey)).isEmpty();
+ }
+
+ @Test
+ public void sdiffWithMultipleNonExistentSet_returnsEmptySet() {
+ assertThat(jedis.sdiff("{user1}nonExistentSet1", "{user1}nonExistentSet2")).isEmpty();
+ }
+
+ @Test
+ public void sdiffWithNonExistentSetAndSet_returnsAllValuesInSet() {
+ createKeyValuesSet();
+ assertThat(jedis.sdiff(nonExistentSetKey, setKey)).isEmpty();
+ }
+
+ @Test
+ public void sdiffWithSetAndNonExistentSet_returnsAllValuesInSet() {
+ String[] values = createKeyValuesSet();
+ assertThat(jedis.sdiff(setKey, nonExistentSetKey))
+ .containsExactlyInAnyOrder(values);
+ }
+
+ @Test
+ public void sdiffWithSetsWithDifferentValues_returnsFirstSetValues() {
+ String[] firstValues = createKeyValuesSet();
+ String[] secondValues = new String[] {"windows", "microsoft", "linux"};
+ jedis.sadd("{user1}setkey2", secondValues);
+
+ assertThat(jedis.sdiff(setKey, "{user1}setkey2")).containsExactlyInAnyOrder(firstValues);
+ }
+
+ @Test
+ public void sdiffWithSetsWithSomeSharedValues_returnsDiffOfSets() {
+ createKeyValuesSet();
+ String[] secondValues = new String[] {"apple", "bottoms", "boots", "fur", "peach"};
+ jedis.sadd("{user1}setkey2", secondValues);
Set<String> result =
- jedis.sdiff("{user1}set1", "{user1}set2", "{user1}set3", "{user1}doesNotExist");
- String[] expected = new String[] {"pear", "plum", "orange"};
+ jedis.sdiff(setKey, "{user1}setkey2");
+ String[] expected = new String[] {"orange", "plum", "pear"};
assertThat(result).containsExactlyInAnyOrder(expected);
+ }
+
+ @Test
+ public void sdiffWithSetsWithAllSharedValues_returnsEmptySet() {
+ String[] values = createKeyValuesSet();
+ jedis.sadd("{user1}setkey2", values);
+ assertThat(jedis.sdiff(setKey, "{user1}setkey2")).isEmpty();
+ }
+
+ @Test
+ public void sdiffWithMultipleSets_returnsDiffOfSets() {
+ String[] values = createKeyValuesSet();
+ String[] secondValues = new String[] {"apple", "bottoms", "boots", "fur", "peach"};
+ String[] thirdValues = new String[] {"queen", "opera", "boho", "orange"};
+
+ jedis.sadd("{user1}setkey2", secondValues);
+ jedis.sadd("{user1}setkey3", thirdValues);
+
+ String[] expected = new String[] {"pear", "plum"};
+ assertThat(jedis.sdiff(setKey, "{user1}setkey2", "{user1}setkey3"))
+ .containsExactlyInAnyOrder(expected);
+ }
+
+ @Test
+ public void sdiffSetsNotModified_returnSetValues() {
+ String[] firstValues = createKeyValuesSet();
+ String[] secondValues = new String[] {"apple", "bottoms", "boots", "fur", "peach"};
+ jedis.sadd("{user1}setkey2", secondValues);
+ jedis.sdiff(setKey, "{user1}setkey2");
+ assertThat(jedis.smembers(setKey)).containsExactlyInAnyOrder(firstValues);
+ assertThat(jedis.smembers("{user1}setkey2")).containsExactlyInAnyOrder(secondValues);
+ }
+
+ @Test
+ public void sdiffNonExistentSetsNotModified_returnEmptySet() {
+ jedis.sdiff(nonExistentSetKey, "{user1}nonExisistent2");
+ assertThat(jedis.smembers(nonExistentSetKey)).isEmpty();
+ assertThat(jedis.smembers("{user1}nonExisistent2")).isEmpty();
+ }
+
+ @Test
+ public void sdiffNonExistentSetAndSetNotModified_returnEmptySetAndSetValues() {
+ String[] firstValues = createKeyValuesSet();
+ jedis.sdiff(nonExistentSetKey, setKey);
+ assertThat(jedis.smembers(nonExistentSetKey).isEmpty());
+ assertThat(jedis.smembers(setKey)).containsExactlyInAnyOrder(firstValues);
+ }
+
+ @Test
+ public void sdiffSetAndNonExistentSetNotModified_returnSetValueAndEmptySet() {
+ String[] firstValues = createKeyValuesSet();
+ jedis.sdiff(setKey, nonExistentSetKey);
+ assertThat(jedis.smembers(setKey)).containsExactlyInAnyOrder(firstValues);
+ assertThat(jedis.smembers(nonExistentSetKey).isEmpty());
+ }
+
+ @Test
+ public void sdiffWithDifferentyKeyType_returnsWrongTypeError() {
+ jedis.set("ding", "dong");
+ assertThatThrownBy(() -> jedis.sdiff("ding")).hasMessageContaining(ERROR_WRONG_TYPE);
+ }
- Set<String> shouldNotChange = jedis.smembers("{user1}set1");
- assertThat(shouldNotChange).containsExactlyInAnyOrder(firstSet);
+ @Test
+ public void ensureSetConsistency_whenRunningConcurrently() {
+ String[] values = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ Set<String> valuesList = new HashSet<>(Arrays.asList(values));
+
+ jedis.sadd("{user1}firstset", values);
+ jedis.sadd("{user1}secondset", values);
+
+ final AtomicReference<Set<String>> sdiffResultReference = new AtomicReference<>();
+ new ConcurrentLoopingThreads(1000,
+ i -> jedis.srem("{user1}secondset", values),
+ i -> sdiffResultReference.set(jedis.sdiff("{user1}firstset", "{user1}secondset")))
+ .runWithAction(() -> {
+ assertThat(sdiffResultReference).satisfiesAnyOf(
+ sdiffResult -> assertThat(sdiffResult.get()).isEmpty(),
+ sdiffResult -> assertThat(sdiffResult.get())
+ .containsExactlyInAnyOrderElementsOf(valuesList));
+ jedis.sadd("{user1}secondset", values);
+ });
+ }
- Set<String> shouldBeEmpty =
- jedis.sdiff("{user1}doesNotExist", "{user1}set1", "{user1}set2", "{user1}set3");
- assertThat(shouldBeEmpty).isEmpty();
- Set<String> copySet = jedis.sdiff("{user1}set1");
- assertThat(copySet).containsExactlyInAnyOrder(firstSet);
+ @Test
+ public void sdiffstoreErrors_givenTooFewArguments() {
+ assertAtLeastNArgs(jedis, Protocol.Command.SDIFFSTORE, 2);
}
@Test
@@ -207,4 +319,10 @@ public abstract class AbstractSDiffIntegrationTest implements RedisIntegrationTe
assertThat(jedis.smembers("{user1}master").toArray())
.containsExactlyInAnyOrder(masterSet.toArray());
}
+
+ private String[] createKeyValuesSet() {
+ String[] values = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ jedis.sadd("{user1}setkey", values);
+ return values;
+ }
}
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 bdc5a30..e5ea22e 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
@@ -240,6 +240,8 @@ public enum RedisCommandType {
/************* Sets *****************/
SADD(new SAddExecutor(), SUPPORTED, new Parameter().min(3).flags(WRITE, DENYOOM, FAST)),
+ SDIFF(new SDiffExecutor(), SUPPORTED,
+ new Parameter().min(2).lastKey(-1).flags(READONLY, SORT_FOR_SCRIPT)),
SMEMBERS(new SMembersExecutor(), SUPPORTED,
new Parameter().exact(2).flags(READONLY, SORT_FOR_SCRIPT)),
SREM(new SRemExecutor(), SUPPORTED, new Parameter().min(3).flags(WRITE, FAST)),
@@ -340,8 +342,6 @@ public enum RedisCommandType {
/**************** Sets *****************/
SCARD(new SCardExecutor(), UNSUPPORTED, new Parameter().exact(2).flags(READONLY, FAST)),
- SDIFF(new SDiffExecutor(), UNSUPPORTED,
- new Parameter().min(2).lastKey(-1).flags(READONLY, SORT_FOR_SCRIPT)),
SDIFFSTORE(new SDiffStoreExecutor(), UNSUPPORTED,
new Parameter().min(3).lastKey(-1).flags(WRITE, DENYOOM)),
SINTER(new SInterExecutor(), UNSUPPORTED,
diff --git a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetOpExecutor.java b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetOpExecutor.java
index bc4ada7..9086ac0 100755
--- a/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetOpExecutor.java
+++ b/geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetOpExecutor.java
@@ -15,6 +15,7 @@
package org.apache.geode.redis.internal.commands.executor.set;
import static java.util.Collections.emptySet;
+import static org.apache.geode.redis.internal.data.RedisSet.sdiff;
import java.util.ArrayList;
import java.util.List;
@@ -24,10 +25,12 @@ import org.apache.geode.redis.internal.commands.Command;
import org.apache.geode.redis.internal.commands.RedisCommandType;
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.RedisDataMovedException;
import org.apache.geode.redis.internal.data.RedisKey;
import org.apache.geode.redis.internal.data.RedisSet;
import org.apache.geode.redis.internal.data.RedisSet.MemberSet;
import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+import org.apache.geode.redis.internal.services.RegionProvider;
public abstract class SetOpExecutor implements CommandExecutor {
@@ -41,12 +44,33 @@ public abstract class SetOpExecutor implements CommandExecutor {
List<RedisKey> commandElements = command.getProcessedCommandKeys();
List<RedisKey> setKeys = commandElements.subList(setsStartIndex, commandElements.size());
- if (isStorage()) {
- RedisKey destination = command.getKey();
- int storeCount = doStoreSetOp(command.getCommandType(), context, destination, setKeys);
- return RedisResponse.integer(storeCount);
+ RegionProvider regionProvider = context.getRegionProvider();
+ try {
+ for (RedisKey k : setKeys) {
+ regionProvider.ensureKeyIsLocal(k);
+ }
+ } catch (RedisDataMovedException ex) {
+ return RedisResponse.error(ex.getMessage());
+ }
+
+ /*
+ * SDIFFSTORE, SINTER, SINTERSTORE, SUNION, SUNIONSTORE currently use the else part of the code
+ * for their implementation.
+ * TODO: Once the above commands have been implemented remove the if else and
+ * refactor so it implements doSetOp
+ */
+ if (command.isOfType(RedisCommandType.SDIFF)) {
+ Set<byte[]> resultSet = context.lockedExecute(setKeys.get(0), new ArrayList<>(setKeys),
+ () -> sdiff(regionProvider, setKeys));
+ return RedisResponse.array(resultSet, true);
} else {
- return doActualSetOperation(context, setKeys);
+ if (isStorage()) {
+ RedisKey destination = command.getKey();
+ int storeCount = doStoreSetOp(command.getCommandType(), context, destination, setKeys);
+ return RedisResponse.integer(storeCount);
+ } else {
+ return doActualSetOperation(context, setKeys);
+ }
}
}
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 32c3ccb..9d036ae0 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
@@ -47,6 +47,7 @@ import org.apache.geode.redis.internal.data.collections.SizeableObjectOpenCustom
import org.apache.geode.redis.internal.data.delta.AddsDeltaInfo;
import org.apache.geode.redis.internal.data.delta.DeltaInfo;
import org.apache.geode.redis.internal.data.delta.RemsDeltaInfo;
+import org.apache.geode.redis.internal.services.RegionProvider;
public class RedisSet extends AbstractRedisData {
protected static final int REDIS_SET_OVERHEAD = memoryOverhead(RedisSet.class);
@@ -65,6 +66,35 @@ public class RedisSet extends AbstractRedisData {
*/
public RedisSet() {}
+ public RedisSet(int size) {
+ members = new RedisSet.MemberSet(size);
+ }
+
+ public static Set<byte[]> sdiff(RegionProvider regionProvider, List<RedisKey> keys) {
+ return calculateDiff(regionProvider, keys);
+ }
+
+ private static Set<byte[]> calculateDiff(RegionProvider regionProvider, List<RedisKey> keys) {
+ RedisSet firstSet = regionProvider.getTypedRedisData(REDIS_SET, keys.get(0), true);
+ if (firstSet.scard() == 0) {
+ return Collections.emptySet();
+ }
+ Set<byte[]> diff = new MemberSet(firstSet.members);
+
+ for (int i = 1; i < keys.size(); i++) {
+ RedisSet curSet = regionProvider.getTypedRedisData(REDIS_SET, keys.get(i), true);
+ if (curSet.scard() == 0) {
+ continue;
+ }
+
+ diff.removeAll(curSet.members);
+ if (diff.isEmpty()) {
+ return Collections.emptySet();
+ }
+ }
+ return diff;
+ }
+
public Pair<BigInteger, List<Object>> sscan(GlobPattern matchPattern, int count,
BigInteger cursor) {
List<Object> returnList = new ArrayList<>();