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/28 19:26:09 UTC
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7319: GEODE-9833: SPOP Command Support
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-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org