[jira] [Commented] (GEODE-8864) finish implementation of Redis HScan Command

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/GEODE-8864?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17276039#comment-17276039 ] 

ASF GitHub Bot commented on GEODE-8864:
---------------------------------------

jdeppe-pivotal commented on a change in pull request #5954:
URL: https://github.com/apache/geode/pull/5954#discussion_r567544947



##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisHash.java
##########
@@ -45,10 +47,14 @@
 public class RedisHash extends AbstractRedisData {
   public static final RedisHash NULL_REDIS_HASH = new NullRedisHash();
   private HashMap<ByteArrayWrapper, ByteArrayWrapper> hash;
+  private HashMap<UUID, Map<ByteArrayWrapper,ByteArrayWrapper>> perClientHScanDataSnapShot;

Review comment:
       We should only be storing the list of keys here and not the whole hash. Redis doesn't guarantee the _value_ of a field remaining constant across scans just the list of fields. It's bad enough that we need to keep a list of fields but would be even worse if we also need to keep the values.

##########
File path: geode-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/hash/HScanDunitTest.java
##########
@@ -0,0 +1,241 @@
+/*
+ * 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.hash;
+
+import static org.apache.geode.distributed.ConfigurationProperties.MAX_WAIT_TIME_RECONNECT;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Properties;
+import java.util.concurrent.CountDownLatch;
+
+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.ScanResult;
+
+import org.apache.geode.distributed.ConfigurationProperties;
+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 HScanDunitTest {
+  @ClassRule
+  public static RedisClusterStartupRule clusterStartUp = new RedisClusterStartupRule(4);
+
+  private static final String LOCAL_HOST = "127.0.0.1";
+  private static final int JEDIS_TIMEOUT =
+      Math.toIntExact(GeodeAwaitility.getTimeout().toMillis());
+
+  private static Jedis jedis1;
+  private static Jedis jedis2;
+  private static Jedis jedis3;
+
+  private static Properties locatorProperties;
+
+  private static MemberVM locator;
+  private static MemberVM server1;
+  private static MemberVM server2;
+  private static MemberVM server3;
+
+  private static int redisServerPort1;
+  private static int redisServerPort2;
+  private static int redisServerPort3;
+
+  private final int SIZE_OF_INITIAL_HASH_DATA = 1000;
+  private final Map<String, String> INITIAL_HASH_DATA = makeEntrySet(SIZE_OF_INITIAL_HASH_DATA);
+  final String HASH_KEY = "key";
+  final String BASE_FIELD = "baseField_";
+
+  @BeforeClass
+  public static void classSetup() {
+    locatorProperties = new Properties();
+    locatorProperties.setProperty(MAX_WAIT_TIME_RECONNECT, "15000");
+    locator = clusterStartUp.startLocatorVM(0, locatorProperties);
+    int locatorPort = locator.getPort();
+
+    server1 = clusterStartUp
+        .startRedisVM(1,
+            x -> x.withProperty(ConfigurationProperties.SERIALIZABLE_OBJECT_FILTER,
+                "org.apache.commons.lang3.tuple.**;org.apache.geode.**")
+                .withConnectionToLocator(locatorPort));
+
+    server2 = clusterStartUp
+        .startRedisVM(2,
+            x -> x.withProperty(ConfigurationProperties.SERIALIZABLE_OBJECT_FILTER,
+                "org.apache.commons.lang3.tuple.**;org.apache.geode.**")
+                .withConnectionToLocator(locatorPort));
+
+    server3 = clusterStartUp
+        .startRedisVM(3,
+            x -> x.withProperty(ConfigurationProperties.SERIALIZABLE_OBJECT_FILTER,
+                "org.apache.commons.lang3.tuple.**;org.apache.geode.**")
+                .withConnectionToLocator(locatorPort));
+
+    redisServerPort1 = clusterStartUp.getRedisPort(1);
+    redisServerPort2 = clusterStartUp.getRedisPort(2);
+    redisServerPort3 = clusterStartUp.getRedisPort(3);
+
+    jedis1 = new Jedis(LOCAL_HOST, redisServerPort1, JEDIS_TIMEOUT);
+    jedis2 = new Jedis(LOCAL_HOST, redisServerPort2, JEDIS_TIMEOUT);
+    jedis3 = new Jedis(LOCAL_HOST, redisServerPort3, JEDIS_TIMEOUT);
+  }
+
+  @Before
+  public void testSetup() {
+    jedis1.flushAll();
+    jedis2.flushAll();
+    jedis3.flushAll();
+
+    jedis1.hset(HASH_KEY, INITIAL_HASH_DATA);
+  }
+
+  @AfterClass
+  public static void tearDown() {
+    jedis1.disconnect();
+    jedis2.disconnect();
+    jedis3.disconnect();
+
+    server1.stop();
+    server2.stop();
+    server3.stop();
+  }
+
+  @Test
+  public void should_notLoseFields_givenConcurrentThreadsDoingHScansAndChangingValues() {
+    final int ITERATION_COUNT = 500;
+
+    new ConcurrentLoopingThreads(ITERATION_COUNT,
+        (i) -> multipleHScanAndAssertOnSizeOfResultSet(jedis1),
+        (i) -> multipleHScanAndAssertOnSizeOfResultSet(jedis2),
+        (i) -> {
+          int fieldSuffix =

Review comment:
       This can simply just be `i % SIZE_OF_INITIAL_HASH_DATA`

##########
File path: geode-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/hash/HScanDunitTest.java
##########
@@ -0,0 +1,241 @@
+/*
+ * 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.hash;
+
+import static org.apache.geode.distributed.ConfigurationProperties.MAX_WAIT_TIME_RECONNECT;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Properties;
+import java.util.concurrent.CountDownLatch;
+
+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.ScanResult;
+
+import org.apache.geode.distributed.ConfigurationProperties;
+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 HScanDunitTest {

Review comment:
       I think these would be very valuable to run as integration tests so that we can validate native redis' functionality.




----------------------------------------------------------------
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


> finish implementation of Redis HScan Command
> --------------------------------------------
>
>                 Key: GEODE-8864
>                 URL: https://issues.apache.org/jira/browse/GEODE-8864
>             Project: Geode
>          Issue Type: New Feature
>          Components: redis
>            Reporter: John Hutchison
>            Priority: Major
>              Labels: pull-request-available
>




--
This message was sent by Atlassian Jira
(v8.3.4#803005)