You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/06/18 21:12:57 UTC
[GitHub] [geode] sabbey37 commented on a change in pull request #6595: GEODE-9166: Implement radish MOVED response
sabbey37 commented on a change in pull request #6595:
URL: https://github.com/apache/geode/pull/6595#discussion_r653833223
##########
File path: geode-apis-compatible-with-redis/src/distributedTest/java/org/apache/geode/redis/internal/cluster/RedisPartitionResolverDUnitTest.java
##########
@@ -92,6 +91,16 @@ public void testRedisHashesMapToCorrectBuckets() {
assertThat(buckets1.size() + buckets2.size() + buckets3.size())
.isEqualTo(RegionProvider.REDIS_REGION_BUCKETS);
+
+ validateBucketMapping(keyToBucketMap1);
+ validateBucketMapping(keyToBucketMap2);
+ validateBucketMapping(keyToBucketMap3);
+ }
+
+ private void validateBucketMapping(Map<String, Integer> bucketMap) {
+ for (Map.Entry<String, Integer> e : bucketMap.entrySet()) {
+ assertThat(new RedisKey(e.getKey().getBytes()).getBucketId()).isEqualTo(e.getValue());
Review comment:
You could use the `stringToBytes` method from `Coder` here.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java
##########
@@ -226,15 +227,9 @@ private RedisResponse getExceptionResponse(ChannelHandlerContext ctx, Throwable
return null;
}
- if (cause instanceof FunctionException
- && !(cause instanceof FunctionInvocationTargetException)) {
- Throwable th = getInitialCause((FunctionException) cause);
Review comment:
Is it no longer possible to hit a `FunctionException` here? I forget why this was added....
##########
File path: geode-apis-compatible-with-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/MovedDUnitTest.java
##########
@@ -0,0 +1,161 @@
+/*
+ * 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;
+
+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 io.lettuce.core.cluster.ClusterClientOptions;
+import io.lettuce.core.cluster.ClusterTopologyRefreshOptions;
+import io.lettuce.core.cluster.RedisClusterClient;
+import io.lettuce.core.cluster.api.sync.RedisAdvancedClusterCommands;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Ignore;
+import org.junit.Test;
+import redis.clients.jedis.Jedis;
+import redis.clients.jedis.exceptions.JedisMovedDataException;
+
+import org.apache.geode.cache.control.RebalanceFactory;
+import org.apache.geode.cache.control.ResourceManager;
+import org.apache.geode.redis.internal.cluster.RedisMemberInfo;
+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 MovedDUnitTest {
+
+ @ClassRule
+ public static RedisClusterStartupRule clusterStartUp = new RedisClusterStartupRule();
+
+ private static Jedis jedis1;
+ private static Jedis jedis2;
+ private static RedisAdvancedClusterCommands<String, String> lettuce;
+ private static RedisClusterClient clusterClient;
+ private static MemberVM locator;
+ private static int redisServerPort1;
+ private static final int ENTRIES = 200;
+
+ @BeforeClass
+ public static void classSetup() {
+ locator = clusterStartUp.startLocatorVM(0);
+ clusterStartUp.startRedisVM(1, locator.getPort());
+ clusterStartUp.startRedisVM(2, locator.getPort());
+
+ redisServerPort1 = clusterStartUp.getRedisPort(1);
+ int redisServerPort2 = clusterStartUp.getRedisPort(2);
+
+ jedis1 = new Jedis(BIND_ADDRESS, redisServerPort1, REDIS_CLIENT_TIMEOUT);
+ jedis2 = new Jedis(BIND_ADDRESS, redisServerPort2, REDIS_CLIENT_TIMEOUT);
+ clusterClient = RedisClusterClient.create("redis://localhost:" + redisServerPort1);
+
+ ClusterTopologyRefreshOptions refreshOptions =
+ ClusterTopologyRefreshOptions.builder()
+ .enableAllAdaptiveRefreshTriggers()
+ .build();
+
+ clusterClient.setOptions(ClusterClientOptions.builder()
+ .topologyRefreshOptions(refreshOptions)
+ .autoReconnect(true)
+ .validateClusterNodeMembership(false)
+ .build());
+
+ lettuce = clusterClient.connect().sync();
+ }
+
+ @AfterClass
+ public static void cleanup() {
+ clusterClient.shutdown();
+ }
+
+ @Before
+ public void testSetup() {
+ clusterStartUp.flushAll();
+ }
+
+ @Test
+ public void testMovedResponse_fromWrongServer() {
+ int movedResponses = 0;
+ Jedis jedis;
+
+ for (int i = 0; i < ENTRIES; i++) {
+ String key = "key-" + i;
+ String value = "value-" + i;
+
+ // Always pick the wrong connection to use
+ RedisMemberInfo memberInfo = clusterStartUp.getMemberInfo(key);
+ jedis = memberInfo.getRedisPort() == redisServerPort1 ? jedis2 : jedis1;
+
+ try {
+ jedis.set(key, value);
+ } catch (JedisMovedDataException mex) {
+ movedResponses++;
+ }
+ }
+
+ assertThat(movedResponses).isEqualTo(ENTRIES);
+ }
+
+ @Test
+ public void testNoMovedResponse_fromCorrectServer() {
+ Jedis jedis;
+
+ for (int i = 0; i < ENTRIES; i++) {
+ String key = "key-" + i;
+ String value = "value-" + i;
+
+ // Always pick the right connection to use
+ RedisMemberInfo memberInfo = clusterStartUp.getMemberInfo(key);
+ jedis = memberInfo.getRedisPort() == redisServerPort1 ? jedis1 : jedis2;
+
+ assertThat(jedis.set(key, value)).isEqualTo("OK");
+ }
+ }
+
+ @Test
+ @Ignore("GEODE-9368")
+ public void movedResponseFollowsFailedServer() throws Exception {
Review comment:
For some reason the name of this test is strange to me and seems different from its contents. What is it meant to test?
##########
File path: geode-apis-compatible-with-redis/src/distributedTest/java/org/apache/geode/redis/internal/data/PartitionedRegionStatsUpdateTest.java
##########
@@ -38,14 +39,13 @@
private static MemberVM server1;
private static MemberVM server2;
- private static Jedis jedis1;
- private static Jedis jedis2;
+ private static JedisCluster jedis;
private static final int JEDIS_TIMEOUT = Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
private static final String LOCAL_HOST = "127.0.0.1";
Review comment:
We could use the constants from `RedisClusterStartUpRule` for these classes.
##########
File path: geode-apis-compatible-with-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/InfoDUnitTest.java
##########
@@ -1,134 +0,0 @@
-/*
- * 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;
-
-import static org.assertj.core.api.Assertions.assertThat;
-
-import java.util.HashMap;
-import java.util.Map;
-import java.util.concurrent.atomic.AtomicInteger;
-
-import org.junit.After;
-import org.junit.AfterClass;
-import org.junit.BeforeClass;
-import org.junit.ClassRule;
-import org.junit.Test;
-import redis.clients.jedis.Jedis;
-
-import org.apache.geode.redis.ConcurrentLoopingThreads;
-import org.apache.geode.test.awaitility.GeodeAwaitility;
-import org.apache.geode.test.dunit.rules.MemberVM;
-import org.apache.geode.test.dunit.rules.RedisClusterStartupRule;
-
-public class InfoDUnitTest {
Review comment:
I don't think we need this any longer, but just want to double check that it was intentionally deleted.
##########
File path: geode-apis-compatible-with-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/MovedDUnitTest.java
##########
@@ -0,0 +1,161 @@
+/*
+ * 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;
+
+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 io.lettuce.core.cluster.ClusterClientOptions;
+import io.lettuce.core.cluster.ClusterTopologyRefreshOptions;
+import io.lettuce.core.cluster.RedisClusterClient;
+import io.lettuce.core.cluster.api.sync.RedisAdvancedClusterCommands;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Ignore;
+import org.junit.Test;
+import redis.clients.jedis.Jedis;
+import redis.clients.jedis.exceptions.JedisMovedDataException;
+
+import org.apache.geode.cache.control.RebalanceFactory;
+import org.apache.geode.cache.control.ResourceManager;
+import org.apache.geode.redis.internal.cluster.RedisMemberInfo;
+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 MovedDUnitTest {
+
+ @ClassRule
+ public static RedisClusterStartupRule clusterStartUp = new RedisClusterStartupRule();
+
+ private static Jedis jedis1;
+ private static Jedis jedis2;
+ private static RedisAdvancedClusterCommands<String, String> lettuce;
+ private static RedisClusterClient clusterClient;
+ private static MemberVM locator;
+ private static int redisServerPort1;
+ private static final int ENTRIES = 200;
+
+ @BeforeClass
+ public static void classSetup() {
+ locator = clusterStartUp.startLocatorVM(0);
+ clusterStartUp.startRedisVM(1, locator.getPort());
+ clusterStartUp.startRedisVM(2, locator.getPort());
+
+ redisServerPort1 = clusterStartUp.getRedisPort(1);
+ int redisServerPort2 = clusterStartUp.getRedisPort(2);
+
+ jedis1 = new Jedis(BIND_ADDRESS, redisServerPort1, REDIS_CLIENT_TIMEOUT);
+ jedis2 = new Jedis(BIND_ADDRESS, redisServerPort2, REDIS_CLIENT_TIMEOUT);
+ clusterClient = RedisClusterClient.create("redis://localhost:" + redisServerPort1);
+
+ ClusterTopologyRefreshOptions refreshOptions =
+ ClusterTopologyRefreshOptions.builder()
+ .enableAllAdaptiveRefreshTriggers()
+ .build();
+
+ clusterClient.setOptions(ClusterClientOptions.builder()
+ .topologyRefreshOptions(refreshOptions)
+ .autoReconnect(true)
+ .validateClusterNodeMembership(false)
+ .build());
+
+ lettuce = clusterClient.connect().sync();
+ }
+
+ @AfterClass
+ public static void cleanup() {
+ clusterClient.shutdown();
Review comment:
Should we also close the jedis clients in this method?
##########
File path: geode-apis-compatible-with-redis/src/distributedTest/java/org/apache/geode/redis/session/springRedisTestApplication/config/DUnitSocketAddressResolver.java
##########
@@ -1,57 +0,0 @@
-/*
- * 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.session.springRedisTestApplication.config;
-
-import java.net.InetAddress;
-import java.net.InetSocketAddress;
-
-import io.lettuce.core.RedisURI;
-import io.lettuce.core.resource.DnsResolver;
-import io.lettuce.core.resource.SocketAddressResolver;
-import org.apache.logging.log4j.Logger;
-
-import org.apache.geode.logging.internal.log4j.api.LogService;
-
-public class DUnitSocketAddressResolver extends SocketAddressResolver {
Review comment:
Doesn't seem like we need this any longer, but I wanted to make sure it was intentionally delted.
##########
File path: geode-apis-compatible-with-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/hash/HlenDUnitTest.java
##########
@@ -98,9 +71,6 @@ public void testSetup() {
@AfterClass
public static void tearDown() throws Exception {
Review comment:
Do we need to shutdown the clusterClient here? I think there are some other tests we might need to do this in as well that I neglected to comment on.
##########
File path: geode-apis-compatible-with-redis/src/distributedTest/java/org/apache/geode/redis/internal/cluster/RedisPartitionResolverDUnitTest.java
##########
@@ -62,7 +61,7 @@ public static void classSetup() {
server3 = cluster.startRedisVM(3, locator.getPort());
int redisServerPort1 = cluster.getRedisPort(1);
- jedis1 = new Jedis(LOCAL_HOST, redisServerPort1, JEDIS_TIMEOUT);
+ jedis = new JedisCluster(new HostAndPort(BIND_ADDRESS, redisServerPort1), REDIS_CLIENT_TIMEOUT);
Review comment:
Should we add a `disconnect` of the jedis client in the teardown method?
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/GeodeRedisServer.java
##########
@@ -149,9 +151,13 @@ public synchronized void shutdown() {
}
@VisibleForTesting
- protected Long getDataStoreBytesInUseForDataRegion() {
+ public Long getDataStoreBytesInUseForDataRegion() {
PartitionedRegion dataRegion = (PartitionedRegion) this.getRegionProvider().getDataRegion();
- long dataStoreBytesInUse = dataRegion.getPrStats().getDataStoreBytesInUse();
- return dataStoreBytesInUse;
+ return dataRegion.getPrStats().getDataStoreBytesInUse();
+ }
+
+ @VisibleForTesting
+ public RedisMemberInfo getMemberInfo(String key) throws InterruptedException {
+ return regionProvider.getSlotAdvisor().getMemberInfo(new RedisKey(key.getBytes()));
Review comment:
I think you could use `stringToBytes` from `Coder` here.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/GeodeRedisServer.java
##########
@@ -149,9 +151,13 @@ public synchronized void shutdown() {
}
@VisibleForTesting
- protected Long getDataStoreBytesInUseForDataRegion() {
+ public Long getDataStoreBytesInUseForDataRegion() {
Review comment:
Just wanted to make sure we intended to change this to a `public` method.
##########
File path: geode-apis-compatible-with-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/hash/HlenDUnitTest.java
##########
@@ -54,41 +48,20 @@
private static MemberVM locator;
private static MemberVM server1;
private static MemberVM server2;
- private static int[] redisPorts;
private static RedisAdvancedClusterCommands<String, String> lettuce;
- private static StatefulRedisClusterConnection<String, String> connection;
- private static ClientResources resources;
@BeforeClass
public static void classSetup() {
- redisPorts = AvailablePortHelper.getRandomAvailableTCPPorts(3);
-
- String redisPort1 = String.valueOf(redisPorts[0]);
- String redisPort2 = String.valueOf(redisPorts[1]);
-
locator = cluster.startLocatorVM(0);
Review comment:
Locator could be a local variable.
--
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:
users@infra.apache.org