DonalEvans commented on a change in pull request #7418: URL: https://github.com/apache/geode/pull/7418#discussion_r818048157
########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/RPopExecutor.java ########## @@ -0,0 +1,38 @@ +/* + * 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 org.apache.geode.cache.Region; +import org.apache.geode.redis.internal.commands.Command; +import org.apache.geode.redis.internal.commands.executor.CommandExecutor; +import org.apache.geode.redis.internal.commands.executor.RedisResponse; +import org.apache.geode.redis.internal.data.RedisData; +import org.apache.geode.redis.internal.data.RedisKey; +import org.apache.geode.redis.internal.netty.ExecutionHandlerContext; + +public class RPopExecutor implements CommandExecutor { + + @Override + public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) + throws Exception { Review comment: An Exception is never thrown from this method, so this should be removed. ########## File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/RPopDUnitTest.java ########## @@ -0,0 +1,189 @@ +/* + * 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 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 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 RPopDUnitTest { + public static final int INITIAL_LIST_SIZE = 10; Review comment: This initial list size is too small to be effective in the `givenBucketsMoveDuringRpop_thenOperationsAreNotLost()` test, as all the elements in the list will be popped before the test has the chance to move any buckets. A better value would be at least 10,000, which in my testing allows for at least 10 bucket moves to happen over the course of the test. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractRPopIntegrationTest.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.assertAtMostNArgs; +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 java.util.concurrent.atomic.AtomicReference; + +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 AbstractRPopIntegrationTest implements RedisIntegrationTest { + public static final String KEY = "key"; + public static final String PREEXISTING_VALUE = "preexistingValue"; + protected JedisCluster jedis; + + @Before + public void setUp() { + jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT); + } + + @After + public void tearDown() { + flushAll(); + jedis.close(); + } + + // Overridden in RPopIntegrationTest until we implement Redis 6.2+ semantics + @Test + public void rpop_givenWrongNumOfArgs_returnsError() { + assertAtMostNArgs(jedis, Protocol.Command.RPOP, 2); + } + + @Test + public void rpop_withNonListKey_Fails() { + jedis.set("nonKey", PREEXISTING_VALUE); + assertThatThrownBy(() -> jedis.rpop("nonKey")).hasMessage(ERROR_WRONG_TYPE); + } + + @Test + public void rpop_withNonExistentKey_returnsNull() { + assertThat(jedis.rpop("nonexistentKey")).isNull(); + } + + @Test + public void rpop_returnsRightmostMember() { + jedis.lpush(KEY, "e1", "e2"); + String result = jedis.rpop(KEY); + assertThat(result).isEqualTo("e1"); + } + + @Test + public void rpop_removesKey_whenLastElementRemoved() { + final String keyWithTagForKeysCommand = "{tag}" + KEY; + + jedis.lpush(keyWithTagForKeysCommand, "e1"); + jedis.rpop(keyWithTagForKeysCommand); + assertThat(jedis.keys(keyWithTagForKeysCommand)).isEmpty(); Review comment: This would be better as: ``` jedis.lpush(KEY, "e1"); jedis.rpop(KEY); assertThat(jedis.exists(KEY)).isFalse(); ``` since we can directly query whether a given key exists without having to use the KEYS command. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractRPopIntegrationTest.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.assertAtMostNArgs; +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 java.util.concurrent.atomic.AtomicReference; + +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 AbstractRPopIntegrationTest implements RedisIntegrationTest { + public static final String KEY = "key"; + public static final String PREEXISTING_VALUE = "preexistingValue"; + protected JedisCluster jedis; + + @Before + public void setUp() { + jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT); + } + + @After + public void tearDown() { + flushAll(); + jedis.close(); + } + + // Overridden in RPopIntegrationTest until we implement Redis 6.2+ semantics + @Test + public void rpop_givenWrongNumOfArgs_returnsError() { + assertAtMostNArgs(jedis, Protocol.Command.RPOP, 2); + } + + @Test + public void rpop_withNonListKey_Fails() { + jedis.set("nonKey", PREEXISTING_VALUE); + assertThatThrownBy(() -> jedis.rpop("nonKey")).hasMessage(ERROR_WRONG_TYPE); + } + + @Test + public void rpop_withNonExistentKey_returnsNull() { + assertThat(jedis.rpop("nonexistentKey")).isNull(); + } + + @Test + public void rpop_returnsRightmostMember() { + jedis.lpush(KEY, "e1", "e2"); + String result = jedis.rpop(KEY); + assertThat(result).isEqualTo("e1"); + } + + @Test + public void rpop_removesKey_whenLastElementRemoved() { + final String keyWithTagForKeysCommand = "{tag}" + KEY; + + jedis.lpush(keyWithTagForKeysCommand, "e1"); + jedis.rpop(keyWithTagForKeysCommand); + assertThat(jedis.keys(keyWithTagForKeysCommand)).isEmpty(); + } + + @Test + public void rpop_removesKey_whenLastElementRemoved_multipleTimes() { + final String key = KEY; + + jedis.lpush(key, "e1"); + assertThat(jedis.rpop(key)).isEqualTo("e1"); + assertThat(jedis.rpop(key)).isNull(); + assertThat(jedis.rpop(key)).isNull(); + assertThat(jedis.exists(key)).isFalse(); + } + + @Test + public void rpop_removesElementsInCorrectOrder_onRepeatedInvocation() { + jedis.lpush(KEY, "e1"); + jedis.lpush(KEY, "e2"); + jedis.lpush(KEY, "e3"); + + String result = jedis.rpop(KEY); + assertThat(result).isEqualTo("e1"); + + result = jedis.rpop(KEY); + assertThat(result).isEqualTo("e2"); + + result = jedis.rpop(KEY); + assertThat(result).isEqualTo("e3"); Review comment: This can be simplified slightly to: ``` assertThat(jedis.rpop(KEY)).isEqualTo("e1"); assertThat(jedis.rpop(KEY)).isEqualTo("e2"); assertThat(jedis.rpop(KEY)).isEqualTo("e3"); ``` ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java ########## @@ -95,6 +95,20 @@ public int llen() { return elementList.size(); } + /** + * @param region the region this instance is stored in + * @param key the name of the set to add to Review comment: This should be "the name of the List to add to" ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractRPopIntegrationTest.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.assertAtMostNArgs; +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 java.util.concurrent.atomic.AtomicReference; + +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 AbstractRPopIntegrationTest implements RedisIntegrationTest { + public static final String KEY = "key"; + public static final String PREEXISTING_VALUE = "preexistingValue"; + protected JedisCluster jedis; + + @Before + public void setUp() { + jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT); + } + + @After + public void tearDown() { + flushAll(); + jedis.close(); + } + + // Overridden in RPopIntegrationTest until we implement Redis 6.2+ semantics + @Test + public void rpop_givenWrongNumOfArgs_returnsError() { + assertAtMostNArgs(jedis, Protocol.Command.RPOP, 2); + } + + @Test + public void rpop_withNonListKey_Fails() { + jedis.set("nonKey", PREEXISTING_VALUE); + assertThatThrownBy(() -> jedis.rpop("nonKey")).hasMessage(ERROR_WRONG_TYPE); + } + + @Test + public void rpop_withNonExistentKey_returnsNull() { + assertThat(jedis.rpop("nonexistentKey")).isNull(); + } + + @Test + public void rpop_returnsRightmostMember() { + jedis.lpush(KEY, "e1", "e2"); + String result = jedis.rpop(KEY); + assertThat(result).isEqualTo("e1"); + } + + @Test + public void rpop_removesKey_whenLastElementRemoved() { + final String keyWithTagForKeysCommand = "{tag}" + KEY; + + jedis.lpush(keyWithTagForKeysCommand, "e1"); + jedis.rpop(keyWithTagForKeysCommand); + assertThat(jedis.keys(keyWithTagForKeysCommand)).isEmpty(); + } + + @Test + public void rpop_removesKey_whenLastElementRemoved_multipleTimes() { + final String key = KEY; + + jedis.lpush(key, "e1"); + assertThat(jedis.rpop(key)).isEqualTo("e1"); + assertThat(jedis.rpop(key)).isNull(); + assertThat(jedis.rpop(key)).isNull(); + assertThat(jedis.exists(key)).isFalse(); + } + + @Test + public void rpop_removesElementsInCorrectOrder_onRepeatedInvocation() { + jedis.lpush(KEY, "e1"); + jedis.lpush(KEY, "e2"); + jedis.lpush(KEY, "e3"); + + String result = jedis.rpop(KEY); + assertThat(result).isEqualTo("e1"); + + result = jedis.rpop(KEY); + assertThat(result).isEqualTo("e2"); + + result = jedis.rpop(KEY); + assertThat(result).isEqualTo("e3"); + } + + @Test + public void rpop_removesElementsInCorrectOrder_givenMultipleElements() { Review comment: This test is functionally identical to the one above, as the only difference is that here we call LPUSH twice. Since this test is not intended to be testing the behaviour of LPUSH, it should be removed. ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java ########## @@ -142,6 +156,14 @@ public int getDSFID() { return elementList.remove(index); } + public synchronized byte[] elementRemoveFirst() { + return elementList.removeFirst(); + } + + public synchronized byte[] elementRemoveLast() { Review comment: These methods might be better named "removeFirstElement" and "removeLastElement". Also, since we now have a method to remove specifically the first element, we can call that in `lpop` rather than `elementRemove(0)`. ########## File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LPopDUnitTest.java ########## @@ -141,15 +141,16 @@ private void lpushPerformAndVerify(String key, List<String> elementList) { private void lpopPerformAndVerify(String key, AtomicLong runningCount) { assertThat(jedis.llen(key)).isEqualTo(INITIAL_LIST_SIZE); - assertThat(jedis.llen(key)).isEqualTo(INITIAL_LIST_SIZE); int elementCount = INITIAL_LIST_SIZE - 1; while (jedis.llen(key) > 0 && runningCount.get() > 0) { - String expected = key + "-" + (elementCount - 1) + "-"; + String expected = "-" + key + "-" + (elementCount) + "-"; Review comment: It would be better to call `makeElementString()` here so that we can guarantee that the expected String is constructed in exactly the same way as the String that was pushed into the list. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractRPopIntegrationTest.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.assertAtMostNArgs; +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 java.util.concurrent.atomic.AtomicReference; + +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 AbstractRPopIntegrationTest implements RedisIntegrationTest { + public static final String KEY = "key"; + public static final String PREEXISTING_VALUE = "preexistingValue"; + protected JedisCluster jedis; + + @Before + public void setUp() { + jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT); + } + + @After + public void tearDown() { + flushAll(); + jedis.close(); + } + + // Overridden in RPopIntegrationTest until we implement Redis 6.2+ semantics + @Test + public void rpop_givenWrongNumOfArgs_returnsError() { + assertAtMostNArgs(jedis, Protocol.Command.RPOP, 2); + } + + @Test + public void rpop_withNonListKey_Fails() { + jedis.set("nonKey", PREEXISTING_VALUE); + assertThatThrownBy(() -> jedis.rpop("nonKey")).hasMessage(ERROR_WRONG_TYPE); + } + + @Test + public void rpop_withNonExistentKey_returnsNull() { + assertThat(jedis.rpop("nonexistentKey")).isNull(); + } + + @Test + public void rpop_returnsRightmostMember() { + jedis.lpush(KEY, "e1", "e2"); + String result = jedis.rpop(KEY); + assertThat(result).isEqualTo("e1"); + } + + @Test + public void rpop_removesKey_whenLastElementRemoved() { + final String keyWithTagForKeysCommand = "{tag}" + KEY; + + jedis.lpush(keyWithTagForKeysCommand, "e1"); + jedis.rpop(keyWithTagForKeysCommand); + assertThat(jedis.keys(keyWithTagForKeysCommand)).isEmpty(); + } + + @Test + public void rpop_removesKey_whenLastElementRemoved_multipleTimes() { + final String key = KEY; Review comment: This variable is redundant. ########## File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/RPopDUnitTest.java ########## @@ -0,0 +1,189 @@ +/* + * 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 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 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 RPopDUnitTest { + public static final int INITIAL_LIST_SIZE = 10; + + @Rule + public RedisClusterStartupRule clusterStartUp = new RedisClusterStartupRule(); + + @Rule + public ExecutorServiceRule executor = new ExecutorServiceRule(); + + private static JedisCluster jedis; + + @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 shouldPropagateDeltasAmongCluster_andRetainDataAfterServerCrash() { + String key = makeListKeyWithHashtag(1, clusterStartUp.getKeyOnServer("rpop", 1)); + List<String> elementList = makeElementList(key, INITIAL_LIST_SIZE); + lpushPerformAndVerify(key, elementList); + + // Remove all but first element + for (int i = 0; i < INITIAL_LIST_SIZE - 1; i++) { + assertThat(jedis.rpop(key)).isEqualTo(makeElementString(key, i)); + } + + clusterStartUp.crashVM(1); // kill primary server + + assertThat(jedis.llen(key)).isEqualTo(1); + assertThat(jedis.rpop(key)).isEqualTo(elementList.get(elementList.size() - 1)); + assertThat(jedis.exists(key)).isFalse(); + } + + @Test + public void givenBucketsMoveDuringRpop_thenOperationsAreNotLost() throws Exception { + AtomicLong runningCount = new AtomicLong(3); + 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 = + () -> rpopPerformAndVerify(keys.get(0), runningCount); + Runnable task2 = + () -> rpopPerformAndVerify(keys.get(1), runningCount); + Runnable task3 = + () -> rpopPerformAndVerify(keys.get(2), runningCount); + + Future<Void> future1 = executor.runAsync(task1); + Future<Void> future2 = executor.runAsync(task2); + Future<Void> future3 = executor.runAsync(task3); + + for (int i = 0; i < 50 && runningCount.get() > 0; i++) { + clusterStartUp.moveBucketForKey(listHashtags.get(i % listHashtags.size())); + Thread.sleep(500); + } + + runningCount.set(0); + + future1.get(); + future2.get(); + future3.get(); + } + + private List<String> makeListHashtags() { + List<String> listHashtags = new ArrayList<>(); + listHashtags.add(clusterStartUp.getKeyOnServer("rpop", 1)); + listHashtags.add(clusterStartUp.getKeyOnServer("rpop", 2)); + listHashtags.add(clusterStartUp.getKeyOnServer("rpop", 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 lpushPerformAndVerify(String key, List<String> elementList) { + jedis.lpush(key, elementList.toArray(new String[] {})); + + Long listLength = jedis.llen(key); + assertThat(listLength).as("Initial list lengths not equal for key %s'", key) + .isEqualTo(elementList.size()); + } + + private void rpopPerformAndVerify(String key, AtomicLong runningCount) { + assertThat(jedis.llen(key)).isEqualTo(INITIAL_LIST_SIZE); + + int elementCount = 0; + int elementListSize = INITIAL_LIST_SIZE - 1; + while (jedis.llen(key) > 0 && elementCount <= elementListSize && runningCount.get() > 0) { + String expected = "-" + key + "-" + elementCount + "-"; + String element = expected; + try { + element = jedis.rpop(key); + if (element.equals(expected)) { + elementCount++; + } else { + int expectedCount = elementCount; + String[] chunks = element.split("-"); + elementCount = Integer.parseInt(chunks[4]); + int actual = elementCount - expectedCount; + assertThat(actual).isLessThanOrEqualTo(1); Review comment: This logic is different from the logic in the equivalent method in `LPopDUnitTest`, in that here we're saying that the actual element count of the popped element should not differ from the expected count by more than one, whereas in `LPopDUnitTest`, we have that the expected count is one less than the current element count AND that the actual element of the popped element should not differ from the expected count by more than one. This means that in the LPOP test, we actually tolerate differences of 1 - 2 in element count rather than 1 - 0 as we do in this test. I think that to make it clearer exactly what we're expecting, the logic in both tests could be modified to be something like: ``` // Allow the actual count to be exactly one greater than the expected count int allowedCount = elementCount + 1; String[] chunks = element.split("-"); int actualCount = Integer.parseInt(chunks[4]); assertThat(actualCount).isEqualTo(allowedCount); // Set elementCount for the next element elementCount = actualCount + 1; ``` The final line here is important, as without it, `elementCount` will still be off by 1 on the next iteration of the loop, since we need to both increment elementCount each loop AND skip one element in the event that we got a duplicated pop due to client retries. Without this adjustment, if we ever got two duplicated pops in the same test, the test would fail, despite this logic being intended to make the test tolerant of client retries. Alternately, this logic could be refactored to avoid the use of an if statement and using `satisfiesAnyOf()` instead: ``` try { element = jedis.rpop(key); String[] chunks = element.split("-"); int actualCount = Integer.parseInt(chunks[4]); int expectedCount = elementCount; assertThat(actualCount) .as("Actual element count did not match expected element count for element " + element) .satisfiesAnyOf( count -> assertThat(count).isEqualTo(expectedCount), count -> assertThat(count).isEqualTo(expectedCount + 1)); elementCount = actualCount + 1; } catch (Exception ex) { ``` -- 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
