DonalEvans commented on a change in pull request #7157:
URL: https://github.com/apache/geode/pull/7157#discussion_r760568834
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
##########
@@ -55,12 +59,34 @@ public void sdiffErrors_givenTooFewArguments() {
}
@Test
- public void sdiffstoreErrors_givenTooFewArguments() {
- assertAtLeastNArgs(jedis, Protocol.Command.SDIFFSTORE, 2);
+ public void sdiff_returnsAllValuesInSet() {
+ String[] values = createKeyValuesSet();
+ assertThat(jedis.sdiff("{user1}setkey")).containsExactlyInAnyOrder(values);
Review comment:
Since this "{user1}setkey" String is used in multiple places, it might
be good to make it a constant in the class.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
##########
@@ -55,12 +59,34 @@ public void sdiffErrors_givenTooFewArguments() {
}
@Test
- public void sdiffstoreErrors_givenTooFewArguments() {
- assertAtLeastNArgs(jedis, Protocol.Command.SDIFFSTORE, 2);
+ public void sdiff_returnsAllValuesInSet() {
+ String[] values = createKeyValuesSet();
+ assertThat(jedis.sdiff("{user1}setkey")).containsExactlyInAnyOrder(values);
+ }
+
+ @Test
+ public void sdiffWithNonExistentSet_returnsEmptySet() {
+ assertThat(jedis.sdiff("{user1}nonExistentSet")).isEmpty();
+ }
+
+ @Test
+ public void sdiffWithMultipleNonExistentSet_returnsEmptySet() {
+ assertThat(jedis.sdiff("{user1}nonExistentSet1",
"{user1}nonExistentSet2")).isEmpty();
+ assertThat(jedis.smembers("{user1}nonExistentSet1").isEmpty());
+ assertThat(jedis.smembers("{user1}nonExistentSet2").isEmpty());
+ }
+
+ @Test
+ public void sdiffWithSetAndNonExistentSet_returnsAllValuesInSet() {
Review comment:
For completeness, could we get a version of this test with the
nonexistent set first, to confirm that it returns an empty set?
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -65,6 +66,35 @@ public RedisSet(Collection<byte[]> members) {
*/
public RedisSet() {}
+ public RedisSet(int size) {
+ members = new RedisSet.MemberSet(size);
+ }
+
+ public Set<byte[]> sdiff(RegionProvider regionProvider, List<RedisKey> keys)
{
+ return calculateDiff(regionProvider, keys);
+ }
+
+ private Set<byte[]> calculateDiff(RegionProvider regionProvider,
List<RedisKey> keys) {
+ RedisSet firstSet = regionProvider.getTypedRedisData(REDIS_SET,
keys.get(0), false);
+ if (firstSet.scard() == 0) {
+ return Collections.emptySet();
+ }
+ members.addAll(firstSet.members);
+
+ for (int i = 1; i < keys.size(); i++) {
+ RedisSet curSet = regionProvider.getTypedRedisData(REDIS_SET,
keys.get(i), false);
Review comment:
The failures in `AbstractHitsMissesIntegrationTest` can be fixed by
changing these `false` to `true`. You can test what the expected behaviour of a
given command is using the redis-cli:
```
$> sadd key1 member1
(integer) 1
$> info stats
```
Look for the "keyspace_hits" and "keyspace_misses" stats. They should both
be 0 for a freshly started Redis server.
```
$> sdiff key1 nonexistentKey
1) "member1"
$> info stats
```
You can now use the values of the "keyspace_hits" and "keyspace_misses"
stats to determine if the given command (sdiff in this example) should update
stats or not. If they're still 0, then the command shouldn't update them, if
they've changed, it should.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
##########
@@ -55,12 +59,34 @@ public void sdiffErrors_givenTooFewArguments() {
}
Review comment:
It would be good to add an SDIFF version of the test
`testSDiffStore_withNonSetKey()` in this class.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
##########
@@ -55,12 +59,34 @@ public void sdiffErrors_givenTooFewArguments() {
}
@Test
- public void sdiffstoreErrors_givenTooFewArguments() {
- assertAtLeastNArgs(jedis, Protocol.Command.SDIFFSTORE, 2);
+ public void sdiff_returnsAllValuesInSet() {
+ String[] values = createKeyValuesSet();
+ assertThat(jedis.sdiff("{user1}setkey")).containsExactlyInAnyOrder(values);
+ }
+
+ @Test
+ public void sdiffWithNonExistentSet_returnsEmptySet() {
+ assertThat(jedis.sdiff("{user1}nonExistentSet")).isEmpty();
+ }
+
+ @Test
+ public void sdiffWithMultipleNonExistentSet_returnsEmptySet() {
+ assertThat(jedis.sdiff("{user1}nonExistentSet1",
"{user1}nonExistentSet2")).isEmpty();
+ assertThat(jedis.smembers("{user1}nonExistentSet1").isEmpty());
+ assertThat(jedis.smembers("{user1}nonExistentSet2").isEmpty());
Review comment:
These calls to `smembers()` can probably be removed, as they're not
really relevant to the behaviour of SDIFF.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
##########
@@ -55,12 +59,34 @@ public void sdiffErrors_givenTooFewArguments() {
}
@Test
- public void sdiffstoreErrors_givenTooFewArguments() {
- assertAtLeastNArgs(jedis, Protocol.Command.SDIFFSTORE, 2);
+ public void sdiff_returnsAllValuesInSet() {
+ String[] values = createKeyValuesSet();
+ assertThat(jedis.sdiff("{user1}setkey")).containsExactlyInAnyOrder(values);
+ }
+
+ @Test
+ public void sdiffWithNonExistentSet_returnsEmptySet() {
+ assertThat(jedis.sdiff("{user1}nonExistentSet")).isEmpty();
+ }
+
+ @Test
+ public void sdiffWithMultipleNonExistentSet_returnsEmptySet() {
+ assertThat(jedis.sdiff("{user1}nonExistentSet1",
"{user1}nonExistentSet2")).isEmpty();
+ assertThat(jedis.smembers("{user1}nonExistentSet1").isEmpty());
+ assertThat(jedis.smembers("{user1}nonExistentSet2").isEmpty());
+ }
+
+ @Test
+ public void sdiffWithSetAndNonExistentSet_returnsAllValuesInSet() {
+ String[] values = createKeyValuesSet();
+ assertThat(jedis.sdiff("{user1}setkey", "{user1}nonExistentSet"))
+ .containsExactlyInAnyOrder(values);
+
assertThat(jedis.smembers("{user1}setkey")).containsExactlyInAnyOrder(values);
+ assertThat(jedis.smembers("{user1}nonExistentSet")).isEmpty();
}
@Test
- public void testSDiff() {
+ public void sdiffWithMultipleSets() {
Review comment:
This test name could be a little more descriptive. Maybe something like
"sdiff_returnsDiffOfMultipleSets". Also, the test seems to be testing multiple
things; that SDIFF returns the correct diff, that it doesn't modify the
contents of the first set passed to it, that it returns an empty set if the
first set is empty, and that it returns the contents of the set if passed just
one set. These cases should be broken out into individual tests, although some
of them are already covered by existing tests in this class.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
##########
@@ -30,6 +33,7 @@
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;
Review comment:
Not code you added, but we can remove the `REDIS_CLIENT_TIMEOUT`
constant below from this class as it's defined in `RedisClusterStartupRule`. We
can also replace the "localhost" String in the `HostAndPort` constructor with
the `BIND_ADDRESS` constant in `RedisClusterStartupRule`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
