DonalEvans commented on a change in pull request #6573: URL: https://github.com/apache/geode/pull/6573#discussion_r647019665
########## File path: geode-apis-compatible-with-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/sortedset/ZAddIncrOptionDUnitTest.java ########## @@ -0,0 +1,216 @@ +/* + * 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.executor.sortedset; + +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 org.junit.After; +import org.junit.Before; +import org.junit.Ignore; +import org.junit.Rule; +import org.junit.Test; +import redis.clients.jedis.HostAndPort; +import redis.clients.jedis.JedisCluster; +import redis.clients.jedis.Protocol; +import redis.clients.jedis.exceptions.JedisClusterMaxAttemptsException; + +import org.apache.geode.cache.Operation; +import org.apache.geode.cache.Region; +import org.apache.geode.internal.cache.PartitionedRegion; +import org.apache.geode.internal.cache.PartitionedRegionHelper; +import org.apache.geode.redis.ConcurrentLoopingThreads; +import org.apache.geode.redis.internal.RegionProvider; +import org.apache.geode.redis.internal.data.RedisData; +import org.apache.geode.redis.internal.data.RedisKey; +import org.apache.geode.redis.internal.netty.Coder; +import org.apache.geode.test.dunit.rules.ClusterStartupRule; +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 ZAddIncrOptionDUnitTest { + @Rule + public ExecutorServiceRule executor = new ExecutorServiceRule(); + + @Rule + public RedisClusterStartupRule clusterStartUp = new RedisClusterStartupRule(4); + + private JedisCluster jedis; + private List<MemberVM> servers; + private static final String sortedSetKey = "key"; + private final String baseMemberName = "member"; + private final int setSize = 1000; + private final double increment1 = 355.681000005; + private final double increment2 = 9554257.921450001; + private final double total = increment1 + increment2; + + @Before + public void setup() { + MemberVM locator = clusterStartUp.startLocatorVM(0); + int locatorPort = locator.getPort(); + MemberVM server1 = clusterStartUp.startRedisVM(1, locatorPort); + MemberVM server2 = clusterStartUp.startRedisVM(2, locatorPort); + MemberVM server3 = clusterStartUp.startRedisVM(3, locatorPort); + servers = new ArrayList<>(); + servers.add(server1); + servers.add(server2); + servers.add(server3); + + int redisServerPort = clusterStartUp.getRedisPort(1); + + jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, redisServerPort), REDIS_CLIENT_TIMEOUT); + } + + @After + public void tearDown() { + jedis.close(); + } + + @Test + public void zAddWithIncrOptionCanAddAndIncrementScoresConcurrently() { + new ConcurrentLoopingThreads(setSize, + (i) -> doZAddIncr(i, increment1, total, true), + (i) -> doZAddIncr(i, increment2, total, true)).run(); + + assertThat(jedis.zcard(sortedSetKey)).isEqualTo(setSize); + verifyZScores(); + } + + private void verifyZScores() { + for (int i = 0; i < setSize; i++) { + assertThat(jedis.zscore(sortedSetKey, baseMemberName + i)).isEqualTo(total); + } + } + + private void doZAddIncr(int i, double increment, double total, boolean isConcurrentExecution) { + Object result = + jedis.sendCommand(sortedSetKey, Protocol.Command.ZADD, sortedSetKey, "INCR", + Coder.doubleToString(increment), baseMemberName + i); + if (isConcurrentExecution) { + assertThat(Coder.bytesToDouble((byte[]) result)).isIn(increment, total); + } else { + assertThat(Coder.bytesToDouble((byte[]) result)).isEqualTo(total); + } + } + + @Test + public void zAddWithIncrOptionCanIncrementScoresAfterPrimaryShutsDown() { + doZAddIncrForAllMembers(increment1, increment1); + + stopNodeWithPrimaryBucketOfTheKey(false); + + doZCardWithRetries(); + doZAddIncrForAllMembers(increment2, total); + verifyZScores(); + } + + private void doZAddIncrForAllMembers(double increment1, double increment12) { Review comment: Typo here, I think it should probably be `increment2` not `increment12`. ########## File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/sortedset/ZAddExecutor.java ########## @@ -46,11 +45,17 @@ public RedisResponse executeCommand(Command command, ExecutionHandlerContext con return RedisResponse.error(zAddExecutorState.exceptionMessage); } - long retVal = redisSortedSetCommands.zadd(command.getKey(), + Object retVal = redisSortedSetCommands.zadd(command.getKey(), new ArrayList<>(commandElements.subList(optionsFoundCount + 2, commandElements.size())), makeOptions(zAddExecutorState)); + if (zAddExecutorState.incrFound) { + if (retVal == null) { + return RedisResponse.nil(); + } + return RedisResponse.string((byte[]) retVal); Review comment: The [documentation for ZADD with INCR option](https://redis.io/commands/ZADD#return-value) states that the response will be a Bulk string response, not a simple string. I think this should be therefore be `RedisResponse.bulkString(retVal);` The documentation for ZINCRBY also says the same, so I think we may need to change the `RedisResponse` type in the `ZIncrByExecutor` as well, since it's also currently just using a simple string response. ########## File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZAddIntegrationTest.java ########## @@ -301,6 +306,106 @@ public void shouldUpdateScore_whenSettingMemberThatAlreadyExists() { assertThat(jedis.zscore(key, member)).isEqualTo(1.0); } + + @Test + public void zaddIncrOptionSupportsOnlyOneIncrementingElementPair() { + assertThatThrownBy(() -> jedis.sendCommand(SORTED_SET_KEY, Protocol.Command.ZADD, + SORTED_SET_KEY, incrOption, "1", member, "2", "member_1")) + .hasMessageContaining(RedisConstants.ERROR_ZADD_OPTION_TOO_MANY_INCR_PAIR); + } + + @Test + public void zaddIncrOptionThrowsIfIncorrectScorePair() { + assertThatThrownBy(() -> jedis.sendCommand(SORTED_SET_KEY, Protocol.Command.ZADD, + SORTED_SET_KEY, incrOption, "1", member, "2")).hasMessageContaining(ERROR_SYNTAX); + } + + @Test + public void zaddIncrOptionDoseNotAddANewMemberWithXXOption() { + Object result = jedis.sendCommand(SORTED_SET_KEY, Protocol.Command.ZADD, SORTED_SET_KEY, "XX", + incrOption, "1", member); + assertThat(result).isNull(); + assertThat(jedis.zscore(SORTED_SET_KEY, member)).isNull(); + } + + private final double initial = 355.681000005; + private final double increment = 9554257.921450001; + private final double expected = initial + increment; Review comment: Could these fields be moved to the top of the class for consistency with other classes please? ########## File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZAddIntegrationTest.java ########## @@ -35,12 +35,17 @@ import redis.clients.jedis.Protocol; import redis.clients.jedis.params.ZAddParams; +import org.apache.geode.redis.ConcurrentLoopingThreads; import org.apache.geode.redis.RedisIntegrationTest; import org.apache.geode.redis.internal.RedisConstants; +import org.apache.geode.redis.internal.netty.Coder; import org.apache.geode.test.awaitility.GeodeAwaitility; public abstract class AbstractZAddIntegrationTest implements RedisIntegrationTest { private JedisCluster jedis; + private final String member = "member"; + private final String incrOption = "INCR"; + private static final int REDIS_CLIENT_TIMEOUT = Review comment: This constant is already defined in `RedisIntegrationTest` so it doesn't need to be defined here too. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
