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 2022/01/04 19:00:23 UTC
[GitHub] [geode] BalaKaza opened a new pull request #7236: GEODE-9829: Add SINTER command to Redis supported commands.
BalaKaza opened a new pull request #7236:
URL: https://github.com/apache/geode/pull/7236
SINTER command is implemented and integration tests and added to test this
command.
Co-authored-by: Bala Kaza Venkata <bk...@vmware.com>
Co-authored-by: Steve Sienkowski <ss...@vmware.com>
Co-authored-by: Kristen Oduca <ko...@vmware.com>
<!-- 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:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] 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 #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r778994415
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -110,6 +111,47 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ MemberSet result = calculateInter(regionProvider, keys);
+ if (result == null) {
+ return Collections.emptySet();
+ }
+ return result;
+ }
+
+ private static MemberSet calculateInter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = new ArrayList<>(keys.size());
+ RedisSet smallestSet = null;
+
+ for (RedisKey key : keys) {
+ RedisSet redisSet = regionProvider.getTypedRedisData(REDIS_SET, key, true);
+ if (redisSet == NULL_REDIS_SET || redisSet.scard() == 0) {
+ return null;
Review comment:
If you inline the `calculateInter()` method, then returning `Collections.emptySet()` here is fine, since `sinter()` returns a `Set<byte[]>`
--
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] jdeppe-pivotal commented on a change in pull request #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r780537973
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -110,6 +111,42 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = new ArrayList<>(keys.size());
+ RedisSet smallestSet = NULL_REDIS_SET;
+
+ for (RedisKey key : keys) {
+ RedisSet redisSet = regionProvider.getTypedRedisData(REDIS_SET, key, true);
+ if (redisSet == NULL_REDIS_SET) {
+ return Collections.emptySet();
+ }
+ if (smallestSet == NULL_REDIS_SET) {
+ smallestSet = redisSet;
+ }
+ if (smallestSet.scard() > redisSet.scard()) {
+ sets.add(smallestSet);
+ smallestSet = redisSet;
+ } else {
+ sets.add(redisSet);
+ }
+ }
+
+ MemberSet result = new MemberSet(smallestSet.scard());
+ for (byte[] member : smallestSet.members) {
+ boolean addToSet = true;
+ for (RedisSet otherSet : sets) {
+ if (!otherSet.members.contains(member)) {
+ addToSet = false;
+ break;
+ }
+ }
+ if (addToSet) {
+ result.add(member);
+ }
Review comment:
The intent of this loop is to ensure that the member is in **all** the sets (intersection). The suggested change would add the member if it appears in even one (of multiple) 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 #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r781451512
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -110,6 +111,42 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = new ArrayList<>(keys.size());
+ RedisSet smallestSet = NULL_REDIS_SET;
Review comment:
I wonder if this code would be easier to understand if you just did the following:
```
List<RedisSet> sets = createRedisSetList(keys, regionProvider);
RedisSet smallestSet = findSmallest(sets);
if (smallestSet.scard() == 0) {
return emptySet();
}
```
now create the result with the existing code except as you iterate otherSet checks to see if "otherSet == smallestSet" and if so skip it.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -110,6 +111,42 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = new ArrayList<>(keys.size());
+ RedisSet smallestSet = NULL_REDIS_SET;
+
+ for (RedisKey key : keys) {
+ RedisSet redisSet = regionProvider.getTypedRedisData(REDIS_SET, key, true);
+ if (redisSet == NULL_REDIS_SET) {
+ return Collections.emptySet();
+ }
+ if (smallestSet == NULL_REDIS_SET) {
+ smallestSet = redisSet;
Review comment:
This still ends up adding the initial "smallestSet" to "sets". You could fix this by adding an "else" to this "if".
--
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 #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r779143928
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSInterIntegrationTest.java
##########
@@ -41,6 +40,9 @@
private JedisCluster jedis;
private static final int REDIS_CLIENT_TIMEOUT =
Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+ private static final String REDIS_USER_SET1 = "{user1}set1";
+ private static final String REDIS_USER_SET2 = "{user1}set2";
+ private static final String REDIS_USER_SET3 = "{user1}set3";
Review comment:
The naming of these constants is potentially a little misleading. The use of a tag at the beginning of the key is not related to users at all, but rather provides a way for Redis (or geode-for-redis) to guarantee that keys will be mapped to the same slot in the database ([see the documentation for the CLUSTER KEYSLOT command](https://redis.io/commands/cluster-keyslot)). These keys could just as well be "{tag}set1", "{tag}set2" etc. so having the constants be named "REDIS_USER_*" could cause confusion.
I think it might be best to change the constant names to just "SET1", "SET2" etc. and change the tag to something more generic so that there's no confusion in future.
--
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 #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r783283487
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -127,6 +128,50 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = createRedisSetList(keys, regionProvider);
+ RedisSet smallestSet = findSmallest(sets);
+ if (smallestSet.scard() == 0) {
+ return Collections.emptySet();
+ }
+
+ MemberSet result = new MemberSet(smallestSet.scard());
+ for (byte[] member : smallestSet.members) {
+ boolean addToSet = true;
+ for (RedisSet otherSet : sets)
Review comment:
add a curly bracket to this "for" loop for its body. Our coding standard requires we always add brackets even if a body has one statement
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -127,6 +128,50 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = createRedisSetList(keys, regionProvider);
+ RedisSet smallestSet = findSmallest(sets);
+ if (smallestSet.scard() == 0) {
+ return Collections.emptySet();
+ }
+
+ MemberSet result = new MemberSet(smallestSet.scard());
+ for (byte[] member : smallestSet.members) {
+ boolean addToSet = true;
+ for (RedisSet otherSet : sets)
+ if (!otherSet.equals(smallestSet) && !otherSet.members.contains(member)) {
Review comment:
don't use "equals()" here. It is too expensive. I think `if (otherSet == smallestSet) {continue;}` is what you want. This is just an optimization (the code would work without this check) but it is waste of time. But by using "equals()" we even waste more time.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -127,6 +128,50 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = createRedisSetList(keys, regionProvider);
+ RedisSet smallestSet = findSmallest(sets);
Review comment:
add "final" just to make clear "smallestSet" will not be changing
--
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 #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r783315322
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetOpExecutor.java
##########
@@ -73,8 +74,11 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
Set<byte[]> resultSet = context.lockedExecute(setKeys.get(0), new ArrayList<>(setKeys),
() -> sdiff(regionProvider, setKeys));
return RedisResponse.array(resultSet, true);
+ } else if (command.isOfType(RedisCommandType.SINTER)) {
Review comment:
remove SINTER from comment on line 59
--
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 #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r782606051
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSInterIntegrationTest.java
##########
@@ -30,13 +32,17 @@
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 AbstractSInterIntegrationTest implements RedisIntegrationTest {
private JedisCluster jedis;
private static final int REDIS_CLIENT_TIMEOUT =
Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+ private static final String SET1 = "{user1}set1";
+ private static final String SET2 = "{user1}set2";
+ private static final String SET3 = "{user1}set3";
Review comment:
Could these (and other instances of the `{user1}` tag in this class) be renamed to `{tag1}` etc. to preserve the changes from GEODE-9932?
--
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 #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r783329651
##########
File path: build.gradle
##########
@@ -26,7 +26,7 @@ plugins {
id "nebula.lint" version "17.1.1" apply false
id "com.palantir.docker" version "0.28.0" apply false
id "io.spring.dependency-management" version "1.0.11.RELEASE" apply false
- id "org.ajoberstar.grgit" version "4.1.0" apply false
+ id "org.ajoberstar.grgit" version "4.1.1" apply false
Review comment:
Should this PR be bumping a dependency version?
--
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 #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r778450522
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -110,6 +111,47 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ MemberSet result = calculateInter(regionProvider, keys);
+ if (result == null) {
+ return Collections.emptySet();
+ }
+ return result;
+ }
+
+ private static MemberSet calculateInter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = new ArrayList<>(keys.size());
+ RedisSet smallestSet = null;
+
+ for (RedisKey key : keys) {
+ RedisSet redisSet = regionProvider.getTypedRedisData(REDIS_SET, key, true);
+ if (redisSet == NULL_REDIS_SET || redisSet.scard() == 0) {
+ return null;
+ } else {
+ if (smallestSet == null || smallestSet.scard() > redisSet.scard()) {
+ smallestSet = redisSet;
+ }
+ sets.add(redisSet);
+ }
+ }
+
+ MemberSet result = new MemberSet(smallestSet.scard());
+ for (byte[] member : smallestSet.members) {
+ boolean addToSet = true;
+ for (int i = 0; i < sets.size(); i++) {
+ RedisSet otherSet = sets.get(i);
+ if (otherSet.members.get(member) == null) {
Review comment:
this could be simplified to:
if (!otherSet.members.contains(member))
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -110,6 +111,47 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ MemberSet result = calculateInter(regionProvider, keys);
+ if (result == null) {
+ return Collections.emptySet();
+ }
+ return result;
+ }
+
+ private static MemberSet calculateInter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = new ArrayList<>(keys.size());
+ RedisSet smallestSet = null;
+
+ for (RedisKey key : keys) {
+ RedisSet redisSet = regionProvider.getTypedRedisData(REDIS_SET, key, true);
+ if (redisSet == NULL_REDIS_SET || redisSet.scard() == 0) {
+ return null;
+ } else {
+ if (smallestSet == null || smallestSet.scard() > redisSet.scard()) {
+ smallestSet = redisSet;
+ }
Review comment:
should you have an else here? I don't see any need to add "smallestSet" to "sets". The current code works but causes an extra set to be compared down in the next loop that compares each item in "sets" to "smallestSet".
When you find a new smallestSet you would to first add add the old smallestSet to "sets" since it is no longer smallest.
--
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] BalaKaza commented on a change in pull request #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
BalaKaza commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r783335084
##########
File path: build.gradle
##########
@@ -26,7 +26,7 @@ plugins {
id "nebula.lint" version "17.1.1" apply false
id "com.palantir.docker" version "0.28.0" apply false
id "io.spring.dependency-management" version "1.0.11.RELEASE" apply false
- id "org.ajoberstar.grgit" version "4.1.0" apply false
+ id "org.ajoberstar.grgit" version "4.1.1" apply false
Review comment:
I just did it to see if all tests pass. Anyways that failed still. Currently, the release team is looking into other alternative mirrors for this dependency. Will rebase once the fix is in.
--
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] nonbinaryprogrammer commented on a change in pull request #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r780466710
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -110,6 +111,42 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = new ArrayList<>(keys.size());
+ RedisSet smallestSet = NULL_REDIS_SET;
+
+ for (RedisKey key : keys) {
+ RedisSet redisSet = regionProvider.getTypedRedisData(REDIS_SET, key, true);
+ if (redisSet == NULL_REDIS_SET) {
+ return Collections.emptySet();
+ }
+ if (smallestSet == NULL_REDIS_SET) {
+ smallestSet = redisSet;
+ }
+ if (smallestSet.scard() > redisSet.scard()) {
+ sets.add(smallestSet);
+ smallestSet = redisSet;
+ } else {
+ sets.add(redisSet);
+ }
+ }
+
+ MemberSet result = new MemberSet(smallestSet.scard());
+ for (byte[] member : smallestSet.members) {
+ boolean addToSet = true;
+ for (RedisSet otherSet : sets) {
+ if (!otherSet.members.contains(member)) {
+ addToSet = false;
+ break;
+ }
+ }
+ if (addToSet) {
+ result.add(member);
+ }
Review comment:
not a big deal, but this can be simplified to
```
for(RedisSet otherSet : sets) {
if(otherSet.members.contains(member)) {
result.add(member);
} else {
break;
}
}
```
--
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] lgtm-com[bot] commented on pull request #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #7236:
URL: https://github.com/apache/geode/pull/7236#issuecomment-1005114365
This pull request **introduces 1 alert** when merging d3877e9af39998bd3722392d6d55a5d9dbbdd0e5 into 5e9c775229d9975793cb6ab3a9df794bc3a78d24 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-2a944887eddf5294d48b2530be59db09ed6f8ae0)
**new alerts:**
* 1 for Dereferenced variable may be null
--
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] BalaKaza commented on a change in pull request #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
BalaKaza commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r778990629
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -110,6 +111,47 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ MemberSet result = calculateInter(regionProvider, keys);
+ if (result == null) {
+ return Collections.emptySet();
+ }
+ return result;
+ }
+
+ private static MemberSet calculateInter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = new ArrayList<>(keys.size());
+ RedisSet smallestSet = null;
+
+ for (RedisKey key : keys) {
+ RedisSet redisSet = regionProvider.getTypedRedisData(REDIS_SET, key, true);
+ if (redisSet == NULL_REDIS_SET || redisSet.scard() == 0) {
+ return null;
Review comment:
Makes Sense. Will be returning `new MemberSet()` as that is the return type.
--
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] onichols-pivotal commented on a change in pull request #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r783360034
##########
File path: build.gradle
##########
@@ -26,7 +26,7 @@ plugins {
id "nebula.lint" version "17.1.1" apply false
id "com.palantir.docker" version "0.28.0" apply false
id "io.spring.dependency-management" version "1.0.11.RELEASE" apply false
- id "org.ajoberstar.grgit" version "4.1.0" apply false
+ id "org.ajoberstar.grgit" version "4.1.1" apply false
Review comment:
there is also already PR #7260 for this one-line change but it shouldn't conflict regardless of which is merged first or whether you rebase or not
--
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] nonbinaryprogrammer commented on a change in pull request #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r780466710
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -110,6 +111,42 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = new ArrayList<>(keys.size());
+ RedisSet smallestSet = NULL_REDIS_SET;
+
+ for (RedisKey key : keys) {
+ RedisSet redisSet = regionProvider.getTypedRedisData(REDIS_SET, key, true);
+ if (redisSet == NULL_REDIS_SET) {
+ return Collections.emptySet();
+ }
+ if (smallestSet == NULL_REDIS_SET) {
+ smallestSet = redisSet;
+ }
+ if (smallestSet.scard() > redisSet.scard()) {
+ sets.add(smallestSet);
+ smallestSet = redisSet;
+ } else {
+ sets.add(redisSet);
+ }
+ }
+
+ MemberSet result = new MemberSet(smallestSet.scard());
+ for (byte[] member : smallestSet.members) {
+ boolean addToSet = true;
+ for (RedisSet otherSet : sets) {
+ if (!otherSet.members.contains(member)) {
+ addToSet = false;
+ break;
+ }
+ }
+ if (addToSet) {
+ result.add(member);
+ }
Review comment:
not a big deal, but this can be simplified to
```
for(RedisSet otherSet : sets) {
if(otherSet.members.contains(member)) {
result.add(member);
} else {
break;
}
}
```
--
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] jdeppe-pivotal commented on a change in pull request #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r780537973
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -110,6 +111,42 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = new ArrayList<>(keys.size());
+ RedisSet smallestSet = NULL_REDIS_SET;
+
+ for (RedisKey key : keys) {
+ RedisSet redisSet = regionProvider.getTypedRedisData(REDIS_SET, key, true);
+ if (redisSet == NULL_REDIS_SET) {
+ return Collections.emptySet();
+ }
+ if (smallestSet == NULL_REDIS_SET) {
+ smallestSet = redisSet;
+ }
+ if (smallestSet.scard() > redisSet.scard()) {
+ sets.add(smallestSet);
+ smallestSet = redisSet;
+ } else {
+ sets.add(redisSet);
+ }
+ }
+
+ MemberSet result = new MemberSet(smallestSet.scard());
+ for (byte[] member : smallestSet.members) {
+ boolean addToSet = true;
+ for (RedisSet otherSet : sets) {
+ if (!otherSet.members.contains(member)) {
+ addToSet = false;
+ break;
+ }
+ }
+ if (addToSet) {
+ result.add(member);
+ }
Review comment:
The intent of this loop is to ensure that the member is in **all** the sets (intersection). The suggested change would add the member if it appears in even one (of multiple) 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] jdeppe-pivotal commented on a change in pull request #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r778403170
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSInterIntegrationTest.java
##########
@@ -60,9 +64,9 @@ public void sinterstoreErrors_givenTooFewArguments() {
}
@Test
- public void testSInter() {
- String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
- String[] secondSet = new String[] {"apple", "microsoft", "linux", "peach"};
+ public void testSInter_givenIntersection_returnsIntersectedMembers() {
+ String[] firstSet = new String[] {"peach"};
+ String[] secondSet = new String[] {"linux", "peach"};
Review comment:
Please could you restore at least the `"apple"` entry so that there is a partial intersection.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -110,6 +111,47 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ MemberSet result = calculateInter(regionProvider, keys);
+ if (result == null) {
+ return Collections.emptySet();
+ }
+ return result;
+ }
+
+ private static MemberSet calculateInter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = new ArrayList<>(keys.size());
+ RedisSet smallestSet = null;
+
+ for (RedisKey key : keys) {
+ RedisSet redisSet = regionProvider.getTypedRedisData(REDIS_SET, key, true);
+ if (redisSet == NULL_REDIS_SET || redisSet.scard() == 0) {
+ return null;
+ } else {
+ if (smallestSet == null || smallestSet.scard() > redisSet.scard()) {
+ smallestSet = redisSet;
+ }
+ sets.add(redisSet);
+ }
+ }
+
+ MemberSet result = new MemberSet(smallestSet.scard());
+ for (byte[] member : smallestSet.members) {
+ boolean addToSet = true;
+ for (int i = 0; i < sets.size(); i++) {
+ RedisSet otherSet = sets.get(i);
Review comment:
This could be simplified to:
```
for (RedisSet otherSet : 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] ringles merged pull request #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
ringles merged pull request #7236:
URL: https://github.com/apache/geode/pull/7236
--
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 #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r778479474
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -110,6 +111,47 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ MemberSet result = calculateInter(regionProvider, keys);
+ if (result == null) {
+ return Collections.emptySet();
+ }
+ return result;
+ }
+
+ private static MemberSet calculateInter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = new ArrayList<>(keys.size());
+ RedisSet smallestSet = null;
+
+ for (RedisKey key : keys) {
+ RedisSet redisSet = regionProvider.getTypedRedisData(REDIS_SET, key, true);
+ if (redisSet == NULL_REDIS_SET || redisSet.scard() == 0) {
+ return null;
Review comment:
Rather than returning null here, it would be better to return `Collections.emptySet()`. This would then make the `sinter()` method just
```
return calculateInter(regionProvider, keys);
```
making the `calculateInter()` method redundant and allowing it to be inlined.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -110,6 +111,47 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ MemberSet result = calculateInter(regionProvider, keys);
+ if (result == null) {
+ return Collections.emptySet();
+ }
+ return result;
+ }
+
+ private static MemberSet calculateInter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = new ArrayList<>(keys.size());
+ RedisSet smallestSet = null;
+
+ for (RedisKey key : keys) {
+ RedisSet redisSet = regionProvider.getTypedRedisData(REDIS_SET, key, true);
+ if (redisSet == NULL_REDIS_SET || redisSet.scard() == 0) {
+ return null;
+ } else {
+ if (smallestSet == null || smallestSet.scard() > redisSet.scard()) {
+ smallestSet = redisSet;
+ }
+ sets.add(redisSet);
+ }
+ }
+
+ MemberSet result = new MemberSet(smallestSet.scard());
Review comment:
The LGTM warning introduced here can be fixed by not initializing `smallestSet` as null but rather as `NULL_REDIS_SET`. The null check on line 131 would then also need to be changed to compare to `NULL_REDIS_SET` rather than null.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSInterIntegrationTest.java
##########
@@ -72,14 +76,78 @@ public void testSInter() {
String[] expected = new String[] {"peach"};
assertThat(resultSet).containsExactlyInAnyOrder(expected);
+ }
+
+ @Test
+ public void testSInter_givenNonSet_returnsErrorWrongType() {
+ String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ String nonSet = "apple";
+ jedis.sadd("{user1}set1", firstSet);
+ jedis.set("{user1}nonSet", nonSet);
+
+ assertThatThrownBy(() -> jedis.sinter("{user1}set1", "{user1}nonSet")).hasMessageContaining(
+ ERROR_WRONG_TYPE);
+ }
- Set<String> emptyResultSet = jedis.sinter("{user1}nonexistent", "{user1}set2", "{user1}set3");
+ @Test
+ public void testSInter_givenNoIntersection_returnsEmptySet() {
+ String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ String[] secondSet = new String[] {"ubuntu", "microsoft", "linux", "solaris"};
+ jedis.sadd("{user1}set1", firstSet);
+ jedis.sadd("{user1}set2", secondSet);
+
+ Set<String> emptyResultSet = jedis.sinter("{user1}set1", "{user1}set2");
assertThat(emptyResultSet).isEmpty();
+ }
- jedis.sadd("{user1}newEmpty", "born2die");
- jedis.srem("{user1}newEmpty", "born2die");
- Set<String> otherEmptyResultSet = jedis.sinter("{user1}set2", "{user1}newEmpty");
- assertThat(otherEmptyResultSet).isEmpty();
+ @Test
+ public void testSInter_givenSingleSet_returnsAllMembers() {
+ String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ jedis.sadd("{user1}set1", firstSet);
+
+ Set<String> resultSet = jedis.sinter("{user1}set1");
+ assertThat(resultSet).containsExactlyInAnyOrder(firstSet);
+ }
+
+ @Test
+ public void testSInter_givenFullyMatchingSet_returnsAllMembers() {
+ String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ String[] secondSet = new String[] {"apple", "pear", "plum", "peach", "orange",};
+ jedis.sadd("{user1}set1", firstSet);
+ jedis.sadd("{user1}set2", secondSet);
+
+ Set<String> resultSet = jedis.sinter("{user1}set1", "{user1}set2");
+ assertThat(resultSet).containsExactlyInAnyOrder(firstSet);
+ }
+
+ @Test
+ public void testSInter_givenNonExistentSingleSet_returnsEmptySet() {
+ Set<String> emptyResultSet = jedis.sinter("{user1}set1");
+ assertThat(emptyResultSet).isEmpty();
+ }
+
+ @Test
+ public void ensureSetConsistency_whenRunningConcurrently() {
+ String[] values = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ Set<String> valuesList = new HashSet<>(Arrays.asList(values));
+ String[] newValues = new String[] {"ubuntu", "orange", "peach", "linux"};
+ jedis.sadd("{user1}firstset", values);
+ jedis.sadd("{user1}secondset", values);
+
+ final AtomicReference<Set<String>> sinterResultReference = new AtomicReference<>();
+ new ConcurrentLoopingThreads(1000,
+ i -> jedis.sadd("{user1}thirdset", newValues),
+ i -> sinterResultReference.set(
+ jedis.sinter("{user1}firstset", "{user1}secondset", "{user1}thirdset")))
+ .runWithAction(() -> {
+ String[] result = new String[] {"orange", "peach"};
+ Set<String> expectedResultSet = new HashSet<>(Arrays.asList(result));
Review comment:
These lines can be moved to outside of the `ConcurrentLoopingThreads`, as they're constant and don't need to be recreated every iteration. Also, it's not necessary to create a HashSet here, you can instead just assert:
```
sInterResult -> assertThat(sInterResult.get()).containsExactlyInAnyOrder(result));
```
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -110,6 +111,47 @@ private static MemberSet calculateDiff(RegionProvider regionProvider, List<Redis
return diff;
}
+ public static Set<byte[]> sinter(RegionProvider regionProvider, List<RedisKey> keys) {
+ MemberSet result = calculateInter(regionProvider, keys);
+ if (result == null) {
+ return Collections.emptySet();
+ }
+ return result;
+ }
+
+ private static MemberSet calculateInter(RegionProvider regionProvider, List<RedisKey> keys) {
+ List<RedisSet> sets = new ArrayList<>(keys.size());
+ RedisSet smallestSet = null;
+
+ for (RedisKey key : keys) {
+ RedisSet redisSet = regionProvider.getTypedRedisData(REDIS_SET, key, true);
+ if (redisSet == NULL_REDIS_SET || redisSet.scard() == 0) {
Review comment:
There is a redundant check here, as it's not possible for a RedisSet to be empty without also being a `NULL_REDIS_SET`.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSInterIntegrationTest.java
##########
@@ -72,14 +76,78 @@ public void testSInter() {
String[] expected = new String[] {"peach"};
assertThat(resultSet).containsExactlyInAnyOrder(expected);
+ }
+
+ @Test
+ public void testSInter_givenNonSet_returnsErrorWrongType() {
+ String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ String nonSet = "apple";
+ jedis.sadd("{user1}set1", firstSet);
+ jedis.set("{user1}nonSet", nonSet);
+
+ assertThatThrownBy(() -> jedis.sinter("{user1}set1", "{user1}nonSet")).hasMessageContaining(
+ ERROR_WRONG_TYPE);
+ }
- Set<String> emptyResultSet = jedis.sinter("{user1}nonexistent", "{user1}set2", "{user1}set3");
+ @Test
+ public void testSInter_givenNoIntersection_returnsEmptySet() {
+ String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ String[] secondSet = new String[] {"ubuntu", "microsoft", "linux", "solaris"};
+ jedis.sadd("{user1}set1", firstSet);
+ jedis.sadd("{user1}set2", secondSet);
+
+ Set<String> emptyResultSet = jedis.sinter("{user1}set1", "{user1}set2");
assertThat(emptyResultSet).isEmpty();
+ }
- jedis.sadd("{user1}newEmpty", "born2die");
- jedis.srem("{user1}newEmpty", "born2die");
- Set<String> otherEmptyResultSet = jedis.sinter("{user1}set2", "{user1}newEmpty");
- assertThat(otherEmptyResultSet).isEmpty();
+ @Test
+ public void testSInter_givenSingleSet_returnsAllMembers() {
+ String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ jedis.sadd("{user1}set1", firstSet);
+
+ Set<String> resultSet = jedis.sinter("{user1}set1");
+ assertThat(resultSet).containsExactlyInAnyOrder(firstSet);
+ }
+
+ @Test
+ public void testSInter_givenFullyMatchingSet_returnsAllMembers() {
+ String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ String[] secondSet = new String[] {"apple", "pear", "plum", "peach", "orange",};
+ jedis.sadd("{user1}set1", firstSet);
+ jedis.sadd("{user1}set2", secondSet);
+
+ Set<String> resultSet = jedis.sinter("{user1}set1", "{user1}set2");
+ assertThat(resultSet).containsExactlyInAnyOrder(firstSet);
+ }
+
+ @Test
+ public void testSInter_givenNonExistentSingleSet_returnsEmptySet() {
+ Set<String> emptyResultSet = jedis.sinter("{user1}set1");
+ assertThat(emptyResultSet).isEmpty();
+ }
+
+ @Test
+ public void ensureSetConsistency_whenRunningConcurrently() {
+ String[] values = new String[] {"pear", "apple", "plum", "orange", "peach"};
+ Set<String> valuesList = new HashSet<>(Arrays.asList(values));
Review comment:
This variable is never used.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSInterIntegrationTest.java
##########
@@ -60,9 +64,9 @@ public void sinterstoreErrors_givenTooFewArguments() {
}
@Test
- public void testSInter() {
- String[] firstSet = new String[] {"pear", "apple", "plum", "orange", "peach"};
- String[] secondSet = new String[] {"apple", "microsoft", "linux", "peach"};
+ public void testSInter_givenIntersection_returnsIntersectedMembers() {
+ String[] firstSet = new String[] {"peach"};
+ String[] secondSet = new String[] {"linux", "peach"};
String[] thirdSet = new String[] {"luigi", "bowser", "peach", "mario"};
jedis.sadd("{user1}set1", firstSet);
Review comment:
The strings `"{user1}set1"` and `"{user1}set2" are used a lot in this class. Could they be extracted to constants please?
--
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] BalaKaza commented on a change in pull request #7236: GEODE-9829: Add SINTER command to Redis supported commands.
Posted by GitBox <gi...@apache.org>.
BalaKaza commented on a change in pull request #7236:
URL: https://github.com/apache/geode/pull/7236#discussion_r779160561
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSInterIntegrationTest.java
##########
@@ -41,6 +40,9 @@
private JedisCluster jedis;
private static final int REDIS_CLIENT_TIMEOUT =
Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+ private static final String REDIS_USER_SET1 = "{user1}set1";
+ private static final String REDIS_USER_SET2 = "{user1}set2";
+ private static final String REDIS_USER_SET3 = "{user1}set3";
Review comment:
Thanks for the explanation. Changed the constants.
--
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