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/23 22:02:59 UTC
[GitHub] [geode] nonbinaryprogrammer opened a new pull request #7392: GEODE-9948: Implement LINSERT Command
nonbinaryprogrammer opened a new pull request #7392:
URL: https://github.com/apache/geode/pull/7392
Implements the LINSERT command as described by the Redis documentation.
LINSERT key BEFORE|AFTER pivot element
The command inserts an element into a list either BEFORE or AFTER the
reference element, which is called the 'pivot'.
It returns the new length of the list if the element is inserted, and -1
if the pivot is not an element of the list.
If the key does not exist, it will not be created, and the command will
return -1.
<!-- Thank you for submitting a contribution to Apache Geode. -->
<!-- In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
-->
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
<!-- Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
-->
--
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
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r817179294
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java
##########
@@ -135,6 +161,27 @@ public int getDSFID() {
return REDIS_LIST_ID;
}
+ public int elementInsert(byte[] elementToInsert, byte[] referenceElement,
+ boolean before) {
+ int i = 0;
+ for (byte[] element : elementList) {
Review comment:
I think it would be better to use elementList.listIterator(0) in this code. Then when you find the referenceElement you can just ask the iterator to add elementToInsert without doing another traversal of the linked list which add(i,e) does. Since ListIterator can move either direction you might need to back it up an element before calling add if "before==true". Also if "before==false" and referencedElement is at the end of elementList then I think you will need to call "elementList.add(elementToInsert)" to add it to the end of the list. I think this is worth doing so that you have O(n) instead of 2*O(n) performance.
--
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
[GitHub] [geode] DonalEvans commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r828297704
##########
File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LInsertDUnitTest.java
##########
@@ -0,0 +1,182 @@
+/*
+ * 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.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 redis.clients.jedis.args.ListPosition.AFTER;
+import static redis.clients.jedis.args.ListPosition.BEFORE;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.args.ListPosition;
+
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+import org.apache.geode.test.junit.rules.ExecutorServiceRule;
+
+public class LInsertDUnitTest {
+ public static final int INITIAL_LIST_SIZE = 500;
+
+ @Rule
+ public RedisClusterStartupRule clusterStartUp = new RedisClusterStartupRule();
+
+ @Rule
+ public ExecutorServiceRule executor = new ExecutorServiceRule();
+
+ private static JedisCluster jedis;
+ private static final String insertedValue = "insertedValue";
+
+ @Before
+ public void testSetup() {
+ MemberVM locator = clusterStartUp.startLocatorVM(0);
+ clusterStartUp.startRedisVM(1, locator.getPort());
+ clusterStartUp.startRedisVM(2, locator.getPort());
+ clusterStartUp.startRedisVM(3, locator.getPort());
+ int redisServerPort = clusterStartUp.getRedisPort(1);
+ jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, redisServerPort), REDIS_CLIENT_TIMEOUT);
+ clusterStartUp.flushAll();
+ }
+
+ @After
+ public void tearDown() {
+ jedis.close();
+ }
+
+ @Test
+ public void shouldDistributeDataAmongCluster_andRetainDataAfterServerCrash() {
+ String key = makeListKeyWithHashtag(1, clusterStartUp.getKeyOnServer("linsert", 1));
+ List<String> elementList = makeElementList(key, INITIAL_LIST_SIZE);
+ lpushPerformAndVerify(key, elementList);
+
+ assertThat(jedis.linsert(key, BEFORE, jedis.lindex(key, 2), insertedValue))
+ .isEqualTo(elementList.size() + 1);
+
+ assertThat(jedis.lindex(key, 2)).isEqualTo(insertedValue);
+
+ clusterStartUp.crashVM(1); // kill primary
+
+ assertThat(jedis.llen(key)).isEqualTo(elementList.size() + 1);
+ assertThat(jedis.lindex(key, 2)).isEqualTo(insertedValue);
+ assertThat(jedis.lindex(key, 3)).isEqualTo(elementList.get(INITIAL_LIST_SIZE - 3));
+ }
+
+ @Test
+ public void givenBucketsMoveDuringLInsert_operationsAreNotLost() throws Exception {
+ AtomicBoolean continueInserting = new AtomicBoolean(true);
+ List<String> listHashtags = makeListHashtags();
+ List<String> keys = makeListKeys(listHashtags);
+
+ List<String> elementList1 = makeElementList(keys.get(0), INITIAL_LIST_SIZE);
+ List<String> elementList2 = makeElementList(keys.get(1), INITIAL_LIST_SIZE);
+ List<String> elementList3 = makeElementList(keys.get(2), INITIAL_LIST_SIZE);
+
+ lpushPerformAndVerify(keys.get(0), elementList1);
+ lpushPerformAndVerify(keys.get(1), elementList2);
+ lpushPerformAndVerify(keys.get(2), elementList3);
+
+ Runnable task1 = () -> linsertPerformAndVerify(keys.get(0), BEFORE,
+ jedis.lindex(keys.get(0), 2), insertedValue, continueInserting);
+ Runnable task2 = () -> linsertPerformAndVerify(keys.get(1), AFTER, jedis.lindex(keys.get(1), 2),
+ insertedValue, continueInserting);
+ Runnable task3 = () -> linsertPerformAndVerify(keys.get(2), AFTER, jedis.lindex(keys.get(2), 2),
+ insertedValue, continueInserting);
+
+ Future<Void> future1 = executor.runAsync(task1);
+ Future<Void> future2 = executor.runAsync(task2);
+ Future<Void> future3 = executor.runAsync(task3);
+
+ for (int i = 0; i < 20; i++) {
+ continueInserting.set(false);
Review comment:
This should be moved to after the for loop, since we want to make sure that we're inserting elements at the same time that buckets are moving, and we only stop inserting once we've done the desired number of bucket moves.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLInsertIntegrationTest.java
##########
@@ -0,0 +1,211 @@
+/*
+ * 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_SYNTAX;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_WRONG_TYPE;
+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 org.apache.geode.redis.ConcurrentLoopingThreads;
+import org.apache.geode.redis.RedisIntegrationTest;
+
+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_onKeyThatDoesNotExist_withInvalidBefore_errorsBecauseOfWrongNumOfArgs() {
Review comment:
This test name doesn't quite capture what the test is doing. It might be better as "linsert_onKeyThatDoesNotExist_withInvalidBefore_returnsSyntaxError"
##########
File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LInsertDUnitTest.java
##########
@@ -0,0 +1,182 @@
+/*
+ * 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.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 redis.clients.jedis.args.ListPosition.AFTER;
+import static redis.clients.jedis.args.ListPosition.BEFORE;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.args.ListPosition;
+
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+import org.apache.geode.test.junit.rules.ExecutorServiceRule;
+
+public class LInsertDUnitTest {
+ public static final int INITIAL_LIST_SIZE = 500;
+
+ @Rule
+ public RedisClusterStartupRule clusterStartUp = new RedisClusterStartupRule();
+
+ @Rule
+ public ExecutorServiceRule executor = new ExecutorServiceRule();
+
+ private static JedisCluster jedis;
+ private static final String insertedValue = "insertedValue";
+
+ @Before
+ public void testSetup() {
+ MemberVM locator = clusterStartUp.startLocatorVM(0);
+ clusterStartUp.startRedisVM(1, locator.getPort());
+ clusterStartUp.startRedisVM(2, locator.getPort());
+ clusterStartUp.startRedisVM(3, locator.getPort());
+ int redisServerPort = clusterStartUp.getRedisPort(1);
+ jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, redisServerPort), REDIS_CLIENT_TIMEOUT);
+ clusterStartUp.flushAll();
+ }
+
+ @After
+ public void tearDown() {
+ jedis.close();
+ }
+
+ @Test
+ public void shouldDistributeDataAmongCluster_andRetainDataAfterServerCrash() {
+ String key = makeListKeyWithHashtag(1, clusterStartUp.getKeyOnServer("linsert", 1));
+ List<String> elementList = makeElementList(key, INITIAL_LIST_SIZE);
+ lpushPerformAndVerify(key, elementList);
+
+ assertThat(jedis.linsert(key, BEFORE, jedis.lindex(key, 2), insertedValue))
+ .isEqualTo(elementList.size() + 1);
+
+ assertThat(jedis.lindex(key, 2)).isEqualTo(insertedValue);
+
+ clusterStartUp.crashVM(1); // kill primary
+
+ assertThat(jedis.llen(key)).isEqualTo(elementList.size() + 1);
+ assertThat(jedis.lindex(key, 2)).isEqualTo(insertedValue);
+ assertThat(jedis.lindex(key, 3)).isEqualTo(elementList.get(INITIAL_LIST_SIZE - 3));
+ }
+
+ @Test
+ public void givenBucketsMoveDuringLInsert_operationsAreNotLost() throws Exception {
+ AtomicBoolean continueInserting = new AtomicBoolean(true);
+ List<String> listHashtags = makeListHashtags();
+ List<String> keys = makeListKeys(listHashtags);
+
+ List<String> elementList1 = makeElementList(keys.get(0), INITIAL_LIST_SIZE);
+ List<String> elementList2 = makeElementList(keys.get(1), INITIAL_LIST_SIZE);
+ List<String> elementList3 = makeElementList(keys.get(2), INITIAL_LIST_SIZE);
+
+ lpushPerformAndVerify(keys.get(0), elementList1);
+ lpushPerformAndVerify(keys.get(1), elementList2);
+ lpushPerformAndVerify(keys.get(2), elementList3);
+
+ Runnable task1 = () -> linsertPerformAndVerify(keys.get(0), BEFORE,
+ jedis.lindex(keys.get(0), 2), insertedValue, continueInserting);
+ Runnable task2 = () -> linsertPerformAndVerify(keys.get(1), AFTER, jedis.lindex(keys.get(1), 2),
+ insertedValue, continueInserting);
+ Runnable task3 = () -> linsertPerformAndVerify(keys.get(2), AFTER, jedis.lindex(keys.get(2), 2),
+ insertedValue, continueInserting);
+
+ Future<Void> future1 = executor.runAsync(task1);
+ Future<Void> future2 = executor.runAsync(task2);
+ Future<Void> future3 = executor.runAsync(task3);
+
+ for (int i = 0; i < 20; i++) {
+ continueInserting.set(false);
+ clusterStartUp.moveBucketForKey(listHashtags.get(i % listHashtags.size()));
+ Thread.sleep(500);
+ }
+
+ future1.get();
+ future2.get();
+ future3.get();
+ }
+
+ private List<String> makeListHashtags() {
+ List<String> listHashtags = new ArrayList<>();
+ listHashtags.add(clusterStartUp.getKeyOnServer("lpop", 1));
+ listHashtags.add(clusterStartUp.getKeyOnServer("lpop", 2));
+ listHashtags.add(clusterStartUp.getKeyOnServer("lpop", 3));
+ return listHashtags;
+ }
+
+ private List<String> makeListKeys(List<String> listHashtags) {
+ List<String> keys = new ArrayList<>();
+ keys.add(makeListKeyWithHashtag(1, listHashtags.get(0)));
+ keys.add(makeListKeyWithHashtag(2, listHashtags.get(1)));
+ keys.add(makeListKeyWithHashtag(3, listHashtags.get(2)));
+ return keys;
+ }
+
+ private void linsertPerformAndVerify(String key, ListPosition pos, String pivot, String value,
+ AtomicBoolean continueInserting) {
+ while (continueInserting.get()) {
+ long startLength = jedis.llen(key);
+ assertThat(jedis.linsert(key, pos, pivot, value)).isEqualTo(startLength + 1);
+
+ for (int i = 0; i < startLength + 1; i++) {
+ if (pos == BEFORE && jedis.lindex(key, i).equalsIgnoreCase(value)) {
+ assertThat(jedis.lindex(key, i + 1)).isEqualTo(pivot);
+ break;
+ } else if (pos == AFTER && jedis.lindex(key, i).equalsIgnoreCase(pivot)) {
+ assertThat(jedis.lindex(key, i + 1)).isEqualTo(value);
+ break;
+ }
+
+ if (i == startLength) {
+ assertThat(jedis.lindex(key, startLength))
+ .as("neither element nor pivot was found")
+ .isEqualTo(value);
+ }
+ }
+ }
+ }
Review comment:
I think it should be possible to simplify and speed up this method a bit by passing in the initial index of the pivot we want to use rather than the value of the pivot itself, and make the assertions more robust by having what gets inserted be distinct on each iteration:
```
private void linsertPerformAndVerify(String key, ListPosition pos, int pivotIndex,
String valueBase, AtomicBoolean continueInserting) {
int counter = 0;
while (continueInserting.get()) {
String insertedValue = valueBase + counter;
long startLength = jedis.llen(key);
String pivot = jedis.lindex(key, pivotIndex);
assertThat(jedis.linsert(key, pos, pivot, insertedValue)).isEqualTo(startLength + 1);
if (pos == BEFORE) {
// Increment the pivot index as we just inserted a new value before it
pivotIndex++;
assertThat(jedis.lindex(key, pivotIndex - 1)).isEqualTo(insertedValue);
assertThat(jedis.lindex(key, pivotIndex)).isEqualTo(pivot);
} else {
assertThat(jedis.lindex(key, pivotIndex + 1)).isEqualTo(insertedValue);
assertThat(jedis.lindex(key, pivotIndex)).isEqualTo(pivot);
}
}
}
```
This would mean we don't have to iterate the list at all when verifying, and makes the assertions a little stronger. This change would then make lines 101-106:
```
Runnable task1 =
() -> linsertPerformAndVerify(keys.get(0), BEFORE, 2, insertedValue, continueInserting);
Runnable task2 =
() -> linsertPerformAndVerify(keys.get(1), AFTER, 2, insertedValue, continueInserting);
Runnable task3 =
() -> linsertPerformAndVerify(keys.get(2), AFTER, 2, insertedValue, continueInserting);
```
--
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
[GitHub] [geode] DonalEvans commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r827259098
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java
##########
@@ -126,6 +127,25 @@ private int getArrayIndex(int listIndex) {
return listIndex;
}
+ /**
+ * @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 list to add to
+ * @return the number of elements in the list after the element is inserted
Review comment:
It would be good to include a mention in this comment that this method can also return -1 if the pivot element was not found.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java
##########
@@ -204,6 +229,32 @@ public int getDSFID() {
return REDIS_LIST_ID;
}
+ public synchronized int elementInsert(byte[] elementToInsert, byte[] referenceElement,
+ boolean before) {
+ int i = 0;
+ ListIterator<byte[]> iterator = elementList.listIterator();
+
+ while (iterator.hasNext()) {
+ if (Arrays.equals(iterator.next(), referenceElement)) {
+ if (!before) {
Review comment:
Really small nitpick, but could this if statement be inverted, so that we have `if (before) {` at the top? I find it makes it easier to read if we can avoid negation in the conditions.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java
##########
@@ -126,6 +127,25 @@ private int getArrayIndex(int listIndex) {
return listIndex;
}
+ /**
+ * @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 list to add to
+ * @return the number of elements in the list after the element is inserted
+ */
+ public int linsert(byte[] elementToInsert, byte[] referenceElement, boolean before,
+ Region<RedisKey, RedisData> region, RedisKey key) {
+ int index = elementInsert(elementToInsert, referenceElement, before);
+ if (index == -1) {
+ return index;
+ }
+ storeChanges(region, key, new InsertByteArray(elementToInsert, index));
+
+ return elementList.size();
Review comment:
This method should be updated to use versioning in the Delta, similar to what was done for the APPEND command in GEODE-10108.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LInsertExecutor.java
##########
@@ -0,0 +1,56 @@
+/*
+ * 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();
+
+ 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")) {
Review comment:
Rather than creating a String here and doing `equalsIgnoreCase()` it would be more performant to add two new constants to `StringBytesGlossary`:
```
// LInsertExecutor
public static final byte[] BEFORE = stringToBytes("BEFORE");
public static final byte[] AFTER = stringToBytes("AFTER");
```
and compare against them using `Arrays.equals()` here in the executor:
```
byte[] direction = toUpperCaseBytes(commandElements.get(2));
if (Arrays.equals(direction, BEFORE)) {
before = true;
} else if (Arrays.equals(direction, AFTER)) {
before = false;
```
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r815214420
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java
##########
@@ -135,6 +161,27 @@ public int getDSFID() {
return REDIS_LIST_ID;
}
+ public synchronized int elementInsert(byte[] elementToInsert, byte[] referenceElement,
+ boolean before) {
+ for (int i = 0; i < elementList.size(); i++) {
+ if (Arrays.equals(elementList.get(i), referenceElement)) {
+ if (before) {
+ elementList.add(i, elementToInsert);
+ return i;
+ } else {
+ elementList.add(i + 1, elementToInsert);
+ return i + 1;
+ }
+ }
+ }
+
+ return -1;
+ }
+
+ public synchronized void elementInsert(byte[] toInsert, int index) {
Review comment:
this elementInsert is used when applying the delta. at that point the only information that we have is the index and the new value
--
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
[GitHub] [geode] ezoerner commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
ezoerner commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r815033639
##########
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:
The last line should end with 3 vertical bars
--
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
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r815132743
##########
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);
+ }
+
+ long numEntries = context.listLockedExecute(key, false,
Review comment:
I noticed in another PR that for index we use a 32-bit "int" on lists. I also see that index on this PR's applyInsertByteArrayDelta is an "int".
So why is "numEntries" a long? If the max size of a list is 2G then this can return an int. But if the list can be bigger than 2G then our indexes need to also be "long".
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java
##########
@@ -135,6 +161,27 @@ public int getDSFID() {
return REDIS_LIST_ID;
}
+ public synchronized int elementInsert(byte[] elementToInsert, byte[] referenceElement,
+ boolean before) {
+ for (int i = 0; i < elementList.size(); i++) {
Review comment:
I think you definitely want to change this to iterate the elements instead of using elementList.get(i). Since elementList is a linked list, every time you call it it has to walk the linked list again to find element 'i'. If you instead just do "for (byte[] element: elementList)" then you will only walk the links once. You will need to also keep you own counter so you know what index you are on but that is easy to do with a local "index" that you increment every time around the for loop,.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java
##########
@@ -135,6 +161,27 @@ public int getDSFID() {
return REDIS_LIST_ID;
}
+ public synchronized int elementInsert(byte[] elementToInsert, byte[] referenceElement,
+ boolean before) {
+ for (int i = 0; i < elementList.size(); i++) {
+ if (Arrays.equals(elementList.get(i), referenceElement)) {
+ if (before) {
+ elementList.add(i, elementToInsert);
+ return i;
+ } else {
+ elementList.add(i + 1, elementToInsert);
+ return i + 1;
+ }
+ }
+ }
+
+ return -1;
+ }
+
+ public synchronized void elementInsert(byte[] toInsert, int index) {
Review comment:
I could be wrong but this seems like overkill. Just have the single 'int elementInsert' method. Callers are free to ignore its result. If you do keep this you don't need it to be synchronized since it does not do anything that is not thread safe.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/delta/InsertByteArray.java
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.data.delta;
+
+import static org.apache.geode.DataSerializer.readByteArray;
+import static org.apache.geode.DataSerializer.readInteger;
+import static org.apache.geode.redis.internal.data.delta.DeltaType.INSERT_BYTE_ARRAY;
+
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+
+import org.apache.geode.DataSerializer;
+import org.apache.geode.internal.InternalDataSerializer;
+import org.apache.geode.redis.internal.data.AbstractRedisData;
+
+public class InsertByteArray implements DeltaInfo {
+ private final byte[] byteArray;
+ private final int index;
+
+ public InsertByteArray(byte[] delta, int index) {
+ byteArray = delta;
+ this.index = index;
+ }
+
+ public void serializeTo(DataOutput out) throws IOException {
+ DataSerializer.writeEnum(INSERT_BYTE_ARRAY, out);
+ InternalDataSerializer.writeInteger(index, out);
Review comment:
Change this to DataSerializer.writePrimitiveInt
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/delta/InsertByteArray.java
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.data.delta;
+
+import static org.apache.geode.DataSerializer.readByteArray;
+import static org.apache.geode.DataSerializer.readInteger;
+import static org.apache.geode.redis.internal.data.delta.DeltaType.INSERT_BYTE_ARRAY;
+
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+
+import org.apache.geode.DataSerializer;
+import org.apache.geode.internal.InternalDataSerializer;
+import org.apache.geode.redis.internal.data.AbstractRedisData;
+
+public class InsertByteArray implements DeltaInfo {
+ private final byte[] byteArray;
+ private final int index;
+
+ public InsertByteArray(byte[] delta, int index) {
+ byteArray = delta;
+ this.index = index;
+ }
+
+ public void serializeTo(DataOutput out) throws IOException {
+ DataSerializer.writeEnum(INSERT_BYTE_ARRAY, out);
+ InternalDataSerializer.writeInteger(index, out);
+ InternalDataSerializer.writeByteArray(byteArray, out);
+ }
+
+ public static void deserializeFrom(DataInput in, AbstractRedisData redisData) throws IOException {
+ int index = readInteger(in);
Review comment:
Change this to DataSerializer.readPrimitiveInt
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java
##########
@@ -135,6 +161,27 @@ public int getDSFID() {
return REDIS_LIST_ID;
}
+ public synchronized int elementInsert(byte[] elementToInsert, byte[] referenceElement,
+ boolean before) {
+ for (int i = 0; i < elementList.size(); i++) {
+ if (Arrays.equals(elementList.get(i), referenceElement)) {
+ if (before) {
Review comment:
Consider something like this:
```
int insertIndex = i;
if (!before) {
insertIndex++;
}
elementList.add(insertIndex, elementToInsert);
return insertIndex;
```
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/delta/InsertByteArray.java
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.data.delta;
+
+import static org.apache.geode.DataSerializer.readByteArray;
+import static org.apache.geode.DataSerializer.readInteger;
+import static org.apache.geode.redis.internal.data.delta.DeltaType.INSERT_BYTE_ARRAY;
+
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+
+import org.apache.geode.DataSerializer;
+import org.apache.geode.internal.InternalDataSerializer;
+import org.apache.geode.redis.internal.data.AbstractRedisData;
+
+public class InsertByteArray implements DeltaInfo {
+ private final byte[] byteArray;
+ private final int index;
+
+ public InsertByteArray(byte[] delta, int index) {
+ byteArray = delta;
+ this.index = index;
+ }
+
+ public void serializeTo(DataOutput out) throws IOException {
+ DataSerializer.writeEnum(INSERT_BYTE_ARRAY, out);
+ InternalDataSerializer.writeInteger(index, out);
+ InternalDataSerializer.writeByteArray(byteArray, out);
Review comment:
Change this to DataSerializer. This class should not import InternalDataSerializer.
--
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r813411588
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLInsertIntegrationTest.java
##########
@@ -0,0 +1,142 @@
+/*
+ * 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 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 org.apache.geode.redis.RedisIntegrationTest;
+
+public abstract class AbstractLInsertIntegrationTest implements RedisIntegrationTest {
Review comment:
Could you add a test that uses `sendCommand` to send a mangled `BEFORE` or `AFTER` keyword?
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r830257517
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java
##########
@@ -229,6 +261,32 @@ public int getDSFID() {
return REDIS_LIST_ID;
}
+ public synchronized int elementInsert(byte[] elementToInsert, byte[] referenceElement,
+ boolean before) {
+ int i = 0;
Review comment:
for some reason this didn't work. I looked at it for awhile but don't see why it wouldn't. I left the code as is.
--
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
[GitHub] [geode] nonbinaryprogrammer merged pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer merged pull request #7392:
URL: https://github.com/apache/geode/pull/7392
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r821193537
##########
File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LInsertDUnitTest.java
##########
@@ -0,0 +1,182 @@
+/*
+ * 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.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 redis.clients.jedis.args.ListPosition.AFTER;
+import static redis.clients.jedis.args.ListPosition.BEFORE;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.args.ListPosition;
+
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+import org.apache.geode.test.junit.rules.ExecutorServiceRule;
+
+public class LInsertDUnitTest {
+ public static final int INITIAL_LIST_SIZE = 500;
+
+ @Rule
+ public RedisClusterStartupRule clusterStartUp = new RedisClusterStartupRule();
+
+ @Rule
+ public ExecutorServiceRule executor = new ExecutorServiceRule();
+
+ private static JedisCluster jedis;
+ private static final String insertedValue = "insertedValue";
+
+ @Before
+ public void testSetup() {
+ MemberVM locator = clusterStartUp.startLocatorVM(0);
+ clusterStartUp.startRedisVM(1, locator.getPort());
+ clusterStartUp.startRedisVM(2, locator.getPort());
+ clusterStartUp.startRedisVM(3, locator.getPort());
+ int redisServerPort = clusterStartUp.getRedisPort(1);
+ jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, redisServerPort), REDIS_CLIENT_TIMEOUT);
+ clusterStartUp.flushAll();
+ }
+
+ @After
+ public void tearDown() {
+ jedis.close();
+ }
+
+ @Test
+ public void shouldDistributeDataAmongCluster_andRetainDataAfterServerCrash() {
+ String key = makeListKeyWithHashtag(1, clusterStartUp.getKeyOnServer("linsert", 1));
+ List<String> elementList = makeElementList(key, INITIAL_LIST_SIZE);
+ lpushPerformAndVerify(key, elementList);
+
+ assertThat(jedis.linsert(key, BEFORE, jedis.lindex(key, 2), insertedValue))
+ .isEqualTo(elementList.size() + 1);
+
+ assertThat(jedis.lindex(key, 2)).isEqualTo(insertedValue);
+
+ clusterStartUp.crashVM(1); // kill primary
+
+ assertThat(jedis.llen(key)).isEqualTo(elementList.size() + 1);
+ assertThat(jedis.lindex(key, 2)).isEqualTo(insertedValue);
+ assertThat(jedis.lindex(key, 3)).isEqualTo(elementList.get(INITIAL_LIST_SIZE - 3));
+ }
+
+ @Test
+ public void givenBucketsMoveDuringLInsert_operationsAreNotLost() throws Exception {
Review comment:
I think this is resolved, let me know if not
--
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
[GitHub] [geode] DonalEvans commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r828492901
##########
File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LInsertDUnitTest.java
##########
@@ -136,26 +136,45 @@ public void givenBucketsMoveDuringLInsert_operationsAreNotLost() throws Exceptio
return keys;
}
- private void linsertPerformAndVerify(String key, ListPosition pos, String pivot, String value,
+ private void linsertPerformAndVerify(String key, ListPosition pos, int pivotIndex,
+ String valueBase, AtomicBoolean continueInserting) {
+ int counter = 0;
+ while (continueInserting.get()) {
+ String insertedValue = valueBase + counter;
+ long startLength = jedis.llen(key);
+ String pivot = jedis.lindex(key, pivotIndex);
+ assertThat(jedis.linsert(key, pos, pivot, insertedValue)).isEqualTo(startLength + 1);
+
+ if (pos == BEFORE) {
+ // Increment the pivot index as we just inserted a new value before it
+ pivotIndex++;
+ assertThat(jedis.lindex(key, pivotIndex - 1)).isEqualTo(insertedValue);
+ assertThat(jedis.lindex(key, pivotIndex)).isEqualTo(pivot);
+ } else {
+ assertThat(jedis.lindex(key, pivotIndex + 1)).isEqualTo(insertedValue);
+ assertThat(jedis.lindex(key, pivotIndex)).isEqualTo(pivot);
+ }
+ }
+ }
+
+ private void linsertPerformAndVerify2(String key, ListPosition pos, int pivotIndex,
Review comment:
I think there are accidentally two identical copies of this method now. This one should be removed.
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r819859428
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLInsertIntegrationTest.java
##########
@@ -0,0 +1,245 @@
+/*
+ * 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.WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND;
+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.ConcurrentLoopingThreads;
+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_onKeyThatDoesNotExist_withInvalidBefore_errorsBecauseOfWrongNumOfArgs() {
+ assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LINSERT, KEY, "LINSERT",
+ "notBefore", "doesn't matter", insertedValue))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage(
+ String.format(WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND, "linsert"));
+ }
+
+ @Test
+ public void linsert_onKeyThatIsNotAList_Errors() {
+ jedis.sadd(KEY, initialValue);
+
+ assertThatThrownBy(() -> jedis.linsert(KEY, BEFORE, initialValue, insertedValue))
+ .isInstanceOf(JedisDataException.class)
Review comment:
oh thought I removed all those
--
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
[GitHub] [geode] DonalEvans commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r819986344
##########
File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LInsertDUnitTest.java
##########
@@ -0,0 +1,182 @@
+/*
+ * 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.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 redis.clients.jedis.args.ListPosition.AFTER;
+import static redis.clients.jedis.args.ListPosition.BEFORE;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.args.ListPosition;
+
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+import org.apache.geode.test.junit.rules.ExecutorServiceRule;
+
+public class LInsertDUnitTest {
+ public static final int INITIAL_LIST_SIZE = 500;
+
+ @Rule
+ public RedisClusterStartupRule clusterStartUp = new RedisClusterStartupRule();
+
+ @Rule
+ public ExecutorServiceRule executor = new ExecutorServiceRule();
+
+ private static JedisCluster jedis;
+ private static final String insertedValue = "insertedValue";
+
+ @Before
+ public void testSetup() {
+ MemberVM locator = clusterStartUp.startLocatorVM(0);
+ clusterStartUp.startRedisVM(1, locator.getPort());
+ clusterStartUp.startRedisVM(2, locator.getPort());
+ clusterStartUp.startRedisVM(3, locator.getPort());
+ int redisServerPort = clusterStartUp.getRedisPort(1);
+ jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, redisServerPort), REDIS_CLIENT_TIMEOUT);
+ clusterStartUp.flushAll();
+ }
+
+ @After
+ public void tearDown() {
+ jedis.close();
+ }
+
+ @Test
+ public void shouldDistributeDataAmongCluster_andRetainDataAfterServerCrash() {
+ String key = makeListKeyWithHashtag(1, clusterStartUp.getKeyOnServer("linsert", 1));
+ List<String> elementList = makeElementList(key, INITIAL_LIST_SIZE);
+ lpushPerformAndVerify(key, elementList);
+
+ assertThat(jedis.linsert(key, BEFORE, jedis.lindex(key, 2), insertedValue))
+ .isEqualTo(elementList.size() + 1);
Review comment:
True, it's probably overkill. It mostly depends how much we want these tests to be black box tests that are agnostic about implementation details vs. white box tests that test what we know the implementation to be. It's fine to leave this change off.
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r816348715
##########
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:
yeah that might be nice. we should make a ticket to do something about the editing. seems like everyone dislikes the current method.
--
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
[GitHub] [geode] DonalEvans commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r819155865
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLInsertIntegrationTest.java
##########
@@ -0,0 +1,245 @@
+/*
+ * 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.WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND;
+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.ConcurrentLoopingThreads;
+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_onKeyThatDoesNotExist_withInvalidBefore_errorsBecauseOfWrongNumOfArgs() {
+ assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LINSERT, KEY, "LINSERT",
+ "notBefore", "doesn't matter", insertedValue))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage(
+ String.format(WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND, "linsert"));
+ }
Review comment:
This test is accidentally passing the wrong number of arguments, hence the `WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND` error. It should be:
```
@Test
public void linsert_onKeyThatDoesNotExist_withInvalidBefore_errors() {
assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LINSERT, KEY, "notBefore",
"doesn't matter", insertedValue))
.hasMessage(ERROR_SYNTAX);
}
```
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLInsertIntegrationTest.java
##########
@@ -0,0 +1,245 @@
+/*
+ * 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.WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND;
+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.ConcurrentLoopingThreads;
+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_onKeyThatDoesNotExist_withInvalidBefore_errorsBecauseOfWrongNumOfArgs() {
+ assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LINSERT, KEY, "LINSERT",
+ "notBefore", "doesn't matter", insertedValue))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage(
+ String.format(WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND, "linsert"));
+ }
+
+ @Test
+ public void linsert_onKeyThatIsNotAList_Errors() {
+ jedis.sadd(KEY, initialValue);
+
+ assertThatThrownBy(() -> jedis.linsert(KEY, BEFORE, initialValue, insertedValue))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage(RedisConstants.ERROR_WRONG_TYPE);
+ }
+
+ @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(WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND, "linsert"));
+ }
+
+ @Test
+ public void linsert_BEFORE_onKeyWithMultipleValues_withValidPivot_insertsValue() {
+ jedis.lpush(KEY, initialValue + "4");
+ jedis.lpush(KEY, initialValue + "3");
+ jedis.lpush(KEY, initialValue + "2");
+ jedis.lpush(KEY, initialValue + "1");
+ jedis.lpush(KEY, initialValue + "0");
+
+ assertThat(jedis.linsert(KEY, BEFORE, initialValue + "2", insertedValue)).isEqualTo(6L);
+
+ assertThat(jedis.llen(KEY)).isEqualTo(6L);
+
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "0");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "1");
+ assertThat(jedis.lpop(KEY)).isEqualTo(insertedValue);
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "2");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "3");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "4");
+ }
+
+ @Test
+ public void linsert_BEFORE_onKeyWithMultipleDuplicateValues_withValidPivot_insertsValue() {
+ jedis.lpush(KEY, initialValue + "3");
+ jedis.lpush(KEY, initialValue + "2");
+ jedis.lpush(KEY, initialValue + "1");
+ jedis.lpush(KEY, initialValue + "3");
+ jedis.lpush(KEY, initialValue + "0");
+
+ assertThat(jedis.linsert(KEY, BEFORE, initialValue + "3", insertedValue)).isEqualTo(6L);
+
+ assertThat(jedis.llen(KEY)).isEqualTo(6L);
+
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "0");
+ assertThat(jedis.lpop(KEY)).isEqualTo(insertedValue);
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "3");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "1");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "2");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "3");
+ }
+
+ @Test
+ public void linsert_AFTER_onKeyWithMultipleDuplicateValues_withValidPivot_insertsValue() {
+ jedis.lpush(KEY, initialValue + "3");
+ jedis.lpush(KEY, initialValue + "2");
+ jedis.lpush(KEY, initialValue + "1");
+ jedis.lpush(KEY, initialValue + "3");
+ jedis.lpush(KEY, initialValue + "0");
+
+ assertThat(jedis.linsert(KEY, AFTER, initialValue + "3", insertedValue)).isEqualTo(6L);
+
+ assertThat(jedis.llen(KEY)).isEqualTo(6L);
+
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "0");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "3");
+ assertThat(jedis.lpop(KEY)).isEqualTo(insertedValue);
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "1");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "2");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "3");
+ }
+
+ @Test
+ public void linsert_AFTER_onKeyWithMultipleValues_withValidPivot_insertsValue() {
+ jedis.lpush(KEY, initialValue + "4");
+ jedis.lpush(KEY, initialValue + "3");
+ jedis.lpush(KEY, initialValue + "2");
+ jedis.lpush(KEY, initialValue + "1");
+ jedis.lpush(KEY, initialValue + "0");
+
+ assertThat(jedis.linsert(KEY, AFTER, initialValue + "2", insertedValue)).isEqualTo(6L);
+
+ 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(insertedValue);
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "3");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "4");
+ }
+
+ @Test
+ public void linsert_BEFORE_firstElement() {
+ jedis.lpush(KEY, initialValue + "4");
+ jedis.lpush(KEY, initialValue + "3");
+ jedis.lpush(KEY, initialValue + "2");
+ jedis.lpush(KEY, initialValue + "1");
+ jedis.lpush(KEY, initialValue + "0");
+
+ assertThat(jedis.linsert(KEY, BEFORE, initialValue + "0", insertedValue)).isEqualTo(6L);
+
+ assertThat(jedis.lpop(KEY)).isEqualTo(insertedValue);
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "0");
+ }
+
+ @Test
+ public void linsert_AFTER_lastElement() {
+ jedis.lpush(KEY, initialValue + "4");
+ jedis.lpush(KEY, initialValue + "3");
+ jedis.lpush(KEY, initialValue + "2");
+ jedis.lpush(KEY, initialValue + "1");
+ jedis.lpush(KEY, initialValue + "0");
+
+ assertThat(jedis.linsert(KEY, AFTER, initialValue + "4", insertedValue)).isEqualTo(6L);
+
+ 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(initialValue + "3");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "4");
+ assertThat(jedis.lpop(KEY)).isEqualTo(insertedValue);
+ assertThat(jedis.lpop(KEY)).isNull();
+ }
+
+ @Test
+ public void testConcurrentLInserts() {
+ // we should never see "snake", "inserted value", "snake", etc.
+ String[] initialElements = {"lizard", "lizard", "lizard", "snake", "lizard", "lizard"};
+ String[] elementsToPush = {"snake", "snake", "snake", "snake", "snake", "snake"};
+
+ jedis.lpush(KEY, initialElements);
+
+ new ConcurrentLoopingThreads(1000,
+ i -> jedis.lpush(KEY, elementsToPush),
+ i -> jedis.linsert(KEY, BEFORE, "snake", insertedValue)).runWithAction(() -> {
+ assertThat(jedis.llen(KEY)).isEqualTo(initialElements.length + elementsToPush.length + 1);
+
+ assertThat(jedis).satisfiesAnyOf(
+ // LINSERT happened first
+ jedisClient -> {
+ assertThat(jedisClient.lindex(KEY, 8))
+ .as("failure 1")
+ .isEqualTo(insertedValue);
+ assertThat(jedisClient.lindex(KEY, 0)).isEqualTo("snake");
+ },
+ // LPUSH happened first
+ jedisClient -> assertThat(jedisClient.lindex(KEY, 0))
+ .as("failure 1").isEqualTo(insertedValue));
Review comment:
These failure messages could do with being a little more descriptive.
##########
File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LInsertDUnitTest.java
##########
@@ -0,0 +1,182 @@
+/*
+ * 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.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 redis.clients.jedis.args.ListPosition.AFTER;
+import static redis.clients.jedis.args.ListPosition.BEFORE;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.args.ListPosition;
+
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+import org.apache.geode.test.junit.rules.ExecutorServiceRule;
+
+public class LInsertDUnitTest {
+ public static final int INITIAL_LIST_SIZE = 500;
+
+ @Rule
+ public RedisClusterStartupRule clusterStartUp = new RedisClusterStartupRule();
+
+ @Rule
+ public ExecutorServiceRule executor = new ExecutorServiceRule();
+
+ private static JedisCluster jedis;
+ private static final String insertedValue = "insertedValue";
+
+ @Before
+ public void testSetup() {
+ MemberVM locator = clusterStartUp.startLocatorVM(0);
+ clusterStartUp.startRedisVM(1, locator.getPort());
+ clusterStartUp.startRedisVM(2, locator.getPort());
+ clusterStartUp.startRedisVM(3, locator.getPort());
+ int redisServerPort = clusterStartUp.getRedisPort(1);
+ jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, redisServerPort), REDIS_CLIENT_TIMEOUT);
+ clusterStartUp.flushAll();
+ }
+
+ @After
+ public void tearDown() {
+ jedis.close();
+ }
+
+ @Test
+ public void shouldDistributeDataAmongCluster_andRetainDataAfterServerCrash() {
+ String key = makeListKeyWithHashtag(1, clusterStartUp.getKeyOnServer("linsert", 1));
+ List<String> elementList = makeElementList(key, INITIAL_LIST_SIZE);
+ lpushPerformAndVerify(key, elementList);
+
+ assertThat(jedis.linsert(key, BEFORE, jedis.lindex(key, 2), insertedValue))
+ .isEqualTo(elementList.size() + 1);
Review comment:
Could this test also do an LINSERT using AFTER to make sure that the delta propagation for that is working correctly too, just in case?
##########
File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LInsertDUnitTest.java
##########
@@ -0,0 +1,182 @@
+/*
+ * 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.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 redis.clients.jedis.args.ListPosition.AFTER;
+import static redis.clients.jedis.args.ListPosition.BEFORE;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.args.ListPosition;
+
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+import org.apache.geode.test.junit.rules.ExecutorServiceRule;
+
+public class LInsertDUnitTest {
+ public static final int INITIAL_LIST_SIZE = 500;
+
+ @Rule
+ public RedisClusterStartupRule clusterStartUp = new RedisClusterStartupRule();
+
+ @Rule
+ public ExecutorServiceRule executor = new ExecutorServiceRule();
+
+ private static JedisCluster jedis;
+ private static final String insertedValue = "insertedValue";
+
+ @Before
+ public void testSetup() {
+ MemberVM locator = clusterStartUp.startLocatorVM(0);
+ clusterStartUp.startRedisVM(1, locator.getPort());
+ clusterStartUp.startRedisVM(2, locator.getPort());
+ clusterStartUp.startRedisVM(3, locator.getPort());
+ int redisServerPort = clusterStartUp.getRedisPort(1);
+ jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, redisServerPort), REDIS_CLIENT_TIMEOUT);
+ clusterStartUp.flushAll();
+ }
+
+ @After
+ public void tearDown() {
+ jedis.close();
+ }
+
+ @Test
+ public void shouldDistributeDataAmongCluster_andRetainDataAfterServerCrash() {
+ String key = makeListKeyWithHashtag(1, clusterStartUp.getKeyOnServer("linsert", 1));
+ List<String> elementList = makeElementList(key, INITIAL_LIST_SIZE);
+ lpushPerformAndVerify(key, elementList);
+
+ assertThat(jedis.linsert(key, BEFORE, jedis.lindex(key, 2), insertedValue))
+ .isEqualTo(elementList.size() + 1);
+
+ assertThat(jedis.lindex(key, 2)).isEqualTo(insertedValue);
+
+ clusterStartUp.crashVM(1); // kill primary
+
+ assertThat(jedis.llen(key)).isEqualTo(elementList.size() + 1);
+ assertThat(jedis.lindex(key, 2)).isEqualTo(insertedValue);
+ assertThat(jedis.lindex(key, 3)).isEqualTo(elementList.get(INITIAL_LIST_SIZE - 3));
+ }
+
+ @Test
+ public void givenBucketsMoveDuringLInsert_operationsAreNotLost() throws Exception {
+ AtomicLong numInserts = new AtomicLong(0);
+ List<String> listHashtags = makeListHashtags();
+ List<String> keys = makeListKeys(listHashtags);
+
+ List<String> elementList1 = makeElementList(keys.get(0), INITIAL_LIST_SIZE);
+ List<String> elementList2 = makeElementList(keys.get(1), INITIAL_LIST_SIZE);
+ List<String> elementList3 = makeElementList(keys.get(2), INITIAL_LIST_SIZE);
+
+ lpushPerformAndVerify(keys.get(0), elementList1);
+ lpushPerformAndVerify(keys.get(1), elementList2);
+ lpushPerformAndVerify(keys.get(2), elementList3);
+
+ Runnable task1 = () -> linsertPerformAndVerify(keys.get(0), BEFORE,
+ jedis.lindex(keys.get(0), 2), insertedValue, numInserts);
+ Runnable task2 = () -> linsertPerformAndVerify(keys.get(1), AFTER,
+ jedis.lindex(keys.get(1), 2), insertedValue, numInserts);
+ Runnable task3 = () -> linsertPerformAndVerify(keys.get(2), AFTER,
+ jedis.lindex(keys.get(2), 2), insertedValue, numInserts);
+
+ Future<Void> future1 = executor.runAsync(task1);
+ Future<Void> future2 = executor.runAsync(task2);
+ Future<Void> future3 = executor.runAsync(task3);
+
+ for (int i = 0; i < 50 && numInserts.get() > 0; i++) {
+ clusterStartUp.moveBucketForKey(listHashtags.get(i % listHashtags.size()));
+ Thread.sleep(500);
+ }
+
+ numInserts.set(0);
+
+ future1.get();
+ future2.get();
+ future3.get();
+ }
+
+ private List<String> makeListHashtags() {
+ List<String> listHashtags = new ArrayList<>();
+ listHashtags.add(clusterStartUp.getKeyOnServer("lpop", 1));
+ listHashtags.add(clusterStartUp.getKeyOnServer("lpop", 2));
+ listHashtags.add(clusterStartUp.getKeyOnServer("lpop", 3));
+ return listHashtags;
+ }
+
+ private List<String> makeListKeys(List<String> listHashtags) {
+ List<String> keys = new ArrayList<>();
+ keys.add(makeListKeyWithHashtag(1, listHashtags.get(0)));
+ keys.add(makeListKeyWithHashtag(2, listHashtags.get(1)));
+ keys.add(makeListKeyWithHashtag(3, listHashtags.get(2)));
+ return keys;
+ }
+
+ private void linsertPerformAndVerify(String key, ListPosition pos, String pivot, String value,
+ AtomicLong runningCount) {
+ long startLength = jedis.llen(key);
+ assertThat(jedis.linsert(key, pos, pivot, value)).isEqualTo(startLength + 1);
+ runningCount.incrementAndGet();
Review comment:
This running count needs to be incremented immediately before we start calling LINSERT and then either decremented again once we've done some maximum number of repeats or set to 0 if one of the threads doing the LINSERT hits an exception. The way the `lpopPerformAndVerify()` method in `LPopDUnitTest` handles this repeating/stopping behaviour is a pretty good guide.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLInsertIntegrationTest.java
##########
@@ -0,0 +1,245 @@
+/*
+ * 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.WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND;
+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.ConcurrentLoopingThreads;
+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_onKeyThatDoesNotExist_withInvalidBefore_errorsBecauseOfWrongNumOfArgs() {
+ assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LINSERT, KEY, "LINSERT",
+ "notBefore", "doesn't matter", insertedValue))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage(
+ String.format(WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND, "linsert"));
+ }
+
+ @Test
+ public void linsert_onKeyThatIsNotAList_Errors() {
+ jedis.sadd(KEY, initialValue);
+
+ assertThatThrownBy(() -> jedis.linsert(KEY, BEFORE, initialValue, insertedValue))
+ .isInstanceOf(JedisDataException.class)
Review comment:
We shouldn't be asserting on the exception type returned here, or anywhere else in this class, because that's behaviour controlled by Jedis, not geode. Just asserting that the message is correct is enough.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLInsertIntegrationTest.java
##########
@@ -0,0 +1,245 @@
+/*
+ * 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.WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND;
+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.ConcurrentLoopingThreads;
+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_onKeyThatDoesNotExist_withInvalidBefore_errorsBecauseOfWrongNumOfArgs() {
+ assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LINSERT, KEY, "LINSERT",
+ "notBefore", "doesn't matter", insertedValue))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage(
+ String.format(WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND, "linsert"));
+ }
+
+ @Test
+ public void linsert_onKeyThatIsNotAList_Errors() {
+ jedis.sadd(KEY, initialValue);
+
+ assertThatThrownBy(() -> jedis.linsert(KEY, BEFORE, initialValue, insertedValue))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage(RedisConstants.ERROR_WRONG_TYPE);
+ }
+
+ @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(WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND, "linsert"));
+ }
+
+ @Test
+ public void linsert_BEFORE_onKeyWithMultipleValues_withValidPivot_insertsValue() {
+ jedis.lpush(KEY, initialValue + "4");
+ jedis.lpush(KEY, initialValue + "3");
+ jedis.lpush(KEY, initialValue + "2");
+ jedis.lpush(KEY, initialValue + "1");
+ jedis.lpush(KEY, initialValue + "0");
Review comment:
This (and other instances of multiple successive calls to `lpush()`) could be simplified to:
```
jedis.lpush(KEY, "4", "3", "2", "1", "0");
```
We also don't need to be prefixing each element with `initialValue`.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLInsertIntegrationTest.java
##########
@@ -0,0 +1,245 @@
+/*
+ * 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.WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND;
+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.ConcurrentLoopingThreads;
+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_onKeyThatDoesNotExist_withInvalidBefore_errorsBecauseOfWrongNumOfArgs() {
+ assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LINSERT, KEY, "LINSERT",
+ "notBefore", "doesn't matter", insertedValue))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage(
+ String.format(WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND, "linsert"));
+ }
+
+ @Test
+ public void linsert_onKeyThatIsNotAList_Errors() {
+ jedis.sadd(KEY, initialValue);
+
+ assertThatThrownBy(() -> jedis.linsert(KEY, BEFORE, initialValue, insertedValue))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage(RedisConstants.ERROR_WRONG_TYPE);
+ }
+
+ @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(WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND, "linsert"));
+ }
Review comment:
This test is also using the wrong number of arguments for the `sendCommand()` method. It should be:
```
@Test
public void linsert_withInvalidBEFORE_errors() {
jedis.lpush(KEY, initialValue);
assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LINSERT, KEY, "notBefore",
initialValue, insertedValue))
.hasMessage(ERROR_SYNTAX);
}
```
##########
File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LInsertDUnitTest.java
##########
@@ -0,0 +1,182 @@
+/*
+ * 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.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 redis.clients.jedis.args.ListPosition.AFTER;
+import static redis.clients.jedis.args.ListPosition.BEFORE;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.args.ListPosition;
+
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+import org.apache.geode.test.junit.rules.ExecutorServiceRule;
+
+public class LInsertDUnitTest {
+ public static final int INITIAL_LIST_SIZE = 500;
+
+ @Rule
+ public RedisClusterStartupRule clusterStartUp = new RedisClusterStartupRule();
+
+ @Rule
+ public ExecutorServiceRule executor = new ExecutorServiceRule();
+
+ private static JedisCluster jedis;
+ private static final String insertedValue = "insertedValue";
+
+ @Before
+ public void testSetup() {
+ MemberVM locator = clusterStartUp.startLocatorVM(0);
+ clusterStartUp.startRedisVM(1, locator.getPort());
+ clusterStartUp.startRedisVM(2, locator.getPort());
+ clusterStartUp.startRedisVM(3, locator.getPort());
+ int redisServerPort = clusterStartUp.getRedisPort(1);
+ jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, redisServerPort), REDIS_CLIENT_TIMEOUT);
+ clusterStartUp.flushAll();
+ }
+
+ @After
+ public void tearDown() {
+ jedis.close();
+ }
+
+ @Test
+ public void shouldDistributeDataAmongCluster_andRetainDataAfterServerCrash() {
+ String key = makeListKeyWithHashtag(1, clusterStartUp.getKeyOnServer("linsert", 1));
+ List<String> elementList = makeElementList(key, INITIAL_LIST_SIZE);
+ lpushPerformAndVerify(key, elementList);
+
+ assertThat(jedis.linsert(key, BEFORE, jedis.lindex(key, 2), insertedValue))
+ .isEqualTo(elementList.size() + 1);
+
+ assertThat(jedis.lindex(key, 2)).isEqualTo(insertedValue);
+
+ clusterStartUp.crashVM(1); // kill primary
+
+ assertThat(jedis.llen(key)).isEqualTo(elementList.size() + 1);
+ assertThat(jedis.lindex(key, 2)).isEqualTo(insertedValue);
+ assertThat(jedis.lindex(key, 3)).isEqualTo(elementList.get(INITIAL_LIST_SIZE - 3));
+ }
+
+ @Test
+ public void givenBucketsMoveDuringLInsert_operationsAreNotLost() throws Exception {
Review comment:
This test is currently only doing 1 single LINSERT for each key. In order to test behaviour during bucket moves effectively, it needs to do repeated LINSERT calls until we've done some reasonable number of bucket moves (about 20 should be good).
--
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
[GitHub] [geode] DonalEvans commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [geode] DonalEvans commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r821227136
##########
File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LInsertDUnitTest.java
##########
@@ -0,0 +1,191 @@
+/*
+ * 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.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 redis.clients.jedis.args.ListPosition.AFTER;
+import static redis.clients.jedis.args.ListPosition.BEFORE;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.args.ListPosition;
+
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+import org.apache.geode.test.junit.rules.ExecutorServiceRule;
+
+public class LInsertDUnitTest {
+ public static final int INITIAL_LIST_SIZE = 500;
+
+ @Rule
+ public RedisClusterStartupRule clusterStartUp = new RedisClusterStartupRule();
+
+ @Rule
+ public ExecutorServiceRule executor = new ExecutorServiceRule();
+
+ private static JedisCluster jedis;
+ private static final String insertedValue = "insertedValue";
+
+ @Before
+ public void testSetup() {
+ MemberVM locator = clusterStartUp.startLocatorVM(0);
+ clusterStartUp.startRedisVM(1, locator.getPort());
+ clusterStartUp.startRedisVM(2, locator.getPort());
+ clusterStartUp.startRedisVM(3, locator.getPort());
+ int redisServerPort = clusterStartUp.getRedisPort(1);
+ jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, redisServerPort), REDIS_CLIENT_TIMEOUT);
+ clusterStartUp.flushAll();
+ }
+
+ @After
+ public void tearDown() {
+ jedis.close();
+ }
+
+ @Test
+ public void shouldDistributeDataAmongCluster_andRetainDataAfterServerCrash() {
+ String key = makeListKeyWithHashtag(1, clusterStartUp.getKeyOnServer("linsert", 1));
+ List<String> elementList = makeElementList(key, INITIAL_LIST_SIZE);
+ lpushPerformAndVerify(key, elementList);
+
+ assertThat(jedis.linsert(key, BEFORE, jedis.lindex(key, 2), insertedValue))
+ .isEqualTo(elementList.size() + 1);
+
+ assertThat(jedis.lindex(key, 2)).isEqualTo(insertedValue);
+
+ clusterStartUp.crashVM(1); // kill primary
+
+ assertThat(jedis.llen(key)).isEqualTo(elementList.size() + 1);
+ assertThat(jedis.lindex(key, 2)).isEqualTo(insertedValue);
+ assertThat(jedis.lindex(key, 3)).isEqualTo(elementList.get(INITIAL_LIST_SIZE - 3));
+ }
+
+ @Test
+ public void givenBucketsMoveDuringLInsert_operationsAreNotLost() throws Exception {
+ AtomicBoolean continueInserting = new AtomicBoolean(true);
+ List<String> listHashtags = makeListHashtags();
+ List<String> keys = makeListKeys(listHashtags);
+
+ List<String> elementList1 = makeElementList(keys.get(0), INITIAL_LIST_SIZE);
+ List<String> elementList2 = makeElementList(keys.get(1), INITIAL_LIST_SIZE);
+ List<String> elementList3 = makeElementList(keys.get(2), INITIAL_LIST_SIZE);
+
+ lpushPerformAndVerify(keys.get(0), elementList1);
+ lpushPerformAndVerify(keys.get(1), elementList2);
+ lpushPerformAndVerify(keys.get(2), elementList3);
+
+ Runnable task1 = () -> {
+ while (continueInserting.get()) {
+ linsertPerformAndVerify(keys.get(0), BEFORE, jedis.lindex(keys.get(0), 2), insertedValue);
+ }
Review comment:
It might be a little neater to put the while loop inside the `linsertPerformAndVerify()` method.
##########
File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LInsertDUnitTest.java
##########
@@ -98,24 +98,35 @@ public void givenBucketsMoveDuringLInsert_operationsAreNotLost() throws Exceptio
lpushPerformAndVerify(keys.get(1), elementList2);
lpushPerformAndVerify(keys.get(2), elementList3);
- Runnable task1 = () -> linsertPerformAndVerify(keys.get(0), BEFORE,
- jedis.lindex(keys.get(0), 2), insertedValue, numInserts);
- Runnable task2 = () -> linsertPerformAndVerify(keys.get(1), AFTER,
- jedis.lindex(keys.get(1), 2), insertedValue, numInserts);
- Runnable task3 = () -> linsertPerformAndVerify(keys.get(2), AFTER,
- jedis.lindex(keys.get(2), 2), insertedValue, numInserts);
+ Runnable task1 = () -> {
+ while (continueInserting.get()) {
+ linsertPerformAndVerify(keys.get(0), BEFORE, jedis.lindex(keys.get(0), 2), insertedValue);
+ }
+ };
+ Runnable task2 = () -> {
+ while (continueInserting.get()) {
+ linsertPerformAndVerify(keys.get(1), AFTER, jedis.lindex(keys.get(1), 2), insertedValue);
+ }
+ };
+ Runnable task3 = () -> {
+ while (continueInserting.get()) {
+ linsertPerformAndVerify(keys.get(2), AFTER, jedis.lindex(keys.get(2), 2), insertedValue);
+ }
+ };
Future<Void> future1 = executor.runAsync(task1);
Future<Void> future2 = executor.runAsync(task2);
Future<Void> future3 = executor.runAsync(task3);
- for (int i = 0; i < 50 && numInserts.get() > 0; i++) {
+ for (int i = 0; i < 50; i++) {
+ if (i == 20) {
+ continueInserting.set(false);
+ }
Review comment:
This could be simplified a bit to just have the loop run 20 times, then call `continueInserting.set(false);` afterwards, since we don't care about moving buckets if there are no operations ongoing.
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r828542347
##########
File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LInsertDUnitTest.java
##########
@@ -136,26 +136,45 @@ public void givenBucketsMoveDuringLInsert_operationsAreNotLost() throws Exceptio
return keys;
}
- private void linsertPerformAndVerify(String key, ListPosition pos, String pivot, String value,
+ private void linsertPerformAndVerify(String key, ListPosition pos, int pivotIndex,
+ String valueBase, AtomicBoolean continueInserting) {
+ int counter = 0;
+ while (continueInserting.get()) {
+ String insertedValue = valueBase + counter;
+ long startLength = jedis.llen(key);
+ String pivot = jedis.lindex(key, pivotIndex);
+ assertThat(jedis.linsert(key, pos, pivot, insertedValue)).isEqualTo(startLength + 1);
+
+ if (pos == BEFORE) {
+ // Increment the pivot index as we just inserted a new value before it
+ pivotIndex++;
+ assertThat(jedis.lindex(key, pivotIndex - 1)).isEqualTo(insertedValue);
+ assertThat(jedis.lindex(key, pivotIndex)).isEqualTo(pivot);
+ } else {
+ assertThat(jedis.lindex(key, pivotIndex + 1)).isEqualTo(insertedValue);
+ assertThat(jedis.lindex(key, pivotIndex)).isEqualTo(pivot);
+ }
+ }
+ }
+
+ private void linsertPerformAndVerify2(String key, ListPosition pos, int pivotIndex,
Review comment:
yeah, I saw that. I'm looking at the stressNewTest failures too
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r814302123
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLInsertIntegrationTest.java
##########
@@ -0,0 +1,142 @@
+/*
+ * 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 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 org.apache.geode.redis.RedisIntegrationTest;
+
+public abstract class AbstractLInsertIntegrationTest implements RedisIntegrationTest {
Review comment:
I added the test, thank you for thinking of that
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r819967022
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLInsertIntegrationTest.java
##########
@@ -0,0 +1,245 @@
+/*
+ * 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.WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND;
+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.ConcurrentLoopingThreads;
+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_onKeyThatDoesNotExist_withInvalidBefore_errorsBecauseOfWrongNumOfArgs() {
+ assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LINSERT, KEY, "LINSERT",
+ "notBefore", "doesn't matter", insertedValue))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage(
+ String.format(WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND, "linsert"));
+ }
+
+ @Test
+ public void linsert_onKeyThatIsNotAList_Errors() {
+ jedis.sadd(KEY, initialValue);
+
+ assertThatThrownBy(() -> jedis.linsert(KEY, BEFORE, initialValue, insertedValue))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage(RedisConstants.ERROR_WRONG_TYPE);
+ }
+
+ @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(WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND, "linsert"));
+ }
+
+ @Test
+ public void linsert_BEFORE_onKeyWithMultipleValues_withValidPivot_insertsValue() {
+ jedis.lpush(KEY, initialValue + "4");
+ jedis.lpush(KEY, initialValue + "3");
+ jedis.lpush(KEY, initialValue + "2");
+ jedis.lpush(KEY, initialValue + "1");
+ jedis.lpush(KEY, initialValue + "0");
+
+ assertThat(jedis.linsert(KEY, BEFORE, initialValue + "2", insertedValue)).isEqualTo(6L);
+
+ assertThat(jedis.llen(KEY)).isEqualTo(6L);
+
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "0");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "1");
+ assertThat(jedis.lpop(KEY)).isEqualTo(insertedValue);
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "2");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "3");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "4");
+ }
+
+ @Test
+ public void linsert_BEFORE_onKeyWithMultipleDuplicateValues_withValidPivot_insertsValue() {
+ jedis.lpush(KEY, initialValue + "3");
+ jedis.lpush(KEY, initialValue + "2");
+ jedis.lpush(KEY, initialValue + "1");
+ jedis.lpush(KEY, initialValue + "3");
+ jedis.lpush(KEY, initialValue + "0");
+
+ assertThat(jedis.linsert(KEY, BEFORE, initialValue + "3", insertedValue)).isEqualTo(6L);
+
+ assertThat(jedis.llen(KEY)).isEqualTo(6L);
+
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "0");
+ assertThat(jedis.lpop(KEY)).isEqualTo(insertedValue);
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "3");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "1");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "2");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "3");
+ }
+
+ @Test
+ public void linsert_AFTER_onKeyWithMultipleDuplicateValues_withValidPivot_insertsValue() {
+ jedis.lpush(KEY, initialValue + "3");
+ jedis.lpush(KEY, initialValue + "2");
+ jedis.lpush(KEY, initialValue + "1");
+ jedis.lpush(KEY, initialValue + "3");
+ jedis.lpush(KEY, initialValue + "0");
+
+ assertThat(jedis.linsert(KEY, AFTER, initialValue + "3", insertedValue)).isEqualTo(6L);
+
+ assertThat(jedis.llen(KEY)).isEqualTo(6L);
+
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "0");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "3");
+ assertThat(jedis.lpop(KEY)).isEqualTo(insertedValue);
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "1");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "2");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "3");
+ }
+
+ @Test
+ public void linsert_AFTER_onKeyWithMultipleValues_withValidPivot_insertsValue() {
+ jedis.lpush(KEY, initialValue + "4");
+ jedis.lpush(KEY, initialValue + "3");
+ jedis.lpush(KEY, initialValue + "2");
+ jedis.lpush(KEY, initialValue + "1");
+ jedis.lpush(KEY, initialValue + "0");
+
+ assertThat(jedis.linsert(KEY, AFTER, initialValue + "2", insertedValue)).isEqualTo(6L);
+
+ 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(insertedValue);
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "3");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "4");
+ }
+
+ @Test
+ public void linsert_BEFORE_firstElement() {
+ jedis.lpush(KEY, initialValue + "4");
+ jedis.lpush(KEY, initialValue + "3");
+ jedis.lpush(KEY, initialValue + "2");
+ jedis.lpush(KEY, initialValue + "1");
+ jedis.lpush(KEY, initialValue + "0");
+
+ assertThat(jedis.linsert(KEY, BEFORE, initialValue + "0", insertedValue)).isEqualTo(6L);
+
+ assertThat(jedis.lpop(KEY)).isEqualTo(insertedValue);
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "0");
+ }
+
+ @Test
+ public void linsert_AFTER_lastElement() {
+ jedis.lpush(KEY, initialValue + "4");
+ jedis.lpush(KEY, initialValue + "3");
+ jedis.lpush(KEY, initialValue + "2");
+ jedis.lpush(KEY, initialValue + "1");
+ jedis.lpush(KEY, initialValue + "0");
+
+ assertThat(jedis.linsert(KEY, AFTER, initialValue + "4", insertedValue)).isEqualTo(6L);
+
+ 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(initialValue + "3");
+ assertThat(jedis.lpop(KEY)).isEqualTo(initialValue + "4");
+ assertThat(jedis.lpop(KEY)).isEqualTo(insertedValue);
+ assertThat(jedis.lpop(KEY)).isNull();
+ }
+
+ @Test
+ public void testConcurrentLInserts() {
+ // we should never see "snake", "inserted value", "snake", etc.
+ String[] initialElements = {"lizard", "lizard", "lizard", "snake", "lizard", "lizard"};
+ String[] elementsToPush = {"snake", "snake", "snake", "snake", "snake", "snake"};
+
+ jedis.lpush(KEY, initialElements);
+
+ new ConcurrentLoopingThreads(1000,
+ i -> jedis.lpush(KEY, elementsToPush),
+ i -> jedis.linsert(KEY, BEFORE, "snake", insertedValue)).runWithAction(() -> {
+ assertThat(jedis.llen(KEY)).isEqualTo(initialElements.length + elementsToPush.length + 1);
+
+ assertThat(jedis).satisfiesAnyOf(
+ // LINSERT happened first
+ jedisClient -> {
+ assertThat(jedisClient.lindex(KEY, 8))
+ .as("failure 1")
+ .isEqualTo(insertedValue);
+ assertThat(jedisClient.lindex(KEY, 0)).isEqualTo("snake");
+ },
+ // LPUSH happened first
+ jedisClient -> assertThat(jedisClient.lindex(KEY, 0))
+ .as("failure 1").isEqualTo(insertedValue));
Review comment:
oops I didn't mean to leave these in at all.
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r819968008
##########
File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LInsertDUnitTest.java
##########
@@ -0,0 +1,182 @@
+/*
+ * 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.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 redis.clients.jedis.args.ListPosition.AFTER;
+import static redis.clients.jedis.args.ListPosition.BEFORE;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import redis.clients.jedis.HostAndPort;
+import redis.clients.jedis.JedisCluster;
+import redis.clients.jedis.args.ListPosition;
+
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+import org.apache.geode.test.junit.rules.ExecutorServiceRule;
+
+public class LInsertDUnitTest {
+ public static final int INITIAL_LIST_SIZE = 500;
+
+ @Rule
+ public RedisClusterStartupRule clusterStartUp = new RedisClusterStartupRule();
+
+ @Rule
+ public ExecutorServiceRule executor = new ExecutorServiceRule();
+
+ private static JedisCluster jedis;
+ private static final String insertedValue = "insertedValue";
+
+ @Before
+ public void testSetup() {
+ MemberVM locator = clusterStartUp.startLocatorVM(0);
+ clusterStartUp.startRedisVM(1, locator.getPort());
+ clusterStartUp.startRedisVM(2, locator.getPort());
+ clusterStartUp.startRedisVM(3, locator.getPort());
+ int redisServerPort = clusterStartUp.getRedisPort(1);
+ jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, redisServerPort), REDIS_CLIENT_TIMEOUT);
+ clusterStartUp.flushAll();
+ }
+
+ @After
+ public void tearDown() {
+ jedis.close();
+ }
+
+ @Test
+ public void shouldDistributeDataAmongCluster_andRetainDataAfterServerCrash() {
+ String key = makeListKeyWithHashtag(1, clusterStartUp.getKeyOnServer("linsert", 1));
+ List<String> elementList = makeElementList(key, INITIAL_LIST_SIZE);
+ lpushPerformAndVerify(key, elementList);
+
+ assertThat(jedis.linsert(key, BEFORE, jedis.lindex(key, 2), insertedValue))
+ .isEqualTo(elementList.size() + 1);
Review comment:
I don't see the purpose of that. By the time the delta propagation happens the concept of BEFORE and AFTER has been lost
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r819859428
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLInsertIntegrationTest.java
##########
@@ -0,0 +1,245 @@
+/*
+ * 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.WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND;
+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.ConcurrentLoopingThreads;
+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_onKeyThatDoesNotExist_withInvalidBefore_errorsBecauseOfWrongNumOfArgs() {
+ assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LINSERT, KEY, "LINSERT",
+ "notBefore", "doesn't matter", insertedValue))
+ .isInstanceOf(JedisDataException.class)
+ .hasMessage(
+ String.format(WRONG_NUMBER_OF_ARGUMENTS_FOR_COMMAND, "linsert"));
+ }
+
+ @Test
+ public void linsert_onKeyThatIsNotAList_Errors() {
+ jedis.sadd(KEY, initialValue);
+
+ assertThatThrownBy(() -> jedis.linsert(KEY, BEFORE, initialValue, insertedValue))
+ .isInstanceOf(JedisDataException.class)
Review comment:
uh thought I removed all those
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r815028555
##########
File path: geode-docs/tools_modules/geode_for_redis.html.md.erb
##########
@@ -188,23 +188,23 @@ Could not connect to Redis at 127.0.0.1:6379: Connection refused
| 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 ||||
+| LPUSH | LINSERT | MGET | MSET |
Review comment:
yes, sorry about that. it is fixed now
--
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
[GitHub] [geode] ezoerner commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
ezoerner commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r815023722
##########
File path: geode-docs/tools_modules/geode_for_redis.html.md.erb
##########
@@ -188,23 +188,23 @@ Could not connect to Redis at 127.0.0.1:6379: Connection refused
| 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 ||||
+| LPUSH | LINSERT | MGET | MSET |
Review comment:
Can you keep the list of commands alphabetically sorted?
--
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
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r816970995
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java
##########
@@ -135,6 +161,27 @@ public int getDSFID() {
return REDIS_LIST_ID;
}
+ public int elementInsert(byte[] elementToInsert, byte[] referenceElement,
Review comment:
I think this method also needs to be synchronized to protect against a concurrent GII which will serialize this RedisList instance. The GII does not do all the other locking that our redis ops does. So anytime we are making a structural change that is not safe with a concurrent "toData" call we need to add synchronization
--
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
[GitHub] [geode] DonalEvans commented on a change in pull request #7392: GEODE-9948: Implement LINSERT Command
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7392:
URL: https://github.com/apache/geode/pull/7392#discussion_r830187158
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java
##########
@@ -127,6 +128,32 @@ private int getArrayIndex(int listIndex) {
return listIndex;
}
+ /**
+ * @param elementToInsert element to insert into the set
Review comment:
This should be "element to insert into the list"
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java
##########
@@ -229,6 +261,32 @@ public int getDSFID() {
return REDIS_LIST_ID;
}
+ public synchronized int elementInsert(byte[] elementToInsert, byte[] referenceElement,
+ boolean before) {
+ int i = 0;
Review comment:
Rather than using a separate counter here, you could instead use the `nextIndex()` or `previousIndex()` methods in `ListIterator` to return the appropriate index. Really the only reason to create a `ListIterator` rather than a plain `Iterator` here is to have access to those methods.
--
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