sabbey37 commented on a change in pull request #6573:
URL: https://github.com/apache/geode/pull/6573#discussion_r647515532
##########
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() {
Review comment:
These tests are great! Could we also add a test that does a ZADD with
INCR, NX, and XX options and verifies that the error message is `ERR XX and NX
options at the same time are not compatible`? It will pass, but I think it
would be good to add in case anyone tries to change that order in the future.
##########
File path:
geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/sortedset/AbstractZAddIntegrationTest.java
##########
@@ -101,14 +106,14 @@ public void zaddErrors_givenBothNXAndXXOptions() {
public void zadd_prioritizesErrors_inTheCorrectOrder() {
assertThatThrownBy(
() -> jedis.sendCommand("fakeKey", Protocol.Command.ZADD, "fakeKey",
"NX", "XX", "xlerb",
- "member", "2"))
+ member, "2"))
.hasMessageContaining(ERROR_SYNTAX);
assertThatThrownBy(
() -> jedis.sendCommand("fakeKey", Protocol.Command.ZADD, "fakeKey",
"NX", "XX", "xlerb",
- "member"))
+ member))
.hasMessageContaining(ERROR_INVALID_ZADD_OPTION_NX_XX);
assertThatThrownBy(
- () -> jedis.sendCommand("fakeKey", Protocol.Command.ZADD, "fakeKey",
"xlerb", "member"))
+ () -> jedis.sendCommand("fakeKey", Protocol.Command.ZADD, "fakeKey",
"xlerb", member))
Review comment:
Could we also add a case to this test that does a ZADD with INCR, NX,
and XX options and verifies that the error message is `ERR XX and NX options at
the same time are not compatible`? It will pass, but I think it would be good
to add in case anyone tries to change that order in the future.
##########
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) {
+ for (int i = 0; i < setSize; i++) {
+ doZAddIncr(i, increment1, increment12, false);
+ }
+ }
+
+ private void doZCardWithRetries() {
+ int maxRetryAttempts = 10;
+ int retryAttempts = 0;
+ while (!zCardWithRetries(retryAttempts, maxRetryAttempts)) {
+ retryAttempts++;
+ }
+ }
Review comment:
It might be a bit more straightforward to take out the extra
`zCardWithRetries` method and just do the following:
```
private void doZCardWithRetries() {
int maxRetryAttempts = 10;
int retryAttempts = 0;
while (retryAttempts < maxRetryAttempts) {
long memberSize;
try {
memberSize = jedis.zcard(sortedSetKey);
} catch (JedisClusterMaxAttemptsException e) {
retryAttempts++;
if(retryAttempts == maxRetryAttempts) {
throw e;
}
continue;
}
assertThat(memberSize).isEqualTo(setSize);
break;
}
}
```
...but that might just be a personal preference.
##########
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) {
+ for (int i = 0; i < setSize; i++) {
+ doZAddIncr(i, increment1, increment12, false);
+ }
+ }
+
+ private void doZCardWithRetries() {
+ int maxRetryAttempts = 10;
+ int retryAttempts = 0;
+ while (!zCardWithRetries(retryAttempts, maxRetryAttempts)) {
+ retryAttempts++;
+ }
+ }
+
+ private boolean zCardWithRetries(int retries, int maxRetries) {
+ long memberSize;
+ try {
+ memberSize = jedis.zcard(sortedSetKey);
+ } catch (JedisClusterMaxAttemptsException e) {
+ if (retries < maxRetries) {
+ return false;
+ }
+ throw e;
+ }
+ assertThat(memberSize).isEqualTo(setSize);
+ return true;
+ }
+
+ private boolean hitJedisClusterIssue2347 = false;
Review comment:
Could we make this variable local to the test and pass it into the
`doZAddIncrForAllMembersDuringCrash` method?
--
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]
