You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/12/01 18:50:42 UTC
[GitHub] [geode] Kris-10-0 opened a new pull request #7157: GEODE-9827: SDIFF command supported
Kris-10-0 opened a new pull request #7157:
URL: https://github.com/apache/geode/pull/7157
<!-- Thank you for submitting a contribution to Apache Geode. -->
<!-- In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
-->
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [x] Is your initial contribution a single, squashed commit?
- [x] Does `gradlew build` run cleanly?
- [x] Have you written or updated unit tests to verify your changes?
- [NA] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
<!-- Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
-->
--
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
[GitHub] [geode] DonalEvans commented on a change in pull request #7157: GEODE-9827: SDIFF command supported
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7157:
URL: https://github.com/apache/geode/pull/7157#discussion_r760555448
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetOpExecutor.java
##########
@@ -41,12 +43,36 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
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),
Review comment:
The `lockedExecute()` method sorts the list passed to it, which results in the order of the keys in `setKeys` changing if it's passed in. Since we also pass `setKeys` into the `sdiff()` method, this results in an incorrect result for the diff, hence the need to pass a copy to one of the methods.
--
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
[GitHub] [geode] DonalEvans commented on a change in pull request #7157: GEODE-9827: SDIFF command supported
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7157:
URL: https://github.com/apache/geode/pull/7157#discussion_r761366823
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetOpExecutor.java
##########
@@ -60,10 +61,10 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
* refactor so it implements doSetOp
*/
+ // Make a static method then calculate diff add all create the resulting set
Review comment:
This comment should probably be removed.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
##########
@@ -61,53 +63,83 @@ public void sdiffErrors_givenTooFewArguments() {
@Test
public void sdiff_returnsAllValuesInSet() {
String[] values = createKeyValuesSet();
- assertThat(jedis.sdiff("{user1}setkey")).containsExactlyInAnyOrder(values);
+ assertThat(jedis.sdiff(setKey)).containsExactlyInAnyOrder(values);
}
@Test
public void sdiffWithNonExistentSet_returnsEmptySet() {
- assertThat(jedis.sdiff("{user1}nonExistentSet")).isEmpty();
+ assertThat(jedis.sdiff(nonExistentSetKey)).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 sdiffWithNonExistentSetAndSet_returnsAllValuesInSet() {
+ String[] values = createKeyValuesSet();
+ assertThat(jedis.sdiff(nonExistentSetKey, setKey)).isEmpty();
+ assertThat(jedis.smembers(nonExistentSetKey)).isEmpty();
+ assertThat(jedis.smembers(setKey)).containsExactlyInAnyOrder(values);
Review comment:
These smembers assertions are unnecessary, I think. This test isn't trying to assert that the contents of the sets are unchanged, just that we get the expected return value from SDIFF. If we want to have a test that SDIFF doesn't modify the sets its called on, that should be a separate test case.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
##########
@@ -61,53 +63,83 @@ public void sdiffErrors_givenTooFewArguments() {
@Test
public void sdiff_returnsAllValuesInSet() {
String[] values = createKeyValuesSet();
- assertThat(jedis.sdiff("{user1}setkey")).containsExactlyInAnyOrder(values);
+ assertThat(jedis.sdiff(setKey)).containsExactlyInAnyOrder(values);
}
@Test
public void sdiffWithNonExistentSet_returnsEmptySet() {
- assertThat(jedis.sdiff("{user1}nonExistentSet")).isEmpty();
+ assertThat(jedis.sdiff(nonExistentSetKey)).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 sdiffWithNonExistentSetAndSet_returnsAllValuesInSet() {
+ String[] values = createKeyValuesSet();
+ assertThat(jedis.sdiff(nonExistentSetKey, setKey)).isEmpty();
+ assertThat(jedis.smembers(nonExistentSetKey)).isEmpty();
+ assertThat(jedis.smembers(setKey)).containsExactlyInAnyOrder(values);
}
@Test
public void sdiffWithSetAndNonExistentSet_returnsAllValuesInSet() {
String[] values = createKeyValuesSet();
- assertThat(jedis.sdiff("{user1}setkey", "{user1}nonExistentSet"))
+ assertThat(jedis.sdiff(setKey, nonExistentSetKey))
.containsExactlyInAnyOrder(values);
- assertThat(jedis.smembers("{user1}setkey")).containsExactlyInAnyOrder(values);
- assertThat(jedis.smembers("{user1}nonExistentSet")).isEmpty();
+ assertThat(jedis.smembers(setKey)).containsExactlyInAnyOrder(values);
+ assertThat(jedis.smembers(nonExistentSetKey)).isEmpty();
Review comment:
These smembers asserts are also unnecessary, I think.
--
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
[GitHub] [geode] Kris-10-0 commented on a change in pull request #7157: GEODE-9827: SDIFF command supported
Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on a change in pull request #7157:
URL: https://github.com/apache/geode/pull/7157#discussion_r760627365
##########
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:
Like a key that is used for another data structure type. IE. a key that is in sorted sets?
--
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
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7157: GEODE-9827: SDIFF command supported
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7157:
URL: https://github.com/apache/geode/pull/7157#discussion_r760502200
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetOpExecutor.java
##########
@@ -41,12 +43,36 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
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),
Review comment:
Could you just pass in "setKeys" instead of "new ArrayList<>(setKeys)"? setKeys is a sublist but I think it allows changes to the sublist which seems okay in this context.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -65,6 +67,35 @@ public RedisSet(Collection<byte[]> members) {
*/
public RedisSet() {}
+ public RedisSet(int size) {
+ this.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 == NULL_REDIS_SET) {
Review comment:
better would be "if (firstSet.scard() == 0)". It is easier to understand since it deals with the firstSet being empty instead of the special NULL_REDIS_SET.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -65,6 +67,35 @@ public RedisSet(Collection<byte[]> members) {
*/
public RedisSet() {}
+ public RedisSet(int size) {
+ this.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 == NULL_REDIS_SET) {
+ 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);
+ if (curSet == NULL_REDIS_SET) {
Review comment:
better would be "if (curSet.scard() == 0)".
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -65,6 +67,35 @@ public RedisSet(Collection<byte[]> members) {
*/
public RedisSet() {}
+ public RedisSet(int size) {
+ this.members = new RedisSet.MemberSet(size);
+ }
+
+ public Set<byte[]> sdiff(RegionProvider regionProvider, List<RedisKey> keys) {
Review comment:
I don't like sdiff being an instance method on RedisSet. It seems like the implementation counts on it always starting with an empty RedisSet. And that is how we use it. So it seems like the only RedisSet instances we care about are the existing ones we will read. So I think it is a bit confusing to create an empty RedisSet instance and then throw it away after we get the members out of it. I think it would be better for sdiff and calculateDIff to be "static" and the "members" in calculateDiff to be a local variable that is created like so: "Set<byte[]> members = new MemberSet(firstSet.members);". This will also give you a better initial size that will do fewer rehashes.
Also you should probably rename "members" to something like "diff".
Instead of static methods on RedisSet these could be on SetOpExecutor but since they read the internals of the RedisSet instances I think it is better on RedisSet.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -65,6 +67,35 @@ public RedisSet(Collection<byte[]> members) {
*/
public RedisSet() {}
+ public RedisSet(int size) {
+ this.members = new RedisSet.MemberSet(size);
Review comment:
Since this line is in RedisSet can you change it to "new MemberSet(size)"?
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
##########
@@ -125,6 +180,19 @@ public void testSDiffStore() {
assertThat(copyResultSet.toArray()).containsExactlyInAnyOrder((Object[]) secondSet);
}
+ // One array
Review comment:
it is not clear what the purpose of this comment is. Do you want to remove it? If not can you add an explanation.
--
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
[GitHub] [geode] DonalEvans commented on a change in pull request #7157: GEODE-9827: SDIFF command supported
Posted by GitBox <gi...@apache.org>.
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] Kris-10-0 commented on a change in pull request #7157: GEODE-9827: SDIFF command supported
Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on a change in pull request #7157:
URL: https://github.com/apache/geode/pull/7157#discussion_r760550219
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSDiffIntegrationTest.java
##########
@@ -125,6 +180,19 @@ public void testSDiffStore() {
assertThat(copyResultSet.toArray()).containsExactlyInAnyOrder((Object[]) secondSet);
}
+ // One array
Review comment:
That was notes to myself. Removed it!
--
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
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7157: GEODE-9827: SDIFF command supported
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7157:
URL: https://github.com/apache/geode/pull/7157#discussion_r760802271
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetOpExecutor.java
##########
@@ -41,12 +43,36 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
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),
Review comment:
Thanks for the explanation. I agree
--
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
[GitHub] [geode] DonalEvans commented on a change in pull request #7157: GEODE-9827: SDIFF command supported
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7157:
URL: https://github.com/apache/geode/pull/7157#discussion_r760631817
##########
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:
Using a sorted set is a bit iffy, because native Redis does support doing diff/union/inter of a mixture of sorted sets and sets, but we don't currently support that in geode-for-redis. It'd be best to use a string, something like:
```
@Test
public void sdiff_withNonSetKey_throwsWrongtypeError() {
jedis.sadd("{user1}set1", "member");
jedis.set("{user1}string1", "value");
assertThatThrownBy(() -> jedis.sdiff("{user1}set1", "{user1}string1"))
.hasMessage("WRONGTYPE Operation against a key holding the wrong kind of value");
}
```
--
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
[GitHub] [geode] Kris-10-0 commented on a change in pull request #7157: GEODE-9827: SDIFF command supported
Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on a change in pull request #7157:
URL: https://github.com/apache/geode/pull/7157#discussion_r760553370
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetOpExecutor.java
##########
@@ -41,12 +43,36 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
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),
Review comment:
If I passed in setKeys directly in then it modifies the list and removes all the keys. @DonalEvans and I were using ZUnionStore as a guide to do this, and it was also passing a new list for the keys.
--
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
[GitHub] [geode] DonalEvans merged pull request #7157: GEODE-9827: SDIFF command supported
Posted by GitBox <gi...@apache.org>.
DonalEvans merged pull request #7157:
URL: https://github.com/apache/geode/pull/7157
--
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