DonalEvans commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r816233853
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/server/AbstractHitsMissesIntegrationTest.java
##########
@@ -573,6 +574,12 @@ public void testSetbit() {
}
/************* List related commands *************/
+ @Test
+ public void testLinsert() {
Review comment:
Small nitpick, but could this be moved to be after `testLindex` to
preserver the alphabetical ordering in the class?
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LInsertExecutor.java
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.RedisConstants;
+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 LInsertExecutor implements CommandExecutor {
+
+ @Override
+ public RedisResponse executeCommand(Command command, ExecutionHandlerContext
context) {
+ List<byte[]> commandElements = command.getProcessedCommand();
+ Region<RedisKey, RedisData> region = context.getRegion();
+ RedisKey key = command.getKey();
Review comment:
These two lines should be moved to below the direction check, as it's
possible we error out before actually needing to use them.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLInsertIntegrationTest.java
##########
@@ -0,0 +1,155 @@
+/*
+ * 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.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 static redis.clients.jedis.args.ListPosition.AFTER;
+import static redis.clients.jedis.args.ListPosition.BEFORE;
+
+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.RedisIntegrationTest;
+import org.apache.geode.redis.internal.RedisConstants;
+
+public abstract class AbstractLInsertIntegrationTest implements
RedisIntegrationTest {
+ public static final String KEY = "key";
+ public static final String initialValue = "initialValue";
+ public static final String insertedValue = "insertedValue";
+ 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 linsertErrors_givenWrongNumberOfArguments() {
+ assertExactNumberOfArgs(jedis, Protocol.Command.LINSERT, 4);
+ }
+
+ @Test
+ public void linsert_onKeyThatDoesNotExist_doesNotCreateKey() {
+ assertThat(jedis.linsert(KEY, BEFORE, "not in here",
insertedValue)).isZero();
+ assertThat(jedis.exists(KEY)).isFalse();
+ }
+
+ @Test
+ public void linsert_withNonexistentPivot_returnsNegativeOne() {
+ jedis.lpush(KEY, initialValue);
+
+ assertThat(jedis.linsert(KEY, BEFORE, "nope",
insertedValue)).isEqualTo(-1);
+
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue);
+ assertThat(jedis.lpop(KEY)).isNull();
+ }
+
+ @Test
+ public void linsert_withInvalidBEFORE_errors() {
Review comment:
Could we also get a test of the behaviour when an invalid BEFORE
argument is passed but the key doesn't exist? Just to make sure we're
processing things in the same order that Redis does.
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLInsertIntegrationTest.java
##########
@@ -0,0 +1,155 @@
+/*
+ * 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.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 static redis.clients.jedis.args.ListPosition.AFTER;
+import static redis.clients.jedis.args.ListPosition.BEFORE;
+
+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.RedisIntegrationTest;
+import org.apache.geode.redis.internal.RedisConstants;
+
+public abstract class AbstractLInsertIntegrationTest implements
RedisIntegrationTest {
Review comment:
Could we also add a test of the behaviour when LINSERT is called
targeting a key that isn't a list?
Also a test of the concurrency behaviour using `ConcurrentLoopingThreads`?
For this test, we should have a list with some pre-populated elements, and two
threads, one calling LINSERT and one modifying the list by adding multiple
elements. Then, as the `runWithAction()` section, we should assert that the
returned value from LINSERT is one of two values (the expected value if it was
called before the elements were inserted, or the expected value if it was
called after the elements were inserted) and also that it was inserted at the
correct place, using LINDEX. Once the assertions are done, the list should be
deleted and then set back to the original value as the final part of the
`runWithAction()` to allow the test to continue looping and performing the same
actions each time.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LInsertExecutor.java
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.RedisConstants;
+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 LInsertExecutor implements CommandExecutor {
+
+ @Override
+ public RedisResponse executeCommand(Command command, ExecutionHandlerContext
context) {
+ List<byte[]> commandElements = command.getProcessedCommand();
+ Region<RedisKey, RedisData> region = context.getRegion();
+ RedisKey key = command.getKey();
+
+ String direction = Coder.bytesToString(commandElements.get(2));
Review comment:
This seems like a good candidate for an addition to the
`StringBytesGlossary` class, to prevent needing to create a String here. We
could then just do an `Arrays.equals()` in the if/else below.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java
##########
@@ -60,6 +62,25 @@ public RedisList() {
}
}
+ /**
+ * @param elementToInsert element to insert into the set
+ * @param referenceElement element to insert next to
+ * @param before true if inserting before reference element, false if it is
after
+ * @param region the region this instance is store in
+ * @param key the name of the set to add to
Review comment:
References to "set" should be changed to "list"
##########
File path:
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLInsertIntegrationTest.java
##########
@@ -0,0 +1,155 @@
+/*
+ * 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.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 static redis.clients.jedis.args.ListPosition.AFTER;
+import static redis.clients.jedis.args.ListPosition.BEFORE;
+
+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.RedisIntegrationTest;
+import org.apache.geode.redis.internal.RedisConstants;
+
+public abstract class AbstractLInsertIntegrationTest implements
RedisIntegrationTest {
+ public static final String KEY = "key";
+ public static final String initialValue = "initialValue";
+ public static final String insertedValue = "insertedValue";
+ 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 linsertErrors_givenWrongNumberOfArguments() {
+ assertExactNumberOfArgs(jedis, Protocol.Command.LINSERT, 4);
+ }
+
+ @Test
+ public void linsert_onKeyThatDoesNotExist_doesNotCreateKey() {
+ assertThat(jedis.linsert(KEY, BEFORE, "not in here",
insertedValue)).isZero();
+ assertThat(jedis.exists(KEY)).isFalse();
+ }
+
+ @Test
+ public void linsert_withNonexistentPivot_returnsNegativeOne() {
+ jedis.lpush(KEY, initialValue);
+
+ assertThat(jedis.linsert(KEY, BEFORE, "nope",
insertedValue)).isEqualTo(-1);
+
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue);
+ assertThat(jedis.lpop(KEY)).isNull();
+ }
+
+ @Test
+ public void linsert_withInvalidBEFORE_errors() {
+ jedis.lpush(KEY, initialValue);
+
+ assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LINSERT,
KEY, "LINSERT",
+ "notBefore", initialValue, insertedValue))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage(
+ String.format("ERR " +
RedisConstants.ERROR_WRONG_NUMBER_OF_ARGS, "linsert"));
+ }
+
+ @Test
+ public void
linsert_BEFORE_onKeyWithMultipleValues_withValidPivot_insertsValue() {
Review comment:
Could we also get tests for the cases that the list contains duplicate
elements, to make sure that we're inserting before or after the right one? For
example, for a list `{1, 2, 2, 2, 3}`, calling `LINSERT KEY BEFORE 2 A` should
result in `{1, A, 2, 2, 2, 3}` and `LINSERT KEY AFTER 2 A` should result in
`{1, 2, A, 2, 2, 3}`
##########
File path: geode-docs/tools_modules/geode_for_redis.html.md.erb
##########
@@ -187,24 +187,24 @@ Could not connect to Redis at 127.0.0.1:6379: Connection
refused
| HMGET | HMSET | HSCAN **[3]** | HSET |
| HSETNX | HSTRLEN | HVALS | INCR |
| INCRBY | INCRBYFLOAT | INFO **[4]** | KEYS |
-| LINDEX | LLEN | LOLWUT | LPOP |
-| LPUSH | MGET | MSET | MSETNX |
-| PERSIST | PEXPIRE | PEXPIREAT | PING |
-| PSETEX | PSUBSCRIBE | PTTL | PUBLISH |
-| PUBSUB | PUNSUBSCRIBE | RENAME | RENAMENX |
-| RESTORE | SADD | SCARD | SDIFF |
-| SDIFFSTORE | SET | SETEX | SETNX |
-| SETRANGE | SINTER | SINTERSTORE | SISMEMBER |
-| SMEMBERS | SMOVE | SPOP | SRANDMEMBER |
-| SREM | SSCAN **[3]** | STRLEN | SUBSCRIBE |
-| SUNION | SUNIONSTORE | TTL | TYPE |
-| UNSUBSCRIBE | QUIT | ZADD | ZCARD |
-| ZCOUNT | ZINCRBY | ZINTERSTORE | ZLEXCOUNT |
-| ZPOPMAX | ZPOPMIN | ZRANGE | ZRANGEBYLEX |
-| ZRANGEBYSCORE | ZRANK | ZREM | ZREMRANGEBYLEX |
-| ZREMRANGEBYRANK | ZREMRANGEBYSCORE | ZREVRANGE | ZREVRANGEBYLEX |
-| ZREVRANGEBYSCORE | ZREVRANK | ZSCAN **[3]** | ZSCORE |
-| ZUNIONSTORE ||||
+| LINDEX | LINSERT | LLEN | LOLWUT |
+| LPOP | LPUSH | MGET | MSET |
+| MSETNX | PERSIST | PEXPIRE | PEXPIREAT |
+| PING | PSETEX | PSUBSCRIBE | PTTL |
+| PUBLISH | PUBSUB | PUNSUBSCRIBE | RENAME |
+| RENAMENX | RESTORE | SADD | SCARD |
+| SDIFF | SDIFFSTORE | SET | SETEX |
+| SETNX | SETRANGE | SINTER | SINTERSTORE |
+| SISMEMBER | SMEMBERS | SMOVE | SPOP |
+| SRANDMEMBER | SREM | SSCAN **[3]** | STRLEN |
+| SUBSCRIBE | SUNION | SUNIONSTORE | TTL |
+| TYPE | UNSUBSCRIBE | QUIT | ZADD |
+| ZCARD | ZCOUNT | ZINCRBY | ZINTERSTORE |
+| ZLEXCOUNT | ZPOPMAX | ZPOPMIN | ZRANGE |
+| ZRANGEBYLEX | ZRANGEBYSCORE | ZRANK | ZREM |
+| ZREMRANGEBYLEX | ZREMRANGEBYRANK | ZREMRANGEBYSCORE | ZREVRANGE |
+| ZREVRANGEBYLEX | ZREVRANGEBYSCORE | ZREVRANK | ZSCAN **[3]** |
+| ZSCORE | ZUNIONSTORE |||
Review comment:
To avoid having to reformat this whole table every time we add a new
list command, might it be sensible to break it up into multiple tables, one for
each command type? That way, we only have to modify the one we're adding a
command for, which is much less work than moving every subsequent command in
the table following the one we're adding. Even if this doesn't end up being the
final state of the docs, it will smooth out the development process while we're
actively adding new commands.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LInsertExecutor.java
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.RedisConstants;
+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 LInsertExecutor implements CommandExecutor {
+
+ @Override
+ public RedisResponse executeCommand(Command command, ExecutionHandlerContext
context) {
+ List<byte[]> commandElements = command.getProcessedCommand();
+ Region<RedisKey, RedisData> region = context.getRegion();
+ RedisKey key = command.getKey();
+
+ String direction = Coder.bytesToString(commandElements.get(2));
+ boolean before;
+ byte[] referenceElement = commandElements.get(3);
+ byte[] elementToInsert = commandElements.get(4);
+
+ if (direction.equalsIgnoreCase("before")) {
+ before = true;
+ } else if (direction.equalsIgnoreCase("after")) {
+ before = false;
+ } else {
+ return RedisResponse.error(RedisConstants.ERROR_SYNTAX);
+ }
+
+ int numEntries = context.listLockedExecute(key, false,
Review comment:
This name is slightly inaccurate, since in the case that the reference
value doesn't exist, we return -1, which is not the number of entries in the
list. A better name might just be "result".
--
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
