ringles commented on a change in pull request #7403: URL: https://github.com/apache/geode/pull/7403#discussion_r839000434
########## File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LTrimDUnitTest.java ########## @@ -0,0 +1,181 @@ +/* + * 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.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 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 LTrimDUnitTest { + public static final int INITIAL_LIST_SIZE = 5_000; + + @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 shouldDistributeDataAmongCluster_andRetainDataAfterServerCrash() { + String key = makeListKeyWithHashtag(1, clusterStartUp.getKeyOnServer("ltrim", 1)); + List<String> elementList = makeElementList(key, INITIAL_LIST_SIZE); + lpushPerformAndVerify(key, elementList); + + // Remove all but last element + jedis.ltrim(key, INITIAL_LIST_SIZE - 1, INITIAL_LIST_SIZE); + + clusterStartUp.crashVM(1); // kill primary server + + assertThat(jedis.lindex(key, 0)).isEqualTo(elementList.get(0)); + jedis.ltrim(key, 0, -2); + assertThat(jedis.exists(key)).isFalse(); + } + + @Test + public void givenBucketsMoveDuringLtrim_thenOperationsAreNotLost() throws Exception { + AtomicBoolean isRunning = 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); + + Runnable task1 = + () -> ltrimPerformAndVerify(keys.get(0), isRunning, elementList1); + Runnable task2 = + () -> ltrimPerformAndVerify(keys.get(1), isRunning, elementList2); + Runnable task3 = + () -> ltrimPerformAndVerify(keys.get(2), isRunning, elementList3); + + Future<Void> future1 = executor.runAsync(task1); + Future<Void> future2 = executor.runAsync(task2); + Future<Void> future3 = executor.runAsync(task3); + + for (int i = 0; i < 100; i++) { Review comment: Conditionalized. ########## File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LTrimDUnitTest.java ########## @@ -0,0 +1,181 @@ +/* + * 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.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 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 LTrimDUnitTest { + public static final int INITIAL_LIST_SIZE = 5_000; + + @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 shouldDistributeDataAmongCluster_andRetainDataAfterServerCrash() { + String key = makeListKeyWithHashtag(1, clusterStartUp.getKeyOnServer("ltrim", 1)); + List<String> elementList = makeElementList(key, INITIAL_LIST_SIZE); + lpushPerformAndVerify(key, elementList); + + // Remove all but last element + jedis.ltrim(key, INITIAL_LIST_SIZE - 1, INITIAL_LIST_SIZE); + + clusterStartUp.crashVM(1); // kill primary server + + assertThat(jedis.lindex(key, 0)).isEqualTo(elementList.get(0)); + jedis.ltrim(key, 0, -2); + assertThat(jedis.exists(key)).isFalse(); + } + + @Test + public void givenBucketsMoveDuringLtrim_thenOperationsAreNotLost() throws Exception { + AtomicBoolean isRunning = 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); + + Runnable task1 = + () -> ltrimPerformAndVerify(keys.get(0), isRunning, elementList1); + Runnable task2 = + () -> ltrimPerformAndVerify(keys.get(1), isRunning, elementList2); + Runnable task3 = + () -> ltrimPerformAndVerify(keys.get(2), isRunning, elementList3); + + Future<Void> future1 = executor.runAsync(task1); + Future<Void> future2 = executor.runAsync(task2); + Future<Void> future3 = executor.runAsync(task3); + + for (int i = 0; i < 100; i++) { + clusterStartUp.moveBucketForKey(listHashtags.get(i % listHashtags.size())); + Thread.sleep(200); + } + + isRunning.set(false); + + future1.get(); + future2.get(); + future3.get(); + } + + private List<String> makeListHashtags() { + List<String> listHashtags = new ArrayList<>(); + listHashtags.add(clusterStartUp.getKeyOnServer("ltrim", 1)); + listHashtags.add(clusterStartUp.getKeyOnServer("ltrim", 2)); + listHashtags.add(clusterStartUp.getKeyOnServer("ltrim", 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 ltrimPerformAndVerify(String key, + AtomicBoolean isRunning, + List<String> elementList) { + while (isRunning.get()) { + lpushPerformAndVerify(key, elementList); + + for (int i = 1; i <= INITIAL_LIST_SIZE / 2 && isRunning.get(); i++) { + long lastIndex = jedis.llen(key) - 2; + try { + assertThat(jedis.lindex(key, 0)) + .as("lpush head verification failed at iteration " + i) + .isEqualTo(makeElementString(key, INITIAL_LIST_SIZE - i)); + assertThat(jedis.lindex(key, lastIndex)) + .as("lpush tail verification failed at iteration " + i) + .isEqualTo(makeElementString(key, i)); + jedis.ltrim(key, 1, lastIndex); + assertThat(jedis.llen(key)).as("Key: %s ", key).isEqualTo(lastIndex); + } catch (Throwable ex) { + isRunning.set(false); // test is over + throw ex; + } + } + if (isRunning.get()) { + assertThat(jedis.exists(key)).isFalse(); + } Review comment: Reworked. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLTrimIntegrationTest.java ########## @@ -0,0 +1,160 @@ +/* + * 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_NOT_INTEGER; +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.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; + +import junitparams.Parameters; +import junitparams.naming.TestCaseName; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +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; +import org.apache.geode.test.junit.runners.GeodeParamsRunner; + +@RunWith(GeodeParamsRunner.class) +public abstract class AbstractLTrimIntegrationTest implements RedisIntegrationTest { + public static final String KEY = "key"; + public static final String PREEXISTING_VALUE = "preexistingValue"; + 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 givenWrongNumOfArgs_returnsError() { + assertExactNumberOfArgs(jedis, Protocol.Command.LTRIM, 3); + } + + @Test + public void withNonListKey_Fails() { + jedis.set("string", PREEXISTING_VALUE); + assertThatThrownBy(() -> jedis.ltrim("string", 0, -1)) + .hasMessage(ERROR_WRONG_TYPE); + } + + @Test + public void withNonExistentKey_returnsOK() { + assertThat(jedis.ltrim("nonexistent", 0, -1)).isEqualTo("OK"); + } + + @Test + public void withNonIntegerRangeSpecifier_Fails() { + jedis.lpush(KEY, "e1", "e2", "e3", "e4"); + + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LTRIM, KEY, + "0", "not-an-integer")) + .hasMessage(ERROR_NOT_INTEGER); + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LTRIM, KEY, + "not-an-integer", "-1")) + .hasMessage(ERROR_NOT_INTEGER); + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LTRIM, KEY, + "not-an-integer", "not-an-integer")) + .hasMessage(ERROR_NOT_INTEGER); + } + + @Test + @Parameters(method = "getValidRanges") + @TestCaseName("{method}: start:{0}, end:{1}, expected:{2}") + public void trimsToSpecifiedRange_givenValidRange(long start, long end, String[] expected) { + initializeTestList(); + + jedis.ltrim(KEY, start, end); + assertThat(jedis.lrange(KEY, 0, -1)).containsExactly(expected); + } + + // @Parameterized.Parameters(name = "start:{0}, end:{1}, expected:{2}") + @SuppressWarnings("unused") + private Object[] getValidRanges() { + // Values are start, end, expected result + // For initial list of {e4, e3, e2, e1} + return new Object[] { + new Object[] {0L, 0L, new String[] {"e4"}}, + new Object[] {0L, 1L, new String[] {"e4", "e3"}}, + new Object[] {0L, 2L, new String[] {"e4", "e3", "e2"}}, + new Object[] {1L, 2L, new String[] {"e3", "e2"}}, + new Object[] {1L, -1L, new String[] {"e3", "e2", "e1"}}, + new Object[] {1L, -2L, new String[] {"e3", "e2"}}, + new Object[] {-2L, -1L, new String[] {"e2", "e1"}}, + new Object[] {-1L, -1L, new String[] {"e1"}}, + new Object[] {0L, 3L, new String[] {"e4", "e3", "e2", "e1"}}, + new Object[] {2L, 3L, new String[] {"e2", "e1"}}, + new Object[] {3L, 4L, new String[] {"e1"}}, + new Object[] {0L, 4L, new String[] {"e4", "e3", "e2", "e1"}}, + new Object[] {0L, 10L, new String[] {"e4", "e3", "e2", "e1"}}, + new Object[] {-5L, -1L, new String[] {"e4", "e3", "e2", "e1"}}, + new Object[] {-10L, 10L, new String[] {"e4", "e3", "e2", "e1"}} + }; + } + + private void initializeTestList() { + jedis.del(KEY); + jedis.lpush(KEY, "e1", "e2", "e3", "e4"); + } + + @Test + public void removesKey_whenAllElementsTrimmed() { + initializeTestList(); + + jedis.ltrim(KEY, 0, -5); Review comment: Parameterized. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLTrimIntegrationTest.java ########## @@ -0,0 +1,160 @@ +/* + * 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_NOT_INTEGER; +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.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; + +import junitparams.Parameters; +import junitparams.naming.TestCaseName; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +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; +import org.apache.geode.test.junit.runners.GeodeParamsRunner; + +@RunWith(GeodeParamsRunner.class) +public abstract class AbstractLTrimIntegrationTest implements RedisIntegrationTest { + public static final String KEY = "key"; + public static final String PREEXISTING_VALUE = "preexistingValue"; + 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 givenWrongNumOfArgs_returnsError() { + assertExactNumberOfArgs(jedis, Protocol.Command.LTRIM, 3); + } + + @Test + public void withNonListKey_Fails() { + jedis.set("string", PREEXISTING_VALUE); + assertThatThrownBy(() -> jedis.ltrim("string", 0, -1)) + .hasMessage(ERROR_WRONG_TYPE); + } + + @Test + public void withNonExistentKey_returnsOK() { + assertThat(jedis.ltrim("nonexistent", 0, -1)).isEqualTo("OK"); + } + + @Test + public void withNonIntegerRangeSpecifier_Fails() { + jedis.lpush(KEY, "e1", "e2", "e3", "e4"); + + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LTRIM, KEY, + "0", "not-an-integer")) + .hasMessage(ERROR_NOT_INTEGER); + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LTRIM, KEY, + "not-an-integer", "-1")) + .hasMessage(ERROR_NOT_INTEGER); + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LTRIM, KEY, + "not-an-integer", "not-an-integer")) + .hasMessage(ERROR_NOT_INTEGER); + } + + @Test + @Parameters(method = "getValidRanges") + @TestCaseName("{method}: start:{0}, end:{1}, expected:{2}") + public void trimsToSpecifiedRange_givenValidRange(long start, long end, String[] expected) { + initializeTestList(); + + jedis.ltrim(KEY, start, end); + assertThat(jedis.lrange(KEY, 0, -1)).containsExactly(expected); + } + + // @Parameterized.Parameters(name = "start:{0}, end:{1}, expected:{2}") Review comment: Good catch, yanked. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLTrimIntegrationTest.java ########## @@ -0,0 +1,160 @@ +/* + * 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_NOT_INTEGER; +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.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; + +import junitparams.Parameters; +import junitparams.naming.TestCaseName; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +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; +import org.apache.geode.test.junit.runners.GeodeParamsRunner; + +@RunWith(GeodeParamsRunner.class) +public abstract class AbstractLTrimIntegrationTest implements RedisIntegrationTest { + public static final String KEY = "key"; + public static final String PREEXISTING_VALUE = "preexistingValue"; Review comment: Inlined. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLTrimIntegrationTest.java ########## @@ -0,0 +1,160 @@ +/* + * 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_NOT_INTEGER; +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.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; + +import junitparams.Parameters; +import junitparams.naming.TestCaseName; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +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; +import org.apache.geode.test.junit.runners.GeodeParamsRunner; + +@RunWith(GeodeParamsRunner.class) +public abstract class AbstractLTrimIntegrationTest implements RedisIntegrationTest { + public static final String KEY = "key"; + public static final String PREEXISTING_VALUE = "preexistingValue"; + 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 givenWrongNumOfArgs_returnsError() { + assertExactNumberOfArgs(jedis, Protocol.Command.LTRIM, 3); + } + + @Test + public void withNonListKey_Fails() { + jedis.set("string", PREEXISTING_VALUE); + assertThatThrownBy(() -> jedis.ltrim("string", 0, -1)) + .hasMessage(ERROR_WRONG_TYPE); + } + + @Test + public void withNonExistentKey_returnsOK() { + assertThat(jedis.ltrim("nonexistent", 0, -1)).isEqualTo("OK"); + } + + @Test + public void withNonIntegerRangeSpecifier_Fails() { + jedis.lpush(KEY, "e1", "e2", "e3", "e4"); + + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LTRIM, KEY, + "0", "not-an-integer")) + .hasMessage(ERROR_NOT_INTEGER); + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LTRIM, KEY, + "not-an-integer", "-1")) + .hasMessage(ERROR_NOT_INTEGER); + assertThatThrownBy(() -> jedis.sendCommand(KEY, Protocol.Command.LTRIM, KEY, + "not-an-integer", "not-an-integer")) + .hasMessage(ERROR_NOT_INTEGER); + } + + @Test + @Parameters(method = "getValidRanges") + @TestCaseName("{method}: start:{0}, end:{1}, expected:{2}") + public void trimsToSpecifiedRange_givenValidRange(long start, long end, String[] expected) { + initializeTestList(); + + jedis.ltrim(KEY, start, end); + assertThat(jedis.lrange(KEY, 0, -1)).containsExactly(expected); + } + + // @Parameterized.Parameters(name = "start:{0}, end:{1}, expected:{2}") + @SuppressWarnings("unused") + private Object[] getValidRanges() { + // Values are start, end, expected result + // For initial list of {e4, e3, e2, e1} + return new Object[] { + new Object[] {0L, 0L, new String[] {"e4"}}, + new Object[] {0L, 1L, new String[] {"e4", "e3"}}, + new Object[] {0L, 2L, new String[] {"e4", "e3", "e2"}}, + new Object[] {1L, 2L, new String[] {"e3", "e2"}}, + new Object[] {1L, -1L, new String[] {"e3", "e2", "e1"}}, + new Object[] {1L, -2L, new String[] {"e3", "e2"}}, + new Object[] {-2L, -1L, new String[] {"e2", "e1"}}, + new Object[] {-1L, -1L, new String[] {"e1"}}, + new Object[] {0L, 3L, new String[] {"e4", "e3", "e2", "e1"}}, + new Object[] {2L, 3L, new String[] {"e2", "e1"}}, + new Object[] {3L, 4L, new String[] {"e1"}}, + new Object[] {0L, 4L, new String[] {"e4", "e3", "e2", "e1"}}, + new Object[] {0L, 10L, new String[] {"e4", "e3", "e2", "e1"}}, + new Object[] {-5L, -1L, new String[] {"e4", "e3", "e2", "e1"}}, + new Object[] {-10L, 10L, new String[] {"e4", "e3", "e2", "e1"}} + }; + } + + private void initializeTestList() { + jedis.del(KEY); Review comment: Simplified. ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/collections/SizeableByteArrayList.java ########## @@ -83,6 +83,39 @@ return indexesRemoved; } + public void clearSublist(int fromIndex, int toIndex) { + if (fromIndex < size() / 2) { + clearFromBeginning(fromIndex, toIndex); + } else { + clearFromEnd(fromIndex, toIndex); + } + } + + public void clearFromBeginning(int fromIndex, int toIndex) { Review comment: Privatized. ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/collections/SizeableByteArrayList.java ########## @@ -83,6 +83,39 @@ return indexesRemoved; } + public void clearSublist(int fromIndex, int toIndex) { + if (fromIndex < size() / 2) { + clearFromBeginning(fromIndex, toIndex); + } else { + clearFromEnd(fromIndex, toIndex); + } + } + + public void clearFromBeginning(int fromIndex, int toIndex) { + ListIterator<byte[]> iterator = listIterator(fromIndex); + int removeCount = toIndex - fromIndex; + int count = 0; + + while (iterator.hasNext() && count < removeCount) { + byte[] element = iterator.next(); + iterator.remove(); + count++; + memberOverhead -= calculateByteArrayOverhead(element); + } + } + + private void clearFromEnd(int fromIndex, int toIndex) { + ListIterator<byte[]> descendingIterator = listIterator(toIndex); + int removedCount = toIndex - fromIndex; + + while (descendingIterator.hasPrevious() && removedCount > 0) { + byte[] element = descendingIterator.previous(); + descendingIterator.remove(); + removedCount--; + memberOverhead -= calculateByteArrayOverhead(element); + } + } Review comment: Updated. ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/collections/SizeableByteArrayList.java ########## @@ -83,6 +83,39 @@ return indexesRemoved; } + public void clearSublist(int fromIndex, int toIndex) { + if (fromIndex < size() / 2) { Review comment: Optimized. ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java ########## @@ -252,6 +253,63 @@ public void lset(Region<RedisKey, RedisData> region, RedisKey key, int index, by storeChanges(region, key, new ReplaceByteArrayAtOffset(index, value)); } + /** + * @param start the index of the first element to retain + * @param end the index of the last element to retain + * @param region the region this instance is stored in + * @param key the name of the list to pop from + * @return the element actually popped + */ + public byte[] ltrim(long start, long end, Region<RedisKey, RedisData> region, + RedisKey key) { + int length = elementList.size(); + int boundedStart = getBoundedStartIndex(start, length); + int boundedEnd = getBoundedEndIndex(end, length); + + if (boundedStart > boundedEnd || boundedStart == length) { + // Remove everything + region.remove(key); + return null; + } + + if (boundedStart == 0 && boundedEnd == length) { + // No-op, return without modifying the list + return null; + } + + RetainElementsByIndexRange retainElementsByRange; + synchronized (this) { + if (boundedEnd < length) { + // trim stuff at end of list + elementList.clearSublist(boundedEnd + 1, length); + } + if (boundedStart > 0) { + // trim stuff at start of list + elementList.clearSublist(0, boundedStart); + } Review comment: De-duped. ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java ########## @@ -252,6 +253,63 @@ public void lset(Region<RedisKey, RedisData> region, RedisKey key, int index, by storeChanges(region, key, new ReplaceByteArrayAtOffset(index, value)); } + /** + * @param start the index of the first element to retain + * @param end the index of the last element to retain + * @param region the region this instance is stored in + * @param key the name of the list to pop from + * @return the element actually popped + */ + public byte[] ltrim(long start, long end, Region<RedisKey, RedisData> region, + RedisKey key) { + int length = elementList.size(); + int boundedStart = getBoundedStartIndex(start, length); + int boundedEnd = getBoundedEndIndex(end, length); + + if (boundedStart > boundedEnd || boundedStart == length) { + // Remove everything + region.remove(key); + return null; + } + + if (boundedStart == 0 && boundedEnd == length) { + // No-op, return without modifying the list + return null; + } + + RetainElementsByIndexRange retainElementsByRange; + synchronized (this) { + if (boundedEnd < length) { + // trim stuff at end of list + elementList.clearSublist(boundedEnd + 1, length); + } + if (boundedStart > 0) { + // trim stuff at start of list + elementList.clearSublist(0, boundedStart); + } + retainElementsByRange = + new RetainElementsByIndexRange(incrementAndGetVersion(), boundedStart, boundedEnd); + } + storeChanges(region, key, retainElementsByRange); + return null; + } + + private int getBoundedStartIndex(long index, int size) { + if (index >= 0L) { + return (int) Math.min(index, size); + } else { + return (int) Math.max(index + size, 0); + } + } + + private int getBoundedEndIndex(long index, int size) { + if (index >= 0L) { + return (int) Math.min(index, size); + } else { + return (int) Math.max(index + size, -1); + } Review comment: Updated. ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java ########## @@ -252,6 +253,63 @@ public void lset(Region<RedisKey, RedisData> region, RedisKey key, int index, by storeChanges(region, key, new ReplaceByteArrayAtOffset(index, value)); } + /** + * @param start the index of the first element to retain + * @param end the index of the last element to retain + * @param region the region this instance is stored in + * @param key the name of the list to pop from + * @return the element actually popped + */ + public byte[] ltrim(long start, long end, Region<RedisKey, RedisData> region, + RedisKey key) { + int length = elementList.size(); + int boundedStart = getBoundedStartIndex(start, length); + int boundedEnd = getBoundedEndIndex(end, length); + + if (boundedStart > boundedEnd || boundedStart == length) { + // Remove everything + region.remove(key); + return null; Review comment: Done. -- 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
