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/02/28 21:21:40 UTC
[GitHub] [geode] DonalEvans commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
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-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org