DonalEvans commented on a change in pull request #7396:
URL: https://github.com/apache/geode/pull/7396#discussion_r817008483
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java
##########
@@ -33,6 +33,7 @@
public static final String ERROR_UNKNOWN_COMMAND =
"unknown command `%s`, with args beginning with: %s";
public static final String ERROR_OUT_OF_RANGE = "The number provided is out
of range";
+ public static final String ERROR_INDEX_OUT_OF_RANGE = "index out of range";
Review comment:
After rebasing to pull in the changes from GEODE-10049, this string
should be "ERR index out of range"
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LSetExecutor.java
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.commands.executor.list;
+
+
+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.data.RedisKey;
+import org.apache.geode.redis.internal.netty.Coder;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class LSetExecutor implements CommandExecutor {
Review comment:
The order in which we handle error conditions in this class needs to be
tweaked a bit to match what Redis does. Currently the two tests below pass with
open source Redis but fail with this code:
```
@Test
public void
lset_returnsNotAnIntegerError_givenKeyExistsAndIndexIsNotAValidInteger() {
jedis.lpush(KEY, initialValue);
assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LSET,
KEY, "notAnInteger", "newElement")).hasMessage("ERR " + ERROR_NOT_INTEGER);
}
@Test
public void
lset_returnsNoSuchKeyError_givenKeyDoesNotExistAndIndexIsNotAValidInteger() {
assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LSET,
KEY, "notAnInteger", "newElement")).hasMessage("ERR " + ERROR_NO_SUCH_KEY);
}
```
We need to first check if the key exists, then check that the index is a
valid integer (using a try/catch around `Coder.bytesToLong()`), all while
holding the lock for the key. Changing the `executeCommand()` method to the
following should fix these issues:
```
public RedisResponse executeCommand(Command command, ExecutionHandlerContext
context) {
List<byte[]> commandElements = command.getProcessedCommand();
RedisKey key = command.getKey();
return context.listLockedExecute(key, false, list -> {
if (!list.exists()) {
return RedisResponse.error(ERROR_NO_SUCH_KEY);
}
int index;
try {
index = narrowLongToInt(bytesToLong(commandElements.get(2)));
} catch (NumberFormatException ex) {
return RedisResponse.error(ERROR_NOT_INTEGER);
}
Region<RedisKey, RedisData> region = context.getRegion();
byte[] value = commandElements.get(3);
list.lset(region, key, index, value);
return RedisResponse.ok();
});
}
```
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLSetIntegrationTest.java
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.commands.executor.list;
+
+import static
org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static
org.apache.geode.redis.internal.RedisConstants.ERROR_INDEX_OUT_OF_RANGE;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NO_SUCH_KEY;
+import static
org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
+import static
org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.Random;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+import redis.clients.jedis.exceptions.JedisDataException;
+
+import org.apache.geode.redis.ConcurrentLoopingThreads;
+import org.apache.geode.redis.RedisIntegrationTest;
+import org.apache.geode.redis.internal.RedisException;
+
+public abstract class AbstractLSetIntegrationTest implements
RedisIntegrationTest {
+ public static final String KEY = "key";
+ public static final String initialValue = "initialValue";
+ public static final String newValue = "newValue";
+ private JedisCluster jedis;
+
+ @Before
+ public void setUp() {
+ jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()),
REDIS_CLIENT_TIMEOUT);
+ }
+
+ @After
+ public void tearDown() {
+ flushAll();
+ jedis.close();
+ }
+
+ @Test
+ public void lset_errors_givenTooFewArguments() {
+ assertExactNumberOfArgs(jedis, Protocol.Command.LSET, 3);
+ }
+
+ @Test
+ public void lset_onKeyThatDoesNotExist_returnsError_doesNotCreateKey() {
+ assertThatThrownBy(() -> jedis.lset(KEY, 1, newValue))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage("ERR " + ERROR_NO_SUCH_KEY);
+ assertThat(jedis.exists(KEY)).isFalse();
+ }
+
+ @Test
+ public void lset_errors_withIndexOutOfRange() {
+ jedis.lpush(KEY, initialValue);
+ assertThatThrownBy(() -> jedis.lset(KEY, 10, newValue))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage("ERR " + ERROR_INDEX_OUT_OF_RANGE);
+
+ assertThatThrownBy(() -> jedis.lset(KEY, -10, newValue))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage("ERR " + ERROR_INDEX_OUT_OF_RANGE);
+ }
+
+ @Test
+ public void lset_setsValue_givenValidPositiveIndex_withOneItemInList() {
+ jedis.lpush(KEY, initialValue);
+ assertThat(jedis.lset(KEY, 0, newValue)).isEqualTo("OK");
+ assertThat(jedis.lpop(KEY)).isEqualTo(newValue);
+ }
+
+ @Test
+ public void lset_setsValue_givenValidPositiveIndex_withMultipleItemsInList()
{
+ for (int i = 4; i >= 0; i--) {
+ jedis.lpush(KEY, initialValue + i);
+ }
+
+ assertThat(jedis.lset(KEY, 2, newValue)).isEqualTo("OK");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "0");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "1");
+ assertThat(jedis.lpop(KEY)).isEqualTo(newValue);
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "3");
+ }
+
+ @Test
+ public void lset_setsValue_givenValidNegativeIndex_withOneItemInList() {
+ jedis.lpush(KEY, initialValue);
+ assertThat(jedis.lset(KEY, -1, newValue)).isEqualTo("OK");
+ assertThat(jedis.lpop(KEY)).isEqualTo(newValue);
+ }
+
+ @Test
+ public void lset_setsValue_givenValidNegativeIndex_withMultipleItemsInList()
{
+ for (int i = 4; i >= 0; i--) {
+ jedis.lpush(KEY, initialValue + i);
+ }
+
+ assertThat(jedis.lset(KEY, -2, newValue)).isEqualTo("OK");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "0");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "1");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "2");
+ assertThat(jedis.lpop(KEY)).isEqualTo(newValue);
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "4");
+ }
+
+ @Test
+ public void lset_whenRunConcurrently_doesNotThrowRedisException() {
Review comment:
This test is not doing what you think it is, due to the way
`ConcurrentLoopingThreads` handles exceptions. Replacing the assertions in the
`catch` block with `assertThat(true).isFalse();` still results in the test
passing.
A better test of correct concurrent behaviour for LSET would be to have a
test that does an LSET concurrently with a multi-element LPUSH and confirms
that either we set the element before the LPUSH happened, or we set it after,
but not partway through:
```
@Test
public void lset_withConcurrentLpush_behavesCorrectly() {
String[] initialElements = {"1", "2", "3", "4", "5", "6"};
jedis.lpush(KEY, initialElements);
String[] elementsToAdd = {"a", "b", "c", "d", "e", "f"};
new ConcurrentLoopingThreads(1000,
i -> jedis.lpush(KEY, elementsToAdd),
i -> jedis.lset(KEY, 0, newValue)
).runWithAction(() -> {
assertThat(jedis.llen(KEY)).isEqualTo(initialElements.length +
elementsToAdd.length);
assertThat(jedis).satisfiesAnyOf(
// LPUSH was applied first
jedisClient -> {
assertThat(jedisClient.lindex(KEY, 0)).isEqualTo(newValue);
assertThat(jedisClient.lindex(KEY,
elementsToAdd.length)).isEqualTo(initialElements[initialElements.length - 1]);
},
// LSET was applied first
jedisClient -> {
assertThat(jedisClient.lindex(KEY,
0)).isEqualTo(elementsToAdd[elementsToAdd.length - 1]);
assertThat(jedisClient.lindex(KEY,
elementsToAdd.length)).isEqualTo(newValue);
}
);
// Reset the list to its original contents
jedis.del(KEY);
jedis.lpush(KEY, initialElements);
});
}
```
We could also have a test for LSET with concurrent LPOP on a single-element
list, to make sure that either LSET is applied first and we correctly set the
element in the list then pop it, or LPOP is applied first and we get an
ERROR_NO_SUCH_KEY response from LSET. If we get an ERROR_INDEX_OUT_OF_RANGE
response, we know that LSET attempted to set the value on a nonexistent list,
which shouldn't happen with correct locking:
```
@Test
public void lset_withConcurrentLpop_behavesCorrectly() {
jedis.lpush(KEY, initialValue);
AtomicReference<String> lpopResult = new AtomicReference<>();
AtomicReference<Throwable> lsetException = new AtomicReference<>();
new ConcurrentLoopingThreads(1000,
i -> lpopResult.set(jedis.lpop(KEY)),
i -> {
try {
jedis.lset(KEY, 0, newValue);
} catch (Exception ex) {
lsetException.set(ex);
}
}
).runWithAction(() -> {
assertThat(jedis.exists(KEY)).isFalse();
assertThat(lpopResult.get()).satisfiesAnyOf(
// LPOP was applied first
poppedValue -> {
assertThat(poppedValue).isEqualTo(initialValue);
assertThat(lsetException.get()).hasMessage("ERR " +
ERROR_NO_SUCH_KEY);
},
// LSET was applied first
poppedValue -> {
assertThat(poppedValue).isEqualTo(newValue);
assertThat(lsetException.get()).isNull();
}
);
// Reset the list to its original contents and clear the atomic
references
jedis.lpush(KEY, initialValue);
lpopResult.set(null);
lsetException.set(null);
});
}
```
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLSetIntegrationTest.java
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.commands.executor.list;
+
+import static
org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static
org.apache.geode.redis.internal.RedisConstants.ERROR_INDEX_OUT_OF_RANGE;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NO_SUCH_KEY;
+import static
org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
+import static
org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.Random;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+import redis.clients.jedis.exceptions.JedisDataException;
+
+import org.apache.geode.redis.ConcurrentLoopingThreads;
+import org.apache.geode.redis.RedisIntegrationTest;
+import org.apache.geode.redis.internal.RedisException;
+
+public abstract class AbstractLSetIntegrationTest implements
RedisIntegrationTest {
+ public static final String KEY = "key";
+ public static final String initialValue = "initialValue";
+ public static final String newValue = "newValue";
+ private JedisCluster jedis;
+
+ @Before
+ public void setUp() {
+ jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()),
REDIS_CLIENT_TIMEOUT);
+ }
+
+ @After
+ public void tearDown() {
+ flushAll();
+ jedis.close();
+ }
+
+ @Test
+ public void lset_errors_givenTooFewArguments() {
+ assertExactNumberOfArgs(jedis, Protocol.Command.LSET, 3);
+ }
+
+ @Test
+ public void lset_onKeyThatDoesNotExist_returnsError_doesNotCreateKey() {
+ assertThatThrownBy(() -> jedis.lset(KEY, 1, newValue))
+ .isInstanceOf(JedisDataException.class)
Review comment:
We shouldn't be asserting on the exception type returned here (or
elsewhere in this class), as that's controlled by Jedis, not our code. if the
Jedis client changes the exception they throw, this test will start failing
despite the behaviour of the geode-for-redis server being correct.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/server/AbstractHitsMissesIntegrationTest.java
##########
@@ -593,6 +593,14 @@ public void testLlen() {
runCommandAndAssertHitsAndMisses(LIST_KEY, k -> jedis.llen(k));
}
+ @Test
+ public void testLset() {
+ runCommandAndAssertNoStatUpdates(LIST_KEY, k -> {
+ jedis.lpush(k, "value");
+ jedis.lset(k, 1, "newvalue");
+ });
+ }
Review comment:
This can be simplified to:
```
runCommandAndAssertNoStatUpdates(LIST_KEY, k -> jedis.lset(k, 0,
"newvalue"));
```
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/NullRedisList.java
##########
@@ -50,4 +53,9 @@ public long lpush(List<byte[]> elementsToAdd,
Region<RedisKey, RedisData> region
public int llen() {
return 0;
}
+
+ @Override
+ public void lset(Region<RedisKey, RedisData> region, RedisKey key, int
index, byte[] value) {
+ throw new RedisException(ERROR_NO_SUCH_KEY);
+ }
Review comment:
If the approach described for `LSetExecutor` is used, this method is no
longer needed.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLSetIntegrationTest.java
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
+ * or implied. See the License for the specific language governing permissions
and limitations under
+ * the License.
+ */
+package org.apache.geode.redis.internal.commands.executor.list;
+
+import static
org.apache.geode.redis.RedisCommandArgumentsTestHelper.assertExactNumberOfArgs;
+import static
org.apache.geode.redis.internal.RedisConstants.ERROR_INDEX_OUT_OF_RANGE;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_NO_SUCH_KEY;
+import static
org.apache.geode.test.dunit.rules.RedisClusterStartupRule.BIND_ADDRESS;
+import static
org.apache.geode.test.dunit.rules.RedisClusterStartupRule.REDIS_CLIENT_TIMEOUT;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.Random;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.Protocol;
+import redis.clients.jedis.exceptions.JedisDataException;
+
+import org.apache.geode.redis.ConcurrentLoopingThreads;
+import org.apache.geode.redis.RedisIntegrationTest;
+import org.apache.geode.redis.internal.RedisException;
+
+public abstract class AbstractLSetIntegrationTest implements
RedisIntegrationTest {
Review comment:
In addition to the two tests described in the comment on `LSetExecutor`
could we also add a test for the behaviour when the key targeted by LSET is not
a 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
