dschneider-pivotal commented on a change in pull request #7319:
URL: https://github.com/apache/geode/pull/7319#discussion_r794782741
##########
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<>();
Review comment:
instead of creating it initially empty I think you should just declare
it here and initialize it after the "if" and after the "else".
After the "if" do this: `result = new ArrayList<>(members.size());`
Afther the "else do this: `result = new ArrayList<>(count);`
##########
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:
What if we changed our getFromBackingArray to not return null? If the
initial index given finds a null it could just inc it until it finds a non-null
or hits the end of the array. If it hits the end it could start at 0 and
continue the linear scan. As long as we start at the random index given I think
we will be honoring the randomness spop desires and since we remove the thing
we find we will not keep returning the same member. Also maybe this should be
"removeFromBackingArray(int)" instead of get. That way you don't need to turn
around and call "remove(byte[])" which has to search for it.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SRandMemberExecutor.java
##########
@@ -14,13 +14,31 @@
*/
package org.apache.geode.redis.internal.commands.executor.set;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NOT_INTEGER;
+import static
org.apache.geode.redis.internal.data.NullRedisDataStructures.NULL_REDIS_SET;
+import static org.apache.geode.redis.internal.data.RedisDataType.REDIS_SET;
+
+import java.util.Collections;
import java.util.List;
+import org.apache.geode.redis.internal.data.RedisKey;
import org.apache.geode.redis.internal.data.RedisSet;
+import org.apache.geode.redis.internal.services.RegionProvider;
public class SRandMemberExecutor extends SetRandomExecutor {
@Override
- protected List<byte[]> performCommand(RedisSet set, int count) {
+ protected List<byte[]> performCommand(int count, RegionProvider
regionProvider, RedisKey key) {
+ RedisSet set =
+ regionProvider.getTypedRedisData(REDIS_SET, key, true);
+ if (count == 0 || set == NULL_REDIS_SET) {
Review comment:
set.isNull()
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SPopExecutor.java
##########
@@ -16,52 +16,36 @@
import static
org.apache.geode.redis.internal.RedisConstants.ERROR_VALUE_MUST_BE_POSITIVE;
-import static org.apache.geode.redis.internal.netty.Coder.bytesToLong;
-import static org.apache.geode.redis.internal.netty.Coder.narrowLongToInt;
+import static
org.apache.geode.redis.internal.data.NullRedisDataStructures.NULL_REDIS_SET;
+import static org.apache.geode.redis.internal.data.RedisDataType.REDIS_SET;
-import java.util.Collection;
+import java.util.Collections;
import java.util.List;
-import org.apache.geode.cache.Region;
-import org.apache.geode.redis.internal.commands.Command;
-import org.apache.geode.redis.internal.commands.executor.CommandExecutor;
-import org.apache.geode.redis.internal.commands.executor.RedisResponse;
-import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.RedisException;
import org.apache.geode.redis.internal.data.RedisKey;
-import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+import org.apache.geode.redis.internal.data.RedisSet;
+import org.apache.geode.redis.internal.services.RegionProvider;
-public class SPopExecutor implements CommandExecutor {
+public class SPopExecutor extends SetRandomExecutor {
@Override
- public RedisResponse executeCommand(Command command, ExecutionHandlerContext
context) {
- List<byte[]> commandElems = command.getProcessedCommand();
- boolean isCountPassed = false;
- int popCount;
-
- if (commandElems.size() == 3) {
- isCountPassed = true;
- try {
- popCount = narrowLongToInt(bytesToLong(commandElems.get(2)));
- } catch (NumberFormatException nex) {
- return RedisResponse.error(ERROR_VALUE_MUST_BE_POSITIVE);
- }
- } else {
- popCount = 1;
+ protected List<byte[]> performCommand(int count, RegionProvider
regionProvider, RedisKey key) {
+ if (count < 0) {
+ throw new RedisException(getError());
}
- Region<RedisKey, RedisData> region = context.getRegion();
- RedisKey key = command.getKey();
- Collection<byte[]> popped = context.setLockedExecute(key, false,
- set -> set.spop(region, key, popCount));
-
- if (popped.isEmpty() && !isCountPassed) {
- return RedisResponse.nil();
+ RedisSet set =
+ regionProvider.getTypedRedisData(REDIS_SET, key, false);
+ if (count == 0 || set == NULL_REDIS_SET) {
Review comment:
consider using `set.isNull()` instead of `set == NULL_REDIS_SET`
##########
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:
As you call "members.remove()" will that cause the backingArrayLength to
eventually shrink?
##########
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);
Review comment:
Instead of using "addAll" I think it would be better to iterate
"members" adding each one to result by calling add. The reason is that both
addAll and the ArrayList(Collection) constructor end up calling toArray on
members. So this allocates an array containing all the members. But then
ArrayList has to copy this array into the array it owns. So we end up copying
the data twice and producing a large garbage array.
--
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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
