DonalEvans commented on a change in pull request #7418:
URL: https://github.com/apache/geode/pull/7418#discussion_r818224042
##########
File path:
geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LPopDUnitTest.java
##########
@@ -144,18 +144,19 @@ private void lpopPerformAndVerify(String key, AtomicLong
runningCount) {
int elementCount = INITIAL_LIST_SIZE - 1;
while (jedis.llen(key) > 0 && runningCount.get() > 0) {
- String expected = "-" + key + "-" + (elementCount) + "-";
+ String expected = makeElementString(key, elementCount);
String element = expected;
try {
element = jedis.lpop(key);
- if (element.equals(expected)) {
- elementCount--;
- } else {
- int expectedCount = elementCount - 1;
- String[] chunks = element.split("-");
- elementCount = Integer.parseInt(chunks[4]);
- assertThat(expectedCount - elementCount).isLessThanOrEqualTo(1);
- }
+ String[] chunks = element.split("-");
+ int actualCount = Integer.parseInt(chunks[4]);
+ int expectedCount = elementCount;
+ assertThat(actualCount)
+ .as("Actual element count did not match expected element count for
element " + element)
+ .satisfiesAnyOf(
+ count -> assertThat(count).isEqualTo(expectedCount),
+ count -> assertThat(count).isEqualTo(expectedCount + 1));
Review comment:
This should be `.isEqualTo(expectedCount - 1)` since when doing LPOP,
we're decrementing the element count each time, so a duplicated LPOP should
result in an element count that's one less than expected.
##########
File path:
geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/RPopDUnitTest.java
##########
@@ -146,19 +146,19 @@ private void rpopPerformAndVerify(String key, AtomicLong
runningCount) {
int elementCount = 0;
int elementListSize = INITIAL_LIST_SIZE - 1;
while (jedis.llen(key) > 0 && elementCount <= elementListSize &&
runningCount.get() > 0) {
- String expected = "-" + key + "-" + elementCount + "-";
+ String expected = makeElementString(key, elementCount);
String element = expected;
Review comment:
This can also be just:
```
String element = "";
```
##########
File path:
geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LPopDUnitTest.java
##########
@@ -144,18 +144,19 @@ private void lpopPerformAndVerify(String key, AtomicLong
runningCount) {
int elementCount = INITIAL_LIST_SIZE - 1;
while (jedis.llen(key) > 0 && runningCount.get() > 0) {
- String expected = "-" + key + "-" + (elementCount) + "-";
+ String expected = makeElementString(key, elementCount);
String element = expected;
Review comment:
`expected` is redundant here, and since the first thing we do inside the
try/catch is overwrite the value of `element`, this can become:
```
String element = "";
```
##########
File path: geode-docs/tools_modules/geode_for_redis.html.md.erb
##########
@@ -192,7 +192,7 @@ Could not connect to Redis at 127.0.0.1:6379: Connection
refused
| MSETNX | PERSIST | PEXPIRE | PEXPIREAT |
| PING | PSETEX | PSUBSCRIBE | PTTL |
| PUBLISH PUBSUB | PUNSUBSCRIBE | RENAME |
-| RENAMENX | RESTORE | SADD | SCARD |
+| RENAMENX | RESTORE | RPOP | SADD | SCARD |
Review comment:
This will break the formatting of the command list table, which
currently has 4 columns, as this line now has 5. It's a massive pain, but
currently in order to add a new command, you need to move every following
command too. I've suggested on other PRs that it would make life easier to
break this table up into multiple tables for each command type (Sets, Lists,
etc.) as that would mean less editing is needed each time a new command is
added.
--
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
