[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #7319: GEODE-9833: SPOP Command Support

[GitBox <gi...@apache.org>]

jdeppe-pivotal commented on a change in pull request #7319:
URL: https://github.com/apache/geode/pull/7319#discussion_r794520008



##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -245,33 +244,25 @@ static int setOpStoreResult(RegionProvider regionProvider, RedisKey destinationK
     return scanResult;
   }
 
-  public Collection<byte[]> spop(Region<RedisKey, RedisData> region, RedisKey key, int popCount) {
-    int originalSize = scard();
-    if (originalSize == 0) {
-      return emptyList();
-    }
-
-    if (popCount >= originalSize) {
-      region.remove(key, this);
-      return members;
-    }
-
-    List<byte[]> popped = new ArrayList<>();
-    byte[][] setMembers = members.toArray(new byte[originalSize][]);
-    Random rand = new Random();
-    while (popped.size() < popCount) {
-      int idx = rand.nextInt(originalSize);
-      byte[] memberToPop = setMembers[idx];
-      if (memberToPop != null) {
-        setMembers[idx] = null;
-        popped.add(memberToPop);
-        membersRemove(memberToPop);
+  public List<byte[]> spop(int count, Region<RedisKey, RedisData> region, RedisKey key) {
+    List<byte[]> result = new ArrayList<>();
+    if (members.size() <= count) {
+      result.addAll(members);
+      members.clear();
+    } else {
+      Random rand = new Random();
+      while (result.size() != count) {
+        int randIndex = rand.nextInt(members.getBackingArrayLength());
+        byte[] member = members.getFromBackingArray(randIndex);
+        if (member != null) {

Review comment:
       Under what circumstances could the returned `member` be null? If it's really possible then there's also the possibility that this while loop could be stuck here infinitely. I think the condition should be removed.




-- 
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