DonalEvans commented on a change in pull request #7513:
URL: https://github.com/apache/geode/pull/7513#discussion_r838872401
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisSet.java
##########
@@ -77,12 +82,20 @@ public static int smove(RedisKey sourceKey, RedisKey
destKey, byte[] member,
RegionProvider regionProvider) {
RedisSet source = regionProvider.getTypedRedisData(REDIS_SET, sourceKey,
false);
RedisSet destination = regionProvider.getTypedRedisData(REDIS_SET,
destKey, false);
- List<byte[]> memberList = new ArrayList<>();
- memberList.add(member);
- if (source.srem(memberList, regionProvider.getDataRegion(), sourceKey) ==
0) {
+
+ if (!source.sismember(member)) {
return 0;
}
- destination.sadd(memberList, regionProvider.getDataRegion(), destKey);
+ if (sourceKey.equals(destKey)) {
+ return 1;
+ }
+
+ List<byte[]> memberList = new ArrayList<>();
+ memberList.add(member);
+ RedisSet newSource = new RedisSet(source);
+ newSource.srem(memberList, regionProvider.getDataRegion(), sourceKey);
+ RedisSet newDestination = new RedisSet(destination);
+ newDestination.sadd(memberList, regionProvider.getDataRegion(), destKey);
Review comment:
Creating new copies of the RedisSets here will result in the version and
expiration timestamp fields being reset to the initial value. This could lead
to the secondary copies not applying a versioned delta that they should have if
the versions line up properly, and to keys that should expire not expiring. I
think we need a true copy constructor for our Redis data classes to retain the
same version and expiration timestamp info.
##########
File path:
geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/string/MSetDUnitTest.java
##########
@@ -114,6 +119,33 @@ public void
testMSet_concurrentInstancesHandleBucketMovement() {
.allSatisfy(value ->
assertThat(value).startsWith("valueTwo"))));
}
+ @Test
+ public void testMSet_isTransactional() {
+ String hashTag = "{" + clusterStartUp.getKeyOnServer("tag", 1) + "}";
+ String key1 = hashTag + "key1";
+ String value1 = "value1";
+ jedis.set(key1, value1);
+
+ clusterStartUp.getMember(1).invoke(() -> {
+ valueReference =
+ (RedisString)
ClusterStartupRule.getCache().getRegion(DEFAULT_REDIS_REGION_NAME)
+ .get(new RedisKey(key1.getBytes()));
+ assertThat(new String(valueReference.getValue())).isEqualTo(value1);
+ });
+
+ String newValue = "should_not_be_set";
Review comment:
This should be changed, since the test expects that it should be set.
##########
File path:
geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/string/MSetDUnitTest.java
##########
@@ -48,6 +51,8 @@
private static final Logger logger = LogService.getLogger();
+ public static RedisString valueReference;
Review comment:
This should probably be replaced with a local variable in the test that
uses it:
```
AtomicReference<RedisString> valueReference = new AtomicReference<>();
clusterStartUp.getMember(1).invoke(() -> {
valueReference.set((RedisString) ClusterStartupRule.getCache()
.getRegion(DEFAULT_REDIS_REGION_NAME).get(new
RedisKey(key1.getBytes())));
assertThat(new
String(valueReference.get().getValue())).isEqualTo(value1);
});
```
##########
File path:
geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/set/SmoveDunitTest.java
##########
@@ -0,0 +1,204 @@
+/*
+ * 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.set;
+
+import static
org.apache.geode.redis.internal.services.RegionProvider.DEFAULT_REDIS_REGION_NAME;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+
+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.redis.internal.data.RedisKey;
+import org.apache.geode.redis.internal.data.RedisSet;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+
+public class SmoveDunitTest {
+
+ @ClassRule
+ public static RedisClusterStartupRule clusterStartUp = new
RedisClusterStartupRule(4);
+
+ private static final String LOCAL_HOST = "127.0.0.1";
+ private static final int SET_SIZE = 1000;
+ private static final int JEDIS_TIMEOUT =
+ Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+ private static JedisCluster jedis;
+ public static RedisSet srcValueReference;
+ public static RedisSet destValueReference;
+
+ private static MemberVM locator;
+ private static MemberVM server1;
+ private static MemberVM server2;
+ private static MemberVM server3;
Review comment:
This fields are never used and can be removed.
##########
File path:
geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/set/SmoveDunitTest.java
##########
@@ -0,0 +1,204 @@
+/*
+ * 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.set;
+
+import static
org.apache.geode.redis.internal.services.RegionProvider.DEFAULT_REDIS_REGION_NAME;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+
+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.redis.internal.data.RedisKey;
+import org.apache.geode.redis.internal.data.RedisSet;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+
+public class SmoveDunitTest {
+
+ @ClassRule
+ public static RedisClusterStartupRule clusterStartUp = new
RedisClusterStartupRule(4);
+
+ private static final String LOCAL_HOST = "127.0.0.1";
+ private static final int SET_SIZE = 1000;
+ private static final int JEDIS_TIMEOUT =
+ Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
Review comment:
Rather than defining new constants in this class for host and timeout,
the `BIND_ADDRESS` and `REDIS_CLIENT_TIMEOUT` constants in
RedisClusterStartupRule should be used.
##########
File path:
geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/set/SmoveDunitTest.java
##########
@@ -0,0 +1,204 @@
+/*
+ * 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.set;
+
+import static
org.apache.geode.redis.internal.services.RegionProvider.DEFAULT_REDIS_REGION_NAME;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+
+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.redis.internal.data.RedisKey;
+import org.apache.geode.redis.internal.data.RedisSet;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+
+public class SmoveDunitTest {
+
+ @ClassRule
+ public static RedisClusterStartupRule clusterStartUp = new
RedisClusterStartupRule(4);
+
+ private static final String LOCAL_HOST = "127.0.0.1";
+ private static final int SET_SIZE = 1000;
+ private static final int JEDIS_TIMEOUT =
+ Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+ private static JedisCluster jedis;
+ public static RedisSet srcValueReference;
+ public static RedisSet destValueReference;
+
+ private static MemberVM locator;
Review comment:
This can be a local variable in `setUp()`
##########
File path:
geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/string/MSetDUnitTest.java
##########
@@ -114,6 +119,33 @@ public void
testMSet_concurrentInstancesHandleBucketMovement() {
.allSatisfy(value ->
assertThat(value).startsWith("valueTwo"))));
}
+ @Test
+ public void testMSet_isTransactional() {
Review comment:
It's not clear to me how this test is showing that MSET is
transactional, since it doesn't test any transactional behaviour, specifically
that failure part way through setting multiple keys results in rolling back the
changes. The assertion that gets made on lines 143-144 shows that the value
stored in the region is not the same object that was originally present, but
that doesn't strictly speaking guarantee that the command is transactional.
To show transactional behaviour, we need a test that attempts to set
multiple keys using MSET, one of the keys should throw an exception causing an
error to be returned to the jedis client, then confirm that we did not set any
of the keys.
##########
File path:
geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/set/SmoveDunitTest.java
##########
@@ -0,0 +1,204 @@
+/*
+ * 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.set;
+
+import static
org.apache.geode.redis.internal.services.RegionProvider.DEFAULT_REDIS_REGION_NAME;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+
+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.redis.internal.data.RedisKey;
+import org.apache.geode.redis.internal.data.RedisSet;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+
+public class SmoveDunitTest {
Review comment:
For consistency, could this be "SMoveDUnitTest"?
##########
File path:
geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/commands/executor/set/SmoveDunitTest.java
##########
@@ -0,0 +1,204 @@
+/*
+ * 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.set;
+
+import static
org.apache.geode.redis.internal.services.RegionProvider.DEFAULT_REDIS_REGION_NAME;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+
+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.redis.internal.data.RedisKey;
+import org.apache.geode.redis.internal.data.RedisSet;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
+
+public class SmoveDunitTest {
+
+ @ClassRule
+ public static RedisClusterStartupRule clusterStartUp = new
RedisClusterStartupRule(4);
+
+ private static final String LOCAL_HOST = "127.0.0.1";
+ private static final int SET_SIZE = 1000;
+ private static final int JEDIS_TIMEOUT =
+ Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+ private static JedisCluster jedis;
+ public static RedisSet srcValueReference;
+ public static RedisSet destValueReference;
+
+ private static MemberVM locator;
+ private static MemberVM server1;
+ private static MemberVM server2;
+ private static MemberVM server3;
+
+ @BeforeClass
+ public static void classSetup() {
Review comment:
The class that was used as the template to create this one was not a
good choice, unfortunately, as the tests added here are not actually testing
any distributed behaviour. A better example class to base this one off would be
one of the List command distributed tests, such as `RPopLPushDUnitTest`.
Specifically, the tests we should have are for:
- Performing an operation on the primary, crashing the primary and then
verifying that the data on the secondary is correct
- Performing operations while simultaneously moving buckets and verifying
that no operations are lost or duplicated
- Confirming that if the operation fails part way through, we don't apply
half of it i.e. we don't ever remove something from the source set and then not
add it to the destination set
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
