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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
