DonalEvans commented on a change in pull request #7261: URL: https://github.com/apache/geode/pull/7261#discussion_r794096487
########## File path: geode-for-redis/src/test/java/org/apache/geode/redis/internal/data/collections/SizeableByteArrayListTest.java ########## @@ -0,0 +1,72 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package org.apache.geode.redis.internal.data.collections; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.Before; +import org.junit.Test; + +import org.apache.geode.cache.util.ObjectSizer; +import org.apache.geode.internal.size.ReflectionObjectSizer; + +public class SizeableByteArrayListTest { + private final ObjectSizer sizer = ReflectionObjectSizer.getInstance(); + + @Before + public void setup() {} Review comment: This method can be removed. ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/collections/SizeableByteArrayList.java ########## @@ -0,0 +1,128 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package org.apache.geode.redis.internal.data.collections; + +import static org.apache.geode.internal.JvmSizeUtils.getObjectHeaderSize; +import static org.apache.geode.internal.JvmSizeUtils.getReferenceSize; +import static org.apache.geode.internal.JvmSizeUtils.memoryOverhead; +import static org.apache.geode.internal.JvmSizeUtils.roundUpSize; + +import java.util.Arrays; +import java.util.LinkedList; +import java.util.ListIterator; + +import org.apache.geode.internal.size.Sizeable; + +public class SizeableByteArrayList extends LinkedList<byte[]> implements Sizeable { + private static final int BYTE_ARRAY_LIST_OVERHEAD = memoryOverhead(SizeableByteArrayList.class); + private static final int NODE_OVERHEAD = + roundUpSize(getObjectHeaderSize() + 3 * getReferenceSize()); + private static final int BYTE_ARRAY_BASE_OVERHEAD = 16; + private int memberOverhead; + + @Override + public int indexOf(Object o) { + ListIterator<byte[]> iterator = this.listIterator(); + while (iterator.hasNext()) { + int index = iterator.nextIndex(); + byte[] element = iterator.next(); + if (Arrays.equals(element, (byte[]) o)) { + return index; + } + } + return -1; + } + + @Override + public int lastIndexOf(Object o) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean remove(Object o) { + int index = indexOf(o); + if (index == -1) { + return false; + } + memberOverhead -= calculateByteArrayOverhead((byte[]) o); + remove(index); + return true; + } + + @Override + public byte[] remove(int index) { + byte[] element = super.remove(index); + memberOverhead -= calculateByteArrayOverhead(element); + return element; + } + + @Override + public void addFirst(byte[] element) { + memberOverhead += calculateByteArrayOverhead(element); + super.addFirst(element); + } + + @Override + public void addLast(byte[] element) { + memberOverhead += calculateByteArrayOverhead(element); + super.addLast(element); + } + + public boolean removeLastOccurrence(Object o) { + throw new UnsupportedOperationException(); + } + + private int calculateByteArrayOverhead(byte[] element) { + return BYTE_ARRAY_BASE_OVERHEAD + (element.length % 8 == 0 ? 0 : 8) + + NODE_OVERHEAD + (element.length / 8) * 8; + } + + @Override + public int getSizeInBytes() { + return BYTE_ARRAY_LIST_OVERHEAD + memberOverhead; + } + + @Override + public int hashCode() { + final int PRIME_NUMBER = 31; + int hashCode = 1; + ListIterator<byte[]> iterator = this.listIterator(); + while (iterator.hasNext()) { + int index = iterator.nextIndex() + 1; + hashCode = hashCode * (PRIME_NUMBER % index) + Arrays.hashCode(iterator.next()); Review comment: The `(PRIME_NUMBER % index)` part of this hashCode calculation will be the same for all lists, so it seems unnecessary. The hashCode method should then become: ``` public int hashCode() { final int primeNumber = 31; int hashCode = 1; for (byte[] bytes : this) { hashCode = primeNumber * hashCode + Arrays.hashCode(bytes); } return hashCode; } ``` ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLPopIntegrationTest.java ########## @@ -0,0 +1,113 @@ +/* + * 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.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.AtomicLong; + +import org.assertj.core.api.AssertionsForClassTypes; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import redis.clients.jedis.HostAndPort; +import redis.clients.jedis.JedisCluster; + +import org.apache.geode.redis.ConcurrentLoopingThreads; +import org.apache.geode.redis.RedisIntegrationTest; + +public abstract class AbstractLPopIntegrationTest implements RedisIntegrationTest { + public static final String KEY = "key"; + public static final String PREEXISTING_VALUE = "preexistingValue"; + // TODO: make private when we implement Redis 6.2+ behavior for LPOP + public JedisCluster jedis; + + @Before + public void setUp() { + jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT); + } + + @After + public void tearDown() { + flushAll(); + jedis.close(); + } + + // not checking LPOP argument count here, see LPopIntegrationTest + + @Test + public void lpop_withStringFails() { + jedis.set("string", PREEXISTING_VALUE); + assertThatThrownBy(() -> jedis.lpop("string")).hasMessageContaining(ERROR_WRONG_TYPE); + } + + @Test + public void lpop_withNonExistentKey_returnsNull() { + assertThat(jedis.lpop("nonexistent")).isNull(); + } + + @Test + public void lpop_returnsOneMember() { + jedis.lpush(KEY, "e1", "e2"); + String result = jedis.lpop(KEY); + assertThat(result).isEqualTo("e2"); + } + + @Test + public void lpop_removesKey_whenLastElementRemoved() { + final String KEY_WITH_TAG = "{tag}" + KEY; + + jedis.lpush(KEY_WITH_TAG, "e1"); + jedis.lpop(KEY_WITH_TAG); + assertThat(jedis.keys(KEY_WITH_TAG)).isEmpty(); + } + + @Test + public void lpop_removesKey_whenLastElementRemoved_multipleTimes() { + final String KEY_WITH_TAG = "{tag}" + KEY; + + jedis.lpush(KEY_WITH_TAG, "e1"); + jedis.lpop(KEY_WITH_TAG); + jedis.lpop(KEY_WITH_TAG); + jedis.lpop(KEY_WITH_TAG); + assertThat(jedis.keys(KEY_WITH_TAG)).isEmpty(); + } + + @Test + public void lpop_withConcurrentLPush_returnsCorrectValue() { + String[] valuesInitial = new String[] {"one", "two", "three"}; + String[] valuesToAdd = new String[] {"pear", "apple", "plum", "orange", "peach"}; + jedis.lpush(KEY, valuesInitial); + + final AtomicLong lpopReference = new AtomicLong(); + new ConcurrentLoopingThreads(1000, + i -> jedis.lpush(KEY, valuesToAdd), + i -> lpopReference.set(jedis.llen(KEY))) + .runWithAction(() -> { + AssertionsForClassTypes.assertThat(lpopReference).satisfiesAnyOf( + lpopResult -> AssertionsForClassTypes.assertThat(lpopResult.get()) + .isEqualTo(valuesInitial.length), + lpopResult -> AssertionsForClassTypes.assertThat(lpopResult.get()) + .isEqualTo(valuesInitial.length + valuesToAdd.length)); Review comment: This test should be doing an LPOP, not LLEN, and the assertions to should be adjusted accordingly. Additionally, `AssertionsForClassTypes` should not be used, but rather just `Assertions`. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/data/AbstractMemoryOverheadIntegrationTest.java ########## @@ -239,7 +276,7 @@ private void measureAndCheckPerEntryOverhead(AddEntryFunction addEntry, Measurem // Put some entries to make sure we initialize any constant size data structures. We are // just trying to measure the cost of each add entry operation. for (int i = 0; i < WARM_UP_ENTRY_COUNT; i++) { - String uniqueString = String.format("warmup-%10d", i); + String uniqueString = String.format("warmup-%010d", i); Review comment: Was this change intentional? What's the purpose of it? ########## File path: geode-for-redis/src/main/resources/org/apache/geode/redis/internal/services/sanctioned-geode-for-redis-serializables.txt ########## @@ -3,6 +3,7 @@ org/apache/geode/redis/internal/commands/executor/BaseSetOptions$Exists,false org/apache/geode/redis/internal/commands/parameters/RedisParametersMismatchException,true,-643700717871858072 org/apache/geode/redis/internal/data/RedisDataType,false,nullType:org/apache/geode/redis/internal/data/RedisData,toStringValue:java/lang/String org/apache/geode/redis/internal/data/RedisSortedSet$MemberAddResult,false +org/apache/geode/redis/internal/data/collections/SizeableByteArrayList,false,memberOverhead:int Review comment: This should be in `excluded-classes` instead of this file, as we never expect to need to serialize or deserialize `SizeableByteArrayList`. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLPopIntegrationTest.java ########## @@ -0,0 +1,113 @@ +/* + * 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.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.AtomicLong; + +import org.assertj.core.api.AssertionsForClassTypes; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import redis.clients.jedis.HostAndPort; +import redis.clients.jedis.JedisCluster; + +import org.apache.geode.redis.ConcurrentLoopingThreads; +import org.apache.geode.redis.RedisIntegrationTest; + +public abstract class AbstractLPopIntegrationTest implements RedisIntegrationTest { + public static final String KEY = "key"; + public static final String PREEXISTING_VALUE = "preexistingValue"; + // TODO: make private when we implement Redis 6.2+ behavior for LPOP + public JedisCluster jedis; Review comment: This can be package private instead of public. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLLenIntegrationTest.java ########## @@ -0,0 +1,124 @@ +/* + * 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_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.AtomicLong; + +import org.assertj.core.api.AssertionsForClassTypes; +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 AbstractLLenIntegrationTest 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 llen_withStringFails() { + jedis.set("string", PREEXISTING_VALUE); + assertThatThrownBy(() -> jedis.llen("string")).hasMessageContaining(ERROR_WRONG_TYPE); + } + + @Test + public void llen_givenWrongNumOfArgs_returnsError() { + assertExactNumberOfArgs(jedis, Protocol.Command.LLEN, 1); + } + + @Test + public void llen_givenNonexistentList_returnsZero() { + assertThat(jedis.llen("nonexistent")).isEqualTo(0L); + } + + @Test + public void llen_returnsListLength() { + jedis.lpush(KEY, "e1", "e2", "e3"); + assertThat(jedis.llen(KEY)).isEqualTo(3L); + + String result = jedis.lpop(KEY); + assertThat(result).isEqualTo("e3"); Review comment: Not a major issue, but with these asserts after each LPOP, this test could fail due to a problem with LPOP, despite LLEN having correct behaviour. To keep tests more focused, it's better to have them only assert on things that they're explicitly trying to test the behaviour of. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLLenIntegrationTest.java ########## @@ -0,0 +1,124 @@ +/* + * 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_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.AtomicLong; + +import org.assertj.core.api.AssertionsForClassTypes; +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 AbstractLLenIntegrationTest 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 llen_withStringFails() { + jedis.set("string", PREEXISTING_VALUE); + assertThatThrownBy(() -> jedis.llen("string")).hasMessageContaining(ERROR_WRONG_TYPE); + } + + @Test + public void llen_givenWrongNumOfArgs_returnsError() { + assertExactNumberOfArgs(jedis, Protocol.Command.LLEN, 1); + } + + @Test + public void llen_givenNonexistentList_returnsZero() { + assertThat(jedis.llen("nonexistent")).isEqualTo(0L); + } + + @Test + public void llen_returnsListLength() { + jedis.lpush(KEY, "e1", "e2", "e3"); + assertThat(jedis.llen(KEY)).isEqualTo(3L); + + String result = jedis.lpop(KEY); + assertThat(result).isEqualTo("e3"); + assertThat(jedis.llen(KEY)).isEqualTo(2L); + + result = jedis.lpop(KEY); + assertThat(result).isEqualTo("e2"); + assertThat(jedis.llen(KEY)).isEqualTo(1L); + + result = jedis.lpop(KEY); + assertThat(result).isEqualTo("e1"); + assertThat(jedis.llen(KEY)).isEqualTo(0L); + } + + @Test + public void llen_withConcurrentLPush_returnsCorrectValue() { + String[] valuesInitial = new String[] {"one", "two", "three"}; + String[] valuesToAdd = new String[] {"pear", "apple", "plum", "orange", "peach"}; + jedis.lpush(KEY, valuesInitial); + + final AtomicLong llenReference = new AtomicLong(); + new ConcurrentLoopingThreads(1000, + i -> jedis.lpush(KEY, valuesToAdd), + i -> llenReference.set(jedis.llen(KEY))) + .runWithAction(() -> { + AssertionsForClassTypes.assertThat(llenReference).satisfiesAnyOf( + llenResult -> AssertionsForClassTypes.assertThat(llenResult.get()) + .isEqualTo(valuesInitial.length), + llenResult -> AssertionsForClassTypes.assertThat(llenResult.get()) Review comment: These assertions should not be using `AssertionsForClassTypes` but rather the vanilla `Assertions` method. ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisList.java ########## @@ -0,0 +1,184 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + * + */ + +package org.apache.geode.redis.internal.data; + +import static org.apache.geode.internal.JvmSizeUtils.memoryOverhead; +import static org.apache.geode.redis.internal.data.RedisDataType.REDIS_LIST; + +import java.io.DataInput; +import java.io.DataOutput; +import java.io.IOException; +import java.util.List; +import java.util.Objects; + +import org.apache.geode.DataSerializer; +import org.apache.geode.cache.Region; +import org.apache.geode.internal.serialization.DeserializationContext; +import org.apache.geode.internal.serialization.KnownVersion; +import org.apache.geode.internal.serialization.SerializationContext; +import org.apache.geode.redis.internal.data.collections.SizeableByteArrayList; +import org.apache.geode.redis.internal.data.delta.AddByteArrays; +import org.apache.geode.redis.internal.data.delta.RemoveElementsByIndex; + +public class RedisList extends AbstractRedisData { + protected static final int REDIS_LIST_OVERHEAD = memoryOverhead(RedisList.class); + private SizeableByteArrayList elementList; + + public RedisList() { + this.elementList = new SizeableByteArrayList(); + } + + /** + * @param elementsToAdd elements to add to this set; NOTE this list may by modified by this call + * @param region the region this instance is stored in + * @param key the name of the set to add to + * @return the number of elements actually added + */ + public long lpush(List<byte[]> elementsToAdd, Region<RedisKey, RedisData> region, RedisKey key) { + for (byte[] element : elementsToAdd) { + elementPush(element); + } + storeChanges(region, key, new AddByteArrays(elementsToAdd)); + return elementList.size(); + } + + /** + * @param region the region this instance is stored in + * @param key the name of the set to add to + * @return the element actually popped + */ + public byte[] lpop(Region<RedisKey, RedisData> region, RedisKey key) { + byte[] popped = elementRemove(0); + RemoveElementsByIndex removed = new RemoveElementsByIndex(); + removed.add(0); + storeChanges(region, key, removed); + return popped; + } + + /** + * @return the number of elements in the list + */ + public int llen() { + return elementList.size(); + } + + @Override + public void applyAddByteArrayDelta(byte[] bytes) { + elementPush(bytes); + } + + @Override + public void applyRemoveElementsByIndex(List<Integer> indexes) { + for (int index : indexes) { + elementRemove(index); + } + } + + /** + * Since GII (getInitialImage) can come in and call toData while other threads are modifying this + * object, the striped executor will not protect toData. So any methods that modify "elements" + * needs to be thread safe with toData. + */ + + @Override + public synchronized void toData(DataOutput out, SerializationContext context) throws IOException { + super.toData(out, context); + DataSerializer.writePrimitiveInt(elementList.size(), out); + for (byte[] element : elementList) { + DataSerializer.writeByteArray(element, out); + } + } + + @Override + public void fromData(DataInput in, DeserializationContext context) + throws IOException, ClassNotFoundException { + super.fromData(in, context); + int size = DataSerializer.readPrimitiveInt(in); + SizeableByteArrayList tempElementList = new SizeableByteArrayList(); + for (int i = 0; i < size; ++i) { + byte[] element = DataSerializer.readByteArray(in); + tempElementList.addLast(element); + } + replaceElementList(tempElementList); + } + + @Override + public int getDSFID() { + return REDIS_LIST_ID; + } + + private synchronized void replaceElementList(SizeableByteArrayList newList) { + elementList = newList; + } Review comment: I think this method might be overkill, since we're not modifying the contents of `elementList` here, just replacing it entirely, so there's no danger of `toData` being affected. ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/collections/SizeableByteArrayList.java ########## @@ -0,0 +1,128 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package org.apache.geode.redis.internal.data.collections; + +import static org.apache.geode.internal.JvmSizeUtils.getObjectHeaderSize; +import static org.apache.geode.internal.JvmSizeUtils.getReferenceSize; +import static org.apache.geode.internal.JvmSizeUtils.memoryOverhead; +import static org.apache.geode.internal.JvmSizeUtils.roundUpSize; + +import java.util.Arrays; +import java.util.LinkedList; +import java.util.ListIterator; + +import org.apache.geode.internal.size.Sizeable; + +public class SizeableByteArrayList extends LinkedList<byte[]> implements Sizeable { + private static final int BYTE_ARRAY_LIST_OVERHEAD = memoryOverhead(SizeableByteArrayList.class); + private static final int NODE_OVERHEAD = + roundUpSize(getObjectHeaderSize() + 3 * getReferenceSize()); + private static final int BYTE_ARRAY_BASE_OVERHEAD = 16; + private int memberOverhead; + + @Override + public int indexOf(Object o) { + ListIterator<byte[]> iterator = this.listIterator(); + while (iterator.hasNext()) { + int index = iterator.nextIndex(); + byte[] element = iterator.next(); + if (Arrays.equals(element, (byte[]) o)) { + return index; + } + } + return -1; + } + + @Override + public int lastIndexOf(Object o) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean remove(Object o) { + int index = indexOf(o); + if (index == -1) { + return false; + } + memberOverhead -= calculateByteArrayOverhead((byte[]) o); + remove(index); + return true; + } + + @Override + public byte[] remove(int index) { + byte[] element = super.remove(index); + memberOverhead -= calculateByteArrayOverhead(element); + return element; + } + + @Override + public void addFirst(byte[] element) { + memberOverhead += calculateByteArrayOverhead(element); + super.addFirst(element); + } + + @Override + public void addLast(byte[] element) { + memberOverhead += calculateByteArrayOverhead(element); + super.addLast(element); + } + + public boolean removeLastOccurrence(Object o) { + throw new UnsupportedOperationException(); + } + + private int calculateByteArrayOverhead(byte[] element) { + return BYTE_ARRAY_BASE_OVERHEAD + (element.length % 8 == 0 ? 0 : 8) + + NODE_OVERHEAD + (element.length / 8) * 8; + } + + @Override + public int getSizeInBytes() { + return BYTE_ARRAY_LIST_OVERHEAD + memberOverhead; + } + + @Override + public int hashCode() { + final int PRIME_NUMBER = 31; Review comment: This variable should not be in all-caps, as that is reserved for static final constants. The [Geode Code Style Guide page](https://cwiki.apache.org/confluence/display/GEODE/Code+Style+Guide) on the wiki contains a link to the [Google Java Style Guide](https://google.github.io/styleguide/javaguide.html) which details the conventions we (try to) follow when writing code for Geode. ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/executor/list/LPopExecutor.java ########## @@ -0,0 +1,43 @@ +/* + * 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.commands.parameters.RedisParametersMismatchException; +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 LPopExecutor implements CommandExecutor { + + @Override + public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) { + Region<RedisKey, RedisData> region = context.getRegion(); + RedisKey key = command.getKey(); + // TODO: Size check needed until we implement 'count' arg (Redis 6.2+) + if (command.getProcessedCommand().size() > 2) { + throw new RedisParametersMismatchException(command.wrongNumberOfArgumentsErrorMessage()); + } Review comment: See my comments in `RedisCommandType` regarding this. We should do our parameter count checking using the `Parameter()` class in `RedisCommandType`. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLLenIntegrationTest.java ########## @@ -0,0 +1,124 @@ +/* + * 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_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.AtomicLong; + +import org.assertj.core.api.AssertionsForClassTypes; +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 AbstractLLenIntegrationTest 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 llen_withStringFails() { + jedis.set("string", PREEXISTING_VALUE); + assertThatThrownBy(() -> jedis.llen("string")).hasMessageContaining(ERROR_WRONG_TYPE); + } + + @Test + public void llen_givenWrongNumOfArgs_returnsError() { + assertExactNumberOfArgs(jedis, Protocol.Command.LLEN, 1); + } + + @Test + public void llen_givenNonexistentList_returnsZero() { + assertThat(jedis.llen("nonexistent")).isEqualTo(0L); + } + + @Test + public void llen_returnsListLength() { + jedis.lpush(KEY, "e1", "e2", "e3"); + assertThat(jedis.llen(KEY)).isEqualTo(3L); + + String result = jedis.lpop(KEY); + assertThat(result).isEqualTo("e3"); + assertThat(jedis.llen(KEY)).isEqualTo(2L); + + result = jedis.lpop(KEY); + assertThat(result).isEqualTo("e2"); + assertThat(jedis.llen(KEY)).isEqualTo(1L); + + result = jedis.lpop(KEY); + assertThat(result).isEqualTo("e1"); + assertThat(jedis.llen(KEY)).isEqualTo(0L); + } + + @Test + public void llen_withConcurrentLPush_returnsCorrectValue() { + String[] valuesInitial = new String[] {"one", "two", "three"}; + String[] valuesToAdd = new String[] {"pear", "apple", "plum", "orange", "peach"}; + jedis.lpush(KEY, valuesInitial); + + final AtomicLong llenReference = new AtomicLong(); + new ConcurrentLoopingThreads(1000, + i -> jedis.lpush(KEY, valuesToAdd), + i -> llenReference.set(jedis.llen(KEY))) + .runWithAction(() -> { + AssertionsForClassTypes.assertThat(llenReference).satisfiesAnyOf( + llenResult -> AssertionsForClassTypes.assertThat(llenResult.get()) + .isEqualTo(valuesInitial.length), + llenResult -> AssertionsForClassTypes.assertThat(llenResult.get()) + .isEqualTo(valuesInitial.length + valuesToAdd.length)); + for (int i = 0; i < valuesToAdd.length; i++) { + jedis.lpop(KEY); + } + }); + } + + @Test + public void llen_withConcurrentLPop_returnsCorrectValue() { Review comment: It's not possible to test concurrent behaviour with LPOP (at least, the version that only pops one element at a time) because there are only two possible states for the list when doing a single LPOP; unmodified, or with one element missing. Since there's no "in-between" state that might be encountered due to improper locking, it's not possible to test that code using LPOP, we just end up testing that LPOP removes an element. As such, this test should be removed. ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/collections/SizeableByteArrayList.java ########## @@ -0,0 +1,128 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package org.apache.geode.redis.internal.data.collections; + +import static org.apache.geode.internal.JvmSizeUtils.getObjectHeaderSize; +import static org.apache.geode.internal.JvmSizeUtils.getReferenceSize; +import static org.apache.geode.internal.JvmSizeUtils.memoryOverhead; +import static org.apache.geode.internal.JvmSizeUtils.roundUpSize; + +import java.util.Arrays; +import java.util.LinkedList; +import java.util.ListIterator; + +import org.apache.geode.internal.size.Sizeable; + +public class SizeableByteArrayList extends LinkedList<byte[]> implements Sizeable { + private static final int BYTE_ARRAY_LIST_OVERHEAD = memoryOverhead(SizeableByteArrayList.class); + private static final int NODE_OVERHEAD = + roundUpSize(getObjectHeaderSize() + 3 * getReferenceSize()); + private static final int BYTE_ARRAY_BASE_OVERHEAD = 16; + private int memberOverhead; + + @Override + public int indexOf(Object o) { + ListIterator<byte[]> iterator = this.listIterator(); + while (iterator.hasNext()) { + int index = iterator.nextIndex(); + byte[] element = iterator.next(); + if (Arrays.equals(element, (byte[]) o)) { + return index; + } + } + return -1; + } + + @Override + public int lastIndexOf(Object o) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean remove(Object o) { + int index = indexOf(o); + if (index == -1) { + return false; + } + memberOverhead -= calculateByteArrayOverhead((byte[]) o); + remove(index); + return true; Review comment: This would be more efficient as: ``` ListIterator<byte[]> iterator = this.listIterator(); while (iterator.hasNext()) { byte[] element = iterator.next(); if (Arrays.equals(element, (byte[]) o)) { iterator.remove(); memberOverhead -= calculateByteArrayOverhead((byte[]) o); return true; } } return false; ``` as then we only have to iterate the list once. ########## File path: geode-for-redis/src/test/java/org/apache/geode/redis/internal/data/RedisListTest.java ########## @@ -0,0 +1,119 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + * + */ + +package org.apache.geode.redis.internal.data; + +import static org.apache.geode.redis.internal.data.NullRedisDataStructures.NULL_REDIS_LIST; +import static org.assertj.core.api.Assertions.assertThat; + +import java.io.DataOutput; +import java.io.IOException; +import java.lang.reflect.Modifier; + +import org.junit.Test; + +import org.apache.geode.DataSerializer; +import org.apache.geode.internal.HeapDataOutputStream; +import org.apache.geode.internal.serialization.ByteArrayDataInput; +import org.apache.geode.internal.serialization.SerializationContext; + +public class RedisListTest { + + @Test + public void confirmSerializationIsStable() throws IOException, ClassNotFoundException { + RedisList list1 = createRedisList(1, 2); + int expirationTimestamp = 1000; + list1.setExpirationTimestampNoDelta(expirationTimestamp); + HeapDataOutputStream out = new HeapDataOutputStream(100); + DataSerializer.writeObject(list1, out); + ByteArrayDataInput in = new ByteArrayDataInput(out.toByteArray()); + RedisList list2 = DataSerializer.readObject(in); + assertThat(list2.getExpirationTimestamp()) + .isEqualTo(list1.getExpirationTimestamp()) + .isEqualTo(expirationTimestamp); + assertThat(list2).isEqualTo(list1); + } + + @Test + public void confirmToDataIsSynchronized() throws NoSuchMethodException { + assertThat(Modifier + .isSynchronized(RedisList.class + .getMethod("toData", DataOutput.class, SerializationContext.class).getModifiers())) + .isTrue(); + } + + @Test + public void hashcode_returnsSameValue_forEqualLists() { + RedisList list1 = createRedisList(1, 2); + RedisList list2 = createRedisList(1, 2); + assertThat(list1).isEqualTo(list2); + assertThat(list1.hashCode()).isEqualTo(list2.hashCode()); + } + + @Test + public void hashcode_returnsDifferentValue_forDifferentLists() { + RedisList list1 = createRedisList(1, 2); + RedisList list2 = createRedisList(2, 1); + assertThat(list1).isNotEqualTo(list2); + assertThat(list1.hashCode()).isEqualTo(list2.hashCode()); Review comment: This should be using `isNotEqualTo()` ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLPopIntegrationTest.java ########## @@ -0,0 +1,113 @@ +/* + * 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.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.AtomicLong; + +import org.assertj.core.api.AssertionsForClassTypes; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import redis.clients.jedis.HostAndPort; +import redis.clients.jedis.JedisCluster; + +import org.apache.geode.redis.ConcurrentLoopingThreads; +import org.apache.geode.redis.RedisIntegrationTest; + +public abstract class AbstractLPopIntegrationTest implements RedisIntegrationTest { + public static final String KEY = "key"; + public static final String PREEXISTING_VALUE = "preexistingValue"; + // TODO: make private when we implement Redis 6.2+ behavior for LPOP + public JedisCluster jedis; + + @Before + public void setUp() { + jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT); + } + + @After + public void tearDown() { + flushAll(); + jedis.close(); + } + + // not checking LPOP argument count here, see LPopIntegrationTest Review comment: We can have a test for argument count here: ``` @Test public void lpop_givenWrongNumOfArgs_returnsError() { assertExactNumberOfArgs(jedis, Protocol.Command.LPOP, 2); } ``` and then `@Override` it in `LPopIntegrationTest` so that we test against native Redis and against geode-for-redis, just with different implementations: ``` @Test @Override public void lpop_givenWrongNumOfArgs_returnsError() { assertExactNumberOfArgs(jedis, Protocol.Command.LPOP, 1); } ``` ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLPopIntegrationTest.java ########## @@ -0,0 +1,113 @@ +/* + * 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.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.AtomicLong; + +import org.assertj.core.api.AssertionsForClassTypes; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import redis.clients.jedis.HostAndPort; +import redis.clients.jedis.JedisCluster; + +import org.apache.geode.redis.ConcurrentLoopingThreads; +import org.apache.geode.redis.RedisIntegrationTest; + +public abstract class AbstractLPopIntegrationTest implements RedisIntegrationTest { + public static final String KEY = "key"; + public static final String PREEXISTING_VALUE = "preexistingValue"; + // TODO: make private when we implement Redis 6.2+ behavior for LPOP + public JedisCluster jedis; + + @Before + public void setUp() { + jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT); + } + + @After + public void tearDown() { + flushAll(); + jedis.close(); + } + + // not checking LPOP argument count here, see LPopIntegrationTest + + @Test + public void lpop_withStringFails() { Review comment: To avoid confusion about RedisString vs Java String here, could this test be renamed "lpop_withNonListKey_returnsWrongTypeError"? ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLPopIntegrationTest.java ########## @@ -0,0 +1,113 @@ +/* + * 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.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.AtomicLong; + +import org.assertj.core.api.AssertionsForClassTypes; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import redis.clients.jedis.HostAndPort; +import redis.clients.jedis.JedisCluster; + +import org.apache.geode.redis.ConcurrentLoopingThreads; +import org.apache.geode.redis.RedisIntegrationTest; + +public abstract class AbstractLPopIntegrationTest implements RedisIntegrationTest { + public static final String KEY = "key"; + public static final String PREEXISTING_VALUE = "preexistingValue"; + // TODO: make private when we implement Redis 6.2+ behavior for LPOP + public JedisCluster jedis; + + @Before + public void setUp() { + jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT); + } + + @After + public void tearDown() { + flushAll(); + jedis.close(); + } + + // not checking LPOP argument count here, see LPopIntegrationTest + + @Test + public void lpop_withStringFails() { + jedis.set("string", PREEXISTING_VALUE); + assertThatThrownBy(() -> jedis.lpop("string")).hasMessageContaining(ERROR_WRONG_TYPE); + } + + @Test + public void lpop_withNonExistentKey_returnsNull() { + assertThat(jedis.lpop("nonexistent")).isNull(); + } + + @Test + public void lpop_returnsOneMember() { + jedis.lpush(KEY, "e1", "e2"); + String result = jedis.lpop(KEY); + assertThat(result).isEqualTo("e2"); + } + + @Test + public void lpop_removesKey_whenLastElementRemoved() { + final String KEY_WITH_TAG = "{tag}" + KEY; + + jedis.lpush(KEY_WITH_TAG, "e1"); + jedis.lpop(KEY_WITH_TAG); + assertThat(jedis.keys(KEY_WITH_TAG)).isEmpty(); + } + + @Test + public void lpop_removesKey_whenLastElementRemoved_multipleTimes() { + final String KEY_WITH_TAG = "{tag}" + KEY; + + jedis.lpush(KEY_WITH_TAG, "e1"); + jedis.lpop(KEY_WITH_TAG); + jedis.lpop(KEY_WITH_TAG); + jedis.lpop(KEY_WITH_TAG); Review comment: Could we have assertions on what's returned from each LPOP here, just to make sure that we're behaving as expected? ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLPopIntegrationTest.java ########## @@ -0,0 +1,113 @@ +/* + * 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.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.AtomicLong; + +import org.assertj.core.api.AssertionsForClassTypes; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import redis.clients.jedis.HostAndPort; +import redis.clients.jedis.JedisCluster; + +import org.apache.geode.redis.ConcurrentLoopingThreads; +import org.apache.geode.redis.RedisIntegrationTest; + +public abstract class AbstractLPopIntegrationTest implements RedisIntegrationTest { + public static final String KEY = "key"; + public static final String PREEXISTING_VALUE = "preexistingValue"; + // TODO: make private when we implement Redis 6.2+ behavior for LPOP + public JedisCluster jedis; + + @Before + public void setUp() { + jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT); + } + + @After + public void tearDown() { + flushAll(); + jedis.close(); + } + + // not checking LPOP argument count here, see LPopIntegrationTest + + @Test + public void lpop_withStringFails() { + jedis.set("string", PREEXISTING_VALUE); + assertThatThrownBy(() -> jedis.lpop("string")).hasMessageContaining(ERROR_WRONG_TYPE); + } + + @Test + public void lpop_withNonExistentKey_returnsNull() { + assertThat(jedis.lpop("nonexistent")).isNull(); + } + + @Test + public void lpop_returnsOneMember() { Review comment: Could this test instead be "lpop_returnsLeftmostMember" since we care *which* member is returned, not just that one member is returned. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLPopIntegrationTest.java ########## @@ -0,0 +1,113 @@ +/* + * 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.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.AtomicLong; + +import org.assertj.core.api.AssertionsForClassTypes; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import redis.clients.jedis.HostAndPort; +import redis.clients.jedis.JedisCluster; + +import org.apache.geode.redis.ConcurrentLoopingThreads; +import org.apache.geode.redis.RedisIntegrationTest; + +public abstract class AbstractLPopIntegrationTest implements RedisIntegrationTest { + public static final String KEY = "key"; + public static final String PREEXISTING_VALUE = "preexistingValue"; + // TODO: make private when we implement Redis 6.2+ behavior for LPOP + public JedisCluster jedis; + + @Before + public void setUp() { + jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT); + } + + @After + public void tearDown() { + flushAll(); + jedis.close(); + } + + // not checking LPOP argument count here, see LPopIntegrationTest + + @Test + public void lpop_withStringFails() { + jedis.set("string", PREEXISTING_VALUE); + assertThatThrownBy(() -> jedis.lpop("string")).hasMessageContaining(ERROR_WRONG_TYPE); + } + + @Test + public void lpop_withNonExistentKey_returnsNull() { + assertThat(jedis.lpop("nonexistent")).isNull(); + } + + @Test + public void lpop_returnsOneMember() { + jedis.lpush(KEY, "e1", "e2"); + String result = jedis.lpop(KEY); + assertThat(result).isEqualTo("e2"); + } + + @Test + public void lpop_removesKey_whenLastElementRemoved() { + final String KEY_WITH_TAG = "{tag}" + KEY; + + jedis.lpush(KEY_WITH_TAG, "e1"); + jedis.lpop(KEY_WITH_TAG); + assertThat(jedis.keys(KEY_WITH_TAG)).isEmpty(); + } + + @Test + public void lpop_removesKey_whenLastElementRemoved_multipleTimes() { + final String KEY_WITH_TAG = "{tag}" + KEY; Review comment: This variable is unnecessary. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLPushIntegrationTest.java ########## @@ -0,0 +1,119 @@ +/* + * 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.assertAtLeastNArgs; +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 org.junit.After; +import org.junit.Before; +import org.junit.Test; +import redis.clients.jedis.HostAndPort; +import redis.clients.jedis.JedisCluster; +import redis.clients.jedis.Protocol; +import redis.clients.jedis.exceptions.JedisDataException; + +import org.apache.geode.redis.RedisIntegrationTest; + +public abstract class AbstractLPushIntegrationTest 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 lpushErrors_givenTooFewArguments() { + assertAtLeastNArgs(jedis, Protocol.Command.LPUSH, 2); + } + + @Test + public void lpushErrors_withExistingKey_ofWrongType() { + String elementValue = "list element value that should never get added"; + + jedis.set(KEY, PREEXISTING_VALUE); + assertThatThrownBy(() -> jedis.lpush(KEY, elementValue)) + .hasMessageContaining(ERROR_WRONG_TYPE); + } + + @Test + public void lpush_withExistingKey_ofWrongType_shouldNotOverWriteExistingKey() { Review comment: Having both of these test cases is redundant, as the second one entirely contains the first one. It should be okay to just have the second test, and update the name to "lpush_withExistingKey_ofWrongType_returnsWrongTypeError_shouldNotOverWriteExistingKey" ########## File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LPopDUnitTest.java ########## @@ -0,0 +1,139 @@ +/* + * 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.function.Supplier; + +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Test; +import redis.clients.jedis.HostAndPort; +import redis.clients.jedis.JedisCluster; + +import org.apache.geode.redis.ConcurrentLoopingThreads; +import org.apache.geode.test.dunit.rules.MemberVM; +import org.apache.geode.test.dunit.rules.RedisClusterStartupRule; + +public class LPopDUnitTest { + private static final String KEY_BASE = "key"; + private static final int NUM_LISTS = 100; + private static final String ELEMENT_BASE = "element-"; + public static final int NUM_VMS = 4; + + @ClassRule + public static RedisClusterStartupRule clusterStartUp = new RedisClusterStartupRule(4); + + private static final int LIST_SIZE = 100; + private static JedisCluster jedis; + + @BeforeClass + public static void classSetup() { + 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); + } + + @Before + public void testSetup() { + clusterStartUp.flushAll(); + } + + @AfterClass + public static void tearDown() { + jedis.close(); + } + + @Test + public void shouldDistributeDataAmongCluster_andRetainDataAfterServerCrash() + throws Exception { + List<String> elementList1 = makeElementList(0, LIST_SIZE / 2, ELEMENT_BASE); + List<String> elementList2 = makeElementList(LIST_SIZE / 2, LIST_SIZE, ELEMENT_BASE); + List<String> allElements = makeElementList(0, LIST_SIZE, ELEMENT_BASE); + + new ConcurrentLoopingThreads(NUM_LISTS / 2, + (i) -> jedis.lpush(KEY_BASE + i, elementList1.toArray(new String[] {})), + (i) -> jedis.lpush(KEY_BASE + i, elementList2.toArray(new String[] {})), + (i) -> jedis.lpush(KEY_BASE + (i + NUM_LISTS / 2), elementList1.toArray(new String[] {})), + (i) -> jedis.lpush(KEY_BASE + (i + NUM_LISTS / 2), elementList2.toArray(new String[] {}))) + .runInLockstep(); + + clusterStartUp.crashVM(NUM_VMS - 1); + + confirmAllDataIsPresent(allElements); + } Review comment: This test is not testing the distributed behaviour of LPOP, but rather testing that secondary copies of buckets contain the same data as primary copies after concurrent LPUSH calls, so that when we failover due to the server crash, data is consistent. As such, this test should be reworked to test distributed LPOP behaviour. Examples of what we could test: - With two servers, loading a list on one server, calling LPOP repeatedly to empty half the list, then crashing the primary server results in the list containing what we expect it to after the secondary is promoted to primary - With at least three servers, loading a list, then concurrently calling LPOP while forcing the primary to move to a member that doesn't host the bucket (similar to what's done in `StringsDUnitTest.givenBucketsMoveDuringAppend_thenDataIsNotLost`) and asserting that we never get the same element returned from LPOP twice (which would indicate that we did an LPOP without actually removing the element). Given that LPOP can suffer from the same problems as APPEND, where a retried command results in the operation being performed twice, care needs to be taken with what we assert in the second test case so as not to produce a flaky test. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLPopIntegrationTest.java ########## @@ -0,0 +1,113 @@ +/* + * 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.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.AtomicLong; + +import org.assertj.core.api.AssertionsForClassTypes; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import redis.clients.jedis.HostAndPort; +import redis.clients.jedis.JedisCluster; + +import org.apache.geode.redis.ConcurrentLoopingThreads; +import org.apache.geode.redis.RedisIntegrationTest; + +public abstract class AbstractLPopIntegrationTest implements RedisIntegrationTest { + public static final String KEY = "key"; + public static final String PREEXISTING_VALUE = "preexistingValue"; + // TODO: make private when we implement Redis 6.2+ behavior for LPOP + public JedisCluster jedis; + + @Before + public void setUp() { + jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, getPort()), REDIS_CLIENT_TIMEOUT); + } + + @After + public void tearDown() { + flushAll(); + jedis.close(); + } + + // not checking LPOP argument count here, see LPopIntegrationTest + + @Test + public void lpop_withStringFails() { + jedis.set("string", PREEXISTING_VALUE); + assertThatThrownBy(() -> jedis.lpop("string")).hasMessageContaining(ERROR_WRONG_TYPE); + } + + @Test + public void lpop_withNonExistentKey_returnsNull() { + assertThat(jedis.lpop("nonexistent")).isNull(); + } + + @Test + public void lpop_returnsOneMember() { + jedis.lpush(KEY, "e1", "e2"); + String result = jedis.lpop(KEY); + assertThat(result).isEqualTo("e2"); + } + + @Test + public void lpop_removesKey_whenLastElementRemoved() { + final String KEY_WITH_TAG = "{tag}" + KEY; Review comment: This variable is unnecessary, since commands using only one key in integration tests do not need to use tags. ########## File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/commands/RedisCommandType.java ########## @@ -286,6 +289,12 @@ ZUNIONSTORE(new ZUnionStoreExecutor(), SUPPORTED, new Parameter().min(4).flags(WRITE, DENYOOM, MOVABLEKEYS)), + /************** Lists *****************/ + + LLEN(new LLenExecutor(), SUPPORTED, new Parameter().exact(2).flags(READONLY, FAST)), + LPOP(new LPopExecutor(), SUPPORTED, new Parameter().min(2).flags(WRITE, FAST)), Review comment: I still feel that this should be changed to `exact(2)` as that's the behaviour we're requiring from the user, and the output of the COMMAND command will reflect what's encoded here. If we have the COMMAND command say that LPOP takes 2 or more arguments, but do not accept more than 2 arguments, that's a bug. ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLPushIntegrationTest.java ########## @@ -0,0 +1,119 @@ +/* + * 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.assertAtLeastNArgs; +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 org.junit.After; +import org.junit.Before; +import org.junit.Test; +import redis.clients.jedis.HostAndPort; +import redis.clients.jedis.JedisCluster; +import redis.clients.jedis.Protocol; +import redis.clients.jedis.exceptions.JedisDataException; + +import org.apache.geode.redis.RedisIntegrationTest; + +public abstract class AbstractLPushIntegrationTest 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 lpushErrors_givenTooFewArguments() { + assertAtLeastNArgs(jedis, Protocol.Command.LPUSH, 2); + } + + @Test + public void lpushErrors_withExistingKey_ofWrongType() { + String elementValue = "list element value that should never get added"; + + jedis.set(KEY, PREEXISTING_VALUE); + assertThatThrownBy(() -> jedis.lpush(KEY, elementValue)) + .hasMessageContaining(ERROR_WRONG_TYPE); + } + + @Test + public void lpush_withExistingKey_ofWrongType_shouldNotOverWriteExistingKey() { + String elementValue = "list element value that should never get added"; + + jedis.set(KEY, PREEXISTING_VALUE); + + assertThatThrownBy(() -> jedis.lpush(KEY, elementValue)).isInstanceOf(JedisDataException.class); + + String result = jedis.get(KEY); + + assertThat(result).isEqualTo(PREEXISTING_VALUE); + } + + @Test + public void lpush_canStoreBinaryData() { + byte[] blob = new byte[256]; + for (int i = 0; i < 256; i++) { + blob[i] = (byte) i; + } + + jedis.lpush(KEY.getBytes(), blob, blob); + + byte[] result = jedis.lpop(KEY.getBytes()); + assertThat(result).containsExactly(blob); + result = jedis.lpop(KEY.getBytes()); + assertThat(result).containsExactly(blob); + } + + @Test + public void lpush_returnsUpdatedListLength() { + Long result = jedis.lpush(KEY, "e1"); + assertThat(result).isEqualTo(jedis.llen(KEY)); + + result = jedis.lpush(KEY, "e2"); + assertThat(result).isEqualTo(jedis.llen(KEY)); + + result = jedis.lpush(KEY, "e3"); + assertThat(result).isEqualTo(jedis.llen(KEY)); Review comment: These assertions should be using hardcoded integer values rather than the returned value of LLEN, as it's possible that LLEN and LPUSH could both be broken in such a way that this test passes, but that the behaviour isn't correct. It would also be good to have one of the LPUSH calls use multiple elements to make sure that the returned value is as expected then too. The test would then become: ``` @Test public void lpush_returnsUpdatedListLength() { assertThat(jedis.lpush(KEY, "e1")).isEqualTo(1); assertThat(jedis.lpush(KEY, "e2")).isEqualTo(2); assertThat(jedis.lpush(KEY, "e3", "e4")).isEqualTo(4); } ``` ########## File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/list/AbstractLPushIntegrationTest.java ########## @@ -0,0 +1,119 @@ +/* + * 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.assertAtLeastNArgs; +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 org.junit.After; +import org.junit.Before; +import org.junit.Test; +import redis.clients.jedis.HostAndPort; +import redis.clients.jedis.JedisCluster; +import redis.clients.jedis.Protocol; +import redis.clients.jedis.exceptions.JedisDataException; + +import org.apache.geode.redis.RedisIntegrationTest; + +public abstract class AbstractLPushIntegrationTest 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 lpushErrors_givenTooFewArguments() { + assertAtLeastNArgs(jedis, Protocol.Command.LPUSH, 2); + } + + @Test + public void lpushErrors_withExistingKey_ofWrongType() { + String elementValue = "list element value that should never get added"; + + jedis.set(KEY, PREEXISTING_VALUE); + assertThatThrownBy(() -> jedis.lpush(KEY, elementValue)) + .hasMessageContaining(ERROR_WRONG_TYPE); + } + + @Test + public void lpush_withExistingKey_ofWrongType_shouldNotOverWriteExistingKey() { + String elementValue = "list element value that should never get added"; + + jedis.set(KEY, PREEXISTING_VALUE); + + assertThatThrownBy(() -> jedis.lpush(KEY, elementValue)).isInstanceOf(JedisDataException.class); + + String result = jedis.get(KEY); + + assertThat(result).isEqualTo(PREEXISTING_VALUE); + } + + @Test + public void lpush_canStoreBinaryData() { Review comment: This test is unnecessary, as the format of the data passed to the geode-for-redis server is determined by the client sending the command. If there was a difference in behaviour between LPUSH with a String and LPUSH with a byte[], it would be due to a bug with Jedis, not with geode-for-redis. ########## File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/list/LPushDUnitTest.java ########## @@ -0,0 +1,143 @@ +/* + * 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 org.junit.AfterClass; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Test; +import redis.clients.jedis.HostAndPort; +import redis.clients.jedis.JedisCluster; + +import org.apache.geode.redis.ConcurrentLoopingThreads; +import org.apache.geode.test.dunit.rules.MemberVM; +import org.apache.geode.test.dunit.rules.RedisClusterStartupRule; + +public class LPushDUnitTest { + + @ClassRule + public static RedisClusterStartupRule clusterStartUp = new RedisClusterStartupRule(4); + + private static final int LIST_SIZE = 1000; + private static JedisCluster jedis; + + private static MemberVM server1; + private static MemberVM server2; + private static MemberVM server3; + + @BeforeClass + public static void classSetup() { + MemberVM locator = clusterStartUp.startLocatorVM(0); + server1 = clusterStartUp.startRedisVM(1, locator.getPort()); + server2 = clusterStartUp.startRedisVM(2, locator.getPort()); + server3 = clusterStartUp.startRedisVM(3, locator.getPort()); + + int redisServerPort = clusterStartUp.getRedisPort(1); + jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, redisServerPort), REDIS_CLIENT_TIMEOUT); + } + + @Before + public void testSetup() { + clusterStartUp.flushAll(); + } + + @AfterClass + public static void tearDown() { + jedis.close(); + + server1.stop(); + server2.stop(); + server3.stop(); Review comment: This is not necessary, as shutdown is handled by the `RedisClusterStartupRule`. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
