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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
