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-unsubscr...@geode.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
