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/27 18:47:47 UTC
[GitHub] [geode] Kris-10-0 opened a new pull request #7319: GEODE-9833: SPOP Command Support
Kris-10-0 opened a new pull request #7319:
URL: https://github.com/apache/geode/pull/7319
<!-- 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:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [x] Is your initial contribution a single, squashed commit?
- [x] Does `gradlew build` run cleanly?
- [x] Have you written or updated unit tests to verify your changes?
- [NA] 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] jdeppe-pivotal commented on a change in pull request #7319: GEODE-9833: SPOP Command Support
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #7319:
URL: https://github.com/apache/geode/pull/7319#discussion_r796139281
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -245,45 +244,61 @@ 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();
+ public List<byte[]> spop(int count, Region<RedisKey, RedisData> region, RedisKey key) {
+ int randMethodRatio = 3;
Review comment:
Can this just be a static constant? Please also add some comments as to what the reasoning is.
I'm somewhat hesitant to add optimizations like this without any quantifying metrics to justify; since, on the face of it, it just ends up complicating the code - especially for someone who may look at it in the future without any context. This is just a general comment and not a request to change it... :)
--
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 #7319: GEODE-9833: SPOP Command Support
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [geode] Kris-10-0 commented on a change in pull request #7319: GEODE-9833: SPOP Command Support
Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on a change in pull request #7319:
URL: https://github.com/apache/geode/pull/7319#discussion_r794905233
##########
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 I call members.remove() it will cause the backingArrayLength to shrink, and might possibly shrink multiple times if the count is large enough.
--
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 #7319: GEODE-9833: SPOP Command Support
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7319:
URL: https://github.com/apache/geode/pull/7319#discussion_r794100459
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -300,7 +283,7 @@ private void srandomDuplicateList(int count, List<byte[]> result) {
}
}
- private void srandomUniqueListWithSmallCount(int count, List<byte[]> result) {
+ private void srandomUniqueListAddToResult(int count, List<byte[]> result, boolean isSPop) {
Review comment:
Ah, true. The behaviour would be correct with SPOP, but as you say, we're creating a HashSet that we don't need to here.
--
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 #7319: GEODE-9833: SPOP Command Support
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7319:
URL: https://github.com/apache/geode/pull/7319#discussion_r794122915
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSPopIntegrationTest.java
##########
@@ -52,213 +56,137 @@ public void tearDown() {
}
@Test
- public void givenKeyNotProvided_returnsWrongNumberOfArgumentsError() {
- assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SPOP))
- .hasMessageContaining("ERR wrong number of arguments for 'spop' command");
+ public void spopTooFewArgs_returnsError() {
+ assertAtLeastNArgs(jedis, Protocol.Command.SPOP, 1);
}
@Test
- public void givenMoreThanThreeArguments_returnsSyntaxError() {
+ public void spopTooManyArgs_returnsError() {
assertThatThrownBy(
- () -> jedis.sendCommand("key", Protocol.Command.SPOP, "key", "NaN", "extraArg"))
+ () -> jedis.sendCommand(SET_KEY, Protocol.Command.SPOP, SET_KEY, "5", "5"))
.hasMessageContaining(ERROR_SYNTAX);
}
@Test
- public void givenCountIsNotAnInteger_returnsNotIntegerError() {
- assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SPOP, "key", "NaN"))
+ public void spop_withNonNumericCount_returnsError() {
+ assertThatThrownBy(() -> jedis.sendCommand(SET_KEY, Protocol.Command.SPOP, SET_KEY, "b"))
.hasMessageContaining(ERROR_VALUE_MUST_BE_POSITIVE);
}
@Test
- public void testSPop() {
- int ENTRIES = 10;
-
- List<String> masterSet = new ArrayList<>();
- for (int i = 0; i < ENTRIES; i++) {
- masterSet.add("master-" + i);
- }
-
- jedis.sadd("master", masterSet.toArray(new String[] {}));
- String poppy = jedis.spop("master");
-
- masterSet.remove(poppy);
- assertThat(jedis.smembers("master").toArray()).containsExactlyInAnyOrder(masterSet.toArray());
-
- assertThat(jedis.spop("spopnonexistent")).isNull();
+ public void spop_withNegativeCount_returnsError() {
+ assertThatThrownBy(() -> jedis.spop(SET_KEY, -1))
+ .hasMessageContaining(ERROR_VALUE_MUST_BE_POSITIVE);
}
@Test
- public void testSPopAll() {
- int ENTRIES = 10;
-
- List<String> masterSet = new ArrayList<>();
- for (int i = 0; i < ENTRIES; i++) {
- masterSet.add("master-" + i);
- }
-
- jedis.sadd("master", masterSet.toArray(new String[] {}));
- Set<String> popped = jedis.spop("master", ENTRIES);
-
- assertThat(jedis.smembers("master").toArray()).isEmpty();
- assertThat(popped.toArray()).containsExactlyInAnyOrder(masterSet.toArray());
+ public void spop_withoutCount_withNonExistentSet_returnsNull_setNotCreated() {
+ assertThat(jedis.spop(NON_EXISTENT_SET_KEY)).isNull();
+ assertThat(jedis.exists(NON_EXISTENT_SET_KEY)).isFalse();
}
@Test
- public void testSPopAllPlusOne() {
- int ENTRIES = 10;
-
- List<String> masterSet = new ArrayList<>();
- for (int i = 0; i < ENTRIES; i++) {
- masterSet.add("master-" + i);
- }
-
- jedis.sadd("master", masterSet.toArray(new String[] {}));
- Set<String> popped = jedis.spop("master", ENTRIES + 1);
+ public void spop_withoutCount_withExistentSet_returnsOneMember_removesReturnedMemberFromSet() {
+ jedis.sadd(SET_KEY, SET_MEMBERS);
- assertThat(jedis.smembers("master").toArray()).isEmpty();
- assertThat(popped.toArray()).containsExactlyInAnyOrder(masterSet.toArray());
+ String result = jedis.spop(SET_KEY);
+ assertThat(result).isIn(Arrays.asList(SET_MEMBERS));
+ assertThat(jedis.smembers(SET_KEY)).doesNotContain(result).isSubsetOf(SET_MEMBERS)
+ .doesNotHaveDuplicates();
Review comment:
Yep, you're exactly right!
--
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] Kris-10-0 commented on a change in pull request #7319: GEODE-9833: SPOP Command Support
Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on a change in pull request #7319:
URL: https://github.com/apache/geode/pull/7319#discussion_r794918547
##########
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:
The method it uses to remove an element at the specified index is private, so it would need to be reimplemented, but I am not sure if there is some legal issues with doing that.
--
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] Kris-10-0 commented on a change in pull request #7319: GEODE-9833: SPOP Command Support
Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on a change in pull request #7319:
URL: https://github.com/apache/geode/pull/7319#discussion_r794918547
##########
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:
The method it uses to remove an element at the specified index is private, so it would need to be reimplemented, but I am not sure if there is some legal issues with doing that.
Also typically wouldn't remove(byte[]) on average be O(1), since a set is using hashing mapping the member to an index.
##########
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:
The method it uses to remove an element at the specified index is private, so it would need to be reimplemented, but I am not sure if there is some legal issues with doing that.
Also typically wouldn't remove(byte[]) on average be O(1), since a set is using hashing mapping the member to an index?
--
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] Kris-10-0 commented on a change in pull request #7319: GEODE-9833: SPOP Command Support
Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on a change in pull request #7319:
URL: https://github.com/apache/geode/pull/7319#discussion_r794119136
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSPopIntegrationTest.java
##########
@@ -52,213 +56,137 @@ public void tearDown() {
}
@Test
- public void givenKeyNotProvided_returnsWrongNumberOfArgumentsError() {
- assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SPOP))
- .hasMessageContaining("ERR wrong number of arguments for 'spop' command");
+ public void spopTooFewArgs_returnsError() {
+ assertAtLeastNArgs(jedis, Protocol.Command.SPOP, 1);
}
@Test
- public void givenMoreThanThreeArguments_returnsSyntaxError() {
+ public void spopTooManyArgs_returnsError() {
assertThatThrownBy(
- () -> jedis.sendCommand("key", Protocol.Command.SPOP, "key", "NaN", "extraArg"))
+ () -> jedis.sendCommand(SET_KEY, Protocol.Command.SPOP, SET_KEY, "5", "5"))
.hasMessageContaining(ERROR_SYNTAX);
}
@Test
- public void givenCountIsNotAnInteger_returnsNotIntegerError() {
- assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SPOP, "key", "NaN"))
+ public void spop_withNonNumericCount_returnsError() {
+ assertThatThrownBy(() -> jedis.sendCommand(SET_KEY, Protocol.Command.SPOP, SET_KEY, "b"))
.hasMessageContaining(ERROR_VALUE_MUST_BE_POSITIVE);
}
@Test
- public void testSPop() {
- int ENTRIES = 10;
-
- List<String> masterSet = new ArrayList<>();
- for (int i = 0; i < ENTRIES; i++) {
- masterSet.add("master-" + i);
- }
-
- jedis.sadd("master", masterSet.toArray(new String[] {}));
- String poppy = jedis.spop("master");
-
- masterSet.remove(poppy);
- assertThat(jedis.smembers("master").toArray()).containsExactlyInAnyOrder(masterSet.toArray());
-
- assertThat(jedis.spop("spopnonexistent")).isNull();
+ public void spop_withNegativeCount_returnsError() {
+ assertThatThrownBy(() -> jedis.spop(SET_KEY, -1))
+ .hasMessageContaining(ERROR_VALUE_MUST_BE_POSITIVE);
}
@Test
- public void testSPopAll() {
- int ENTRIES = 10;
-
- List<String> masterSet = new ArrayList<>();
- for (int i = 0; i < ENTRIES; i++) {
- masterSet.add("master-" + i);
- }
-
- jedis.sadd("master", masterSet.toArray(new String[] {}));
- Set<String> popped = jedis.spop("master", ENTRIES);
-
- assertThat(jedis.smembers("master").toArray()).isEmpty();
- assertThat(popped.toArray()).containsExactlyInAnyOrder(masterSet.toArray());
+ public void spop_withoutCount_withNonExistentSet_returnsNull_setNotCreated() {
+ assertThat(jedis.spop(NON_EXISTENT_SET_KEY)).isNull();
+ assertThat(jedis.exists(NON_EXISTENT_SET_KEY)).isFalse();
}
@Test
- public void testSPopAllPlusOne() {
- int ENTRIES = 10;
-
- List<String> masterSet = new ArrayList<>();
- for (int i = 0; i < ENTRIES; i++) {
- masterSet.add("master-" + i);
- }
-
- jedis.sadd("master", masterSet.toArray(new String[] {}));
- Set<String> popped = jedis.spop("master", ENTRIES + 1);
+ public void spop_withoutCount_withExistentSet_returnsOneMember_removesReturnedMemberFromSet() {
+ jedis.sadd(SET_KEY, SET_MEMBERS);
- assertThat(jedis.smembers("master").toArray()).isEmpty();
- assertThat(popped.toArray()).containsExactlyInAnyOrder(masterSet.toArray());
+ String result = jedis.spop(SET_KEY);
+ assertThat(result).isIn(Arrays.asList(SET_MEMBERS));
+ assertThat(jedis.smembers(SET_KEY)).doesNotContain(result).isSubsetOf(SET_MEMBERS)
+ .doesNotHaveDuplicates();
Review comment:
If the result is removed from the setMembersList, would the doesNotContain(result) be a necessary check, since containsExactlyInAnyOrderElementsOf have to have the same exact members in any oder.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSPopIntegrationTest.java
##########
@@ -52,213 +56,137 @@ public void tearDown() {
}
@Test
- public void givenKeyNotProvided_returnsWrongNumberOfArgumentsError() {
- assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SPOP))
- .hasMessageContaining("ERR wrong number of arguments for 'spop' command");
+ public void spopTooFewArgs_returnsError() {
+ assertAtLeastNArgs(jedis, Protocol.Command.SPOP, 1);
}
@Test
- public void givenMoreThanThreeArguments_returnsSyntaxError() {
+ public void spopTooManyArgs_returnsError() {
assertThatThrownBy(
- () -> jedis.sendCommand("key", Protocol.Command.SPOP, "key", "NaN", "extraArg"))
+ () -> jedis.sendCommand(SET_KEY, Protocol.Command.SPOP, SET_KEY, "5", "5"))
.hasMessageContaining(ERROR_SYNTAX);
}
@Test
- public void givenCountIsNotAnInteger_returnsNotIntegerError() {
- assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SPOP, "key", "NaN"))
+ public void spop_withNonNumericCount_returnsError() {
+ assertThatThrownBy(() -> jedis.sendCommand(SET_KEY, Protocol.Command.SPOP, SET_KEY, "b"))
.hasMessageContaining(ERROR_VALUE_MUST_BE_POSITIVE);
}
@Test
- public void testSPop() {
- int ENTRIES = 10;
-
- List<String> masterSet = new ArrayList<>();
- for (int i = 0; i < ENTRIES; i++) {
- masterSet.add("master-" + i);
- }
-
- jedis.sadd("master", masterSet.toArray(new String[] {}));
- String poppy = jedis.spop("master");
-
- masterSet.remove(poppy);
- assertThat(jedis.smembers("master").toArray()).containsExactlyInAnyOrder(masterSet.toArray());
-
- assertThat(jedis.spop("spopnonexistent")).isNull();
+ public void spop_withNegativeCount_returnsError() {
+ assertThatThrownBy(() -> jedis.spop(SET_KEY, -1))
+ .hasMessageContaining(ERROR_VALUE_MUST_BE_POSITIVE);
}
@Test
- public void testSPopAll() {
- int ENTRIES = 10;
-
- List<String> masterSet = new ArrayList<>();
- for (int i = 0; i < ENTRIES; i++) {
- masterSet.add("master-" + i);
- }
-
- jedis.sadd("master", masterSet.toArray(new String[] {}));
- Set<String> popped = jedis.spop("master", ENTRIES);
-
- assertThat(jedis.smembers("master").toArray()).isEmpty();
- assertThat(popped.toArray()).containsExactlyInAnyOrder(masterSet.toArray());
+ public void spop_withoutCount_withNonExistentSet_returnsNull_setNotCreated() {
+ assertThat(jedis.spop(NON_EXISTENT_SET_KEY)).isNull();
+ assertThat(jedis.exists(NON_EXISTENT_SET_KEY)).isFalse();
}
@Test
- public void testSPopAllPlusOne() {
- int ENTRIES = 10;
-
- List<String> masterSet = new ArrayList<>();
- for (int i = 0; i < ENTRIES; i++) {
- masterSet.add("master-" + i);
- }
-
- jedis.sadd("master", masterSet.toArray(new String[] {}));
- Set<String> popped = jedis.spop("master", ENTRIES + 1);
+ public void spop_withoutCount_withExistentSet_returnsOneMember_removesReturnedMemberFromSet() {
+ jedis.sadd(SET_KEY, SET_MEMBERS);
- assertThat(jedis.smembers("master").toArray()).isEmpty();
- assertThat(popped.toArray()).containsExactlyInAnyOrder(masterSet.toArray());
+ String result = jedis.spop(SET_KEY);
+ assertThat(result).isIn(Arrays.asList(SET_MEMBERS));
+ assertThat(jedis.smembers(SET_KEY)).doesNotContain(result).isSubsetOf(SET_MEMBERS)
+ .doesNotHaveDuplicates();
Review comment:
If the result is removed from the setMembersList, would the doesNotContain(result) be a necessary check, since containsExactlyInAnyOrderElementsOf have to have the same exact members in any oder?
--
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] Kris-10-0 commented on a change in pull request #7319: GEODE-9833: SPOP Command Support
Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on a change in pull request #7319:
URL: https://github.com/apache/geode/pull/7319#discussion_r794763148
##########
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:
The load factor is 3/4, so I believe it would be null 1/4 of the time. I used this method to get set members from the backing array, because of Darrel's comment on not copying over the member set. I do think this check is necessary for the implementation I have now, but I could change the way I am doing it if it can be stuck here infinitely.
I could be wrong, but I don't think it would be stuck in the loop infinitely. In SPOPEXECUTOR there is a check to make sure that the set size is greater than 0, and in spop there is a check before this that makes sure the count is less than the size of the set. So when entering this loop there would be some members in the set, and it would remove an amount that is less than the total size of the set. I guess it could loop infinitely if spop is called outside SPOPEXECUTOR, but I could move the 0 size check in this method.
--
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] Kris-10-0 commented on a change in pull request #7319: GEODE-9833: SPOP Command Support
Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on a change in pull request #7319:
URL: https://github.com/apache/geode/pull/7319#discussion_r794100445
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSPopIntegrationTest.java
##########
@@ -52,213 +56,137 @@ public void tearDown() {
}
@Test
- public void givenKeyNotProvided_returnsWrongNumberOfArgumentsError() {
- assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SPOP))
- .hasMessageContaining("ERR wrong number of arguments for 'spop' command");
+ public void spopTooFewArgs_returnsError() {
+ assertAtLeastNArgs(jedis, Protocol.Command.SPOP, 1);
}
@Test
- public void givenMoreThanThreeArguments_returnsSyntaxError() {
+ public void spopTooManyArgs_returnsError() {
assertThatThrownBy(
- () -> jedis.sendCommand("key", Protocol.Command.SPOP, "key", "NaN", "extraArg"))
+ () -> jedis.sendCommand(SET_KEY, Protocol.Command.SPOP, SET_KEY, "5", "5"))
.hasMessageContaining(ERROR_SYNTAX);
Review comment:
By just calling that method or assertThatThrownBy(() ->RedisCommandArgumentsTestHelper.assertAtMostNArgs()).hasMessageContaining(ERROR_SYNTAX);
When I was using RedisCommandArgumentsTestHelper.assertAtMostNArgs() on it's own it failed both the native and integration test.
--
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 merged pull request #7319: GEODE-9833: SPOP Command Support
Posted by GitBox <gi...@apache.org>.
DonalEvans merged pull request #7319:
URL: https://github.com/apache/geode/pull/7319
--
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 #7319: GEODE-9833: SPOP Command Support
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7319:
URL: https://github.com/apache/geode/pull/7319#discussion_r794121541
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSPopIntegrationTest.java
##########
@@ -52,213 +56,137 @@ public void tearDown() {
}
@Test
- public void givenKeyNotProvided_returnsWrongNumberOfArgumentsError() {
- assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SPOP))
- .hasMessageContaining("ERR wrong number of arguments for 'spop' command");
+ public void spopTooFewArgs_returnsError() {
+ assertAtLeastNArgs(jedis, Protocol.Command.SPOP, 1);
}
@Test
- public void givenMoreThanThreeArguments_returnsSyntaxError() {
+ public void spopTooManyArgs_returnsError() {
assertThatThrownBy(
- () -> jedis.sendCommand("key", Protocol.Command.SPOP, "key", "NaN", "extraArg"))
+ () -> jedis.sendCommand(SET_KEY, Protocol.Command.SPOP, SET_KEY, "5", "5"))
.hasMessageContaining(ERROR_SYNTAX);
Review comment:
Aah, I see what the problem is. When SPOP has too few arguments it uses the "ERR wrong number of arguments for 'spop' command" error message, but when it has too many arguments, it uses the "ERR syntax error" error message. The `assertAtMostNArgs()` method only expects to see the first error message, which is why it was failing.
I withdraw this comment, the test was right to begin with.
--
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 #7319: GEODE-9833: SPOP Command Support
Posted by 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
[GitHub] [geode] Kris-10-0 commented on a change in pull request #7319: GEODE-9833: SPOP Command Support
Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on a change in pull request #7319:
URL: https://github.com/apache/geode/pull/7319#discussion_r794905233
##########
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 I call members.remove() it might cause the backingArrayLength to shrink/rehash, and might possibly shrink/rehash multiple times if the count is large enough.
--
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 #7319: GEODE-9833: SPOP Command Support
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #7319:
URL: https://github.com/apache/geode/pull/7319#discussion_r795785591
##########
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:
I see how this works now. I think Darrel's suggestion is good especially when the backing array is fairly sparse. I'm not sure if I read the code properly, but my back-of-the-napkin calculation was that for an initial size of 100 elements the backing array would have a size of 256. With a load factor of 0.75, the worst case fill percentage is about 37.5%.
--
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] Kris-10-0 commented on a change in pull request #7319: GEODE-9833: SPOP Command Support
Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on a change in pull request #7319:
URL: https://github.com/apache/geode/pull/7319#discussion_r794763148
##########
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:
The load factor is 3/4, so I believe it would be null 1/4 of the time. I used this method to get set members from the backing array, because of Darrel's comment on not copying over the member set.
I could be wrong, but I don't think it would be stuck in the loop infinitely. In SPOPEXECUTOR there is a check to make sure that the set size is greater than 0, and in spop there is a check before this that makes sure the count is less than the size of the set. So when entering this loop there would be some members in the set, and it would remove an amount that is less than the total size of the set. I guess it could loop infinitely if spop is called outside SPOPEXECUTOR, but I could move the 0 size check in this method.
--
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] Kris-10-0 commented on a change in pull request #7319: GEODE-9833: SPOP Command Support
Posted by GitBox <gi...@apache.org>.
Kris-10-0 commented on a change in pull request #7319:
URL: https://github.com/apache/geode/pull/7319#discussion_r794098696
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -300,7 +283,7 @@ private void srandomDuplicateList(int count, List<byte[]> result) {
}
}
- private void srandomUniqueListWithSmallCount(int count, List<byte[]> result) {
+ private void srandomUniqueListAddToResult(int count, List<byte[]> result, boolean isSPop) {
Review comment:
It only adds the index if it isn't spop. When it does spop there is an indexUsed set, but nothing gets added so it is a waste.
--
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 #7319: GEODE-9833: SPOP Command Support
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7319:
URL: https://github.com/apache/geode/pull/7319#discussion_r794072811
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -300,7 +283,7 @@ private void srandomDuplicateList(int count, List<byte[]> result) {
}
}
- private void srandomUniqueListWithSmallCount(int count, List<byte[]> result) {
+ private void srandomUniqueListAddToResult(int count, List<byte[]> result, boolean isSPop) {
Review comment:
I think it would be better to leave this method and `srandomUniqueListWithLargeCount()` as they were and introduce new logic specifically for SPOP. Although the process for SPOP and SRANDMEMBER is broadly the same, there's a key difference, which is that SPOP modifies the set as it goes, which could lead to rehashing of the backing array, which in turn leads to `indexesUsed` becoming incorrect. Since with SPOP we can't possibly remove the same element twice, it's not necessary to keep track of the indexes used, and we can instead just check if the member at the random index we picked was null or not:
```
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) {
result.add(member);
members.remove(member);
}
}
}
storeChanges(region, key, new RemoveByteArrays(result));
return result;
}
```
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSPopIntegrationTest.java
##########
@@ -52,213 +56,137 @@ public void tearDown() {
}
@Test
- public void givenKeyNotProvided_returnsWrongNumberOfArgumentsError() {
- assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SPOP))
- .hasMessageContaining("ERR wrong number of arguments for 'spop' command");
+ public void spopTooFewArgs_returnsError() {
+ assertAtLeastNArgs(jedis, Protocol.Command.SPOP, 1);
}
@Test
- public void givenMoreThanThreeArguments_returnsSyntaxError() {
+ public void spopTooManyArgs_returnsError() {
assertThatThrownBy(
- () -> jedis.sendCommand("key", Protocol.Command.SPOP, "key", "NaN", "extraArg"))
+ () -> jedis.sendCommand(SET_KEY, Protocol.Command.SPOP, SET_KEY, "5", "5"))
.hasMessageContaining(ERROR_SYNTAX);
}
@Test
- public void givenCountIsNotAnInteger_returnsNotIntegerError() {
- assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SPOP, "key", "NaN"))
+ public void spop_withNonNumericCount_returnsError() {
+ assertThatThrownBy(() -> jedis.sendCommand(SET_KEY, Protocol.Command.SPOP, SET_KEY, "b"))
.hasMessageContaining(ERROR_VALUE_MUST_BE_POSITIVE);
}
@Test
- public void testSPop() {
- int ENTRIES = 10;
-
- List<String> masterSet = new ArrayList<>();
- for (int i = 0; i < ENTRIES; i++) {
- masterSet.add("master-" + i);
- }
-
- jedis.sadd("master", masterSet.toArray(new String[] {}));
- String poppy = jedis.spop("master");
-
- masterSet.remove(poppy);
- assertThat(jedis.smembers("master").toArray()).containsExactlyInAnyOrder(masterSet.toArray());
-
- assertThat(jedis.spop("spopnonexistent")).isNull();
+ public void spop_withNegativeCount_returnsError() {
+ assertThatThrownBy(() -> jedis.spop(SET_KEY, -1))
+ .hasMessageContaining(ERROR_VALUE_MUST_BE_POSITIVE);
}
@Test
- public void testSPopAll() {
- int ENTRIES = 10;
-
- List<String> masterSet = new ArrayList<>();
- for (int i = 0; i < ENTRIES; i++) {
- masterSet.add("master-" + i);
- }
-
- jedis.sadd("master", masterSet.toArray(new String[] {}));
- Set<String> popped = jedis.spop("master", ENTRIES);
-
- assertThat(jedis.smembers("master").toArray()).isEmpty();
- assertThat(popped.toArray()).containsExactlyInAnyOrder(masterSet.toArray());
+ public void spop_withoutCount_withNonExistentSet_returnsNull_setNotCreated() {
+ assertThat(jedis.spop(NON_EXISTENT_SET_KEY)).isNull();
+ assertThat(jedis.exists(NON_EXISTENT_SET_KEY)).isFalse();
}
@Test
- public void testSPopAllPlusOne() {
- int ENTRIES = 10;
-
- List<String> masterSet = new ArrayList<>();
- for (int i = 0; i < ENTRIES; i++) {
- masterSet.add("master-" + i);
- }
-
- jedis.sadd("master", masterSet.toArray(new String[] {}));
- Set<String> popped = jedis.spop("master", ENTRIES + 1);
+ public void spop_withoutCount_withExistentSet_returnsOneMember_removesReturnedMemberFromSet() {
+ jedis.sadd(SET_KEY, SET_MEMBERS);
- assertThat(jedis.smembers("master").toArray()).isEmpty();
- assertThat(popped.toArray()).containsExactlyInAnyOrder(masterSet.toArray());
+ String result = jedis.spop(SET_KEY);
+ assertThat(result).isIn(Arrays.asList(SET_MEMBERS));
+ assertThat(jedis.smembers(SET_KEY)).doesNotContain(result).isSubsetOf(SET_MEMBERS)
+ .doesNotHaveDuplicates();
Review comment:
The assertions here could be made a little more explicit by doing:
```
List<String> setMembersList = Arrays.asList(SET_MEMBERS);
assertThat(result).isIn(setMembersList);
setMembersList.remove(result);
assertThat(jedis.smembers(SET_KEY)).doesNotContain(result)
.containsExactlyInAnyOrderElementsOf(setMembersList);
```
so that we're saying that we removed the member returned from SPOP and didn't change the set in any way other than that. The same approach should be used in other places where we're currently asserting `.isSubsetOf(SET_MEMBERS).doesNotHaveDuplicates()`.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/set/AbstractSPopIntegrationTest.java
##########
@@ -52,213 +56,137 @@ public void tearDown() {
}
@Test
- public void givenKeyNotProvided_returnsWrongNumberOfArgumentsError() {
- assertThatThrownBy(() -> jedis.sendCommand("key", Protocol.Command.SPOP))
- .hasMessageContaining("ERR wrong number of arguments for 'spop' command");
+ public void spopTooFewArgs_returnsError() {
+ assertAtLeastNArgs(jedis, Protocol.Command.SPOP, 1);
}
@Test
- public void givenMoreThanThreeArguments_returnsSyntaxError() {
+ public void spopTooManyArgs_returnsError() {
assertThatThrownBy(
- () -> jedis.sendCommand("key", Protocol.Command.SPOP, "key", "NaN", "extraArg"))
+ () -> jedis.sendCommand(SET_KEY, Protocol.Command.SPOP, SET_KEY, "5", "5"))
.hasMessageContaining(ERROR_SYNTAX);
Review comment:
This test can be simplified by using the `RedisCommandArgumentsTestHelper.assertAtMostNArgs()` method.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/set/SetRandomExecutor.java
##########
@@ -43,34 +38,30 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con
try {
count = narrowLongToInt(bytesToLong(commandElems.get(2)));
} catch (NumberFormatException e) {
- return RedisResponse.error(ERROR_NOT_INTEGER);
+ return RedisResponse.error(getError());
}
} else {
count = 1;
}
List<byte[]> results =
- context.lockedExecute(key, () -> getResult(count, context.getRegionProvider(), key));
+ context.lockedExecute(key, () -> performCommand(count, context.getRegionProvider(), key));
+
if (hasCount) {
return RedisResponse.array(results, true);
} else {
if (results.isEmpty()) {
return RedisResponse.nil();
} else {
- return RedisResponse.bulkString(results.get(0));
+ return RedisResponse.bulkString(results.iterator().next());
Review comment:
I think this was better as it was before, since there's some overhead associated with creating an iterator that isn't there for just directly accessing the first element in the list.
--
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