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 23:26:01 UTC
[GitHub] [geode] DonalEvans commented on a change in pull request #7319: GEODE-9833: SPOP Command Support
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