[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #6117: GEODE-9023: Add sharding support to redis region

[GitBox <gi...@apache.org>]

pivotal-jbarrett commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r592440110



##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -8149,8 +8149,8 @@ public void dumpSelfEntryFromAllPartitionedRegions() {
    * A test method to get the list of all the bucket ids for the partitioned region in the data
    * Store.
    */
-  public List getLocalBucketsListTestOnly() {
-    List localBucketList = null;
+  public List<Integer> getLocalBucketsListTestOnly() {

Review comment:
       Can this be package protected at least?

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -14,19 +14,24 @@
  */
 package org.apache.geode.redis.internal;
 
+import org.apache.geode.cache.PartitionAttributesFactory;
 import org.apache.geode.cache.Region;
 import org.apache.geode.cache.RegionShortcut;
 import org.apache.geode.internal.cache.InternalCache;
 import org.apache.geode.internal.cache.InternalRegionFactory;
 import org.apache.geode.redis.internal.data.ByteArrayWrapper;
 import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.executor.cluster.RedisPartitionResolver;
 
 public class RegionProvider {
   /**
    * The name of the region that holds data stored in redis.
    */
-  private static final String REDIS_DATA_REGION = "__REDIS_DATA";
-  private static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+  public static final String REDIS_DATA_REGION = "__REDIS_DATA";
+  public static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+  public static final int REDIS_REGION_BUCKETS = Integer.getInteger("redis.region.buckets", 128);
+  public static final int REDIS_SLOTS = Integer.getInteger("redis.slots", 16384);

Review comment:
       Why is this value configurable? It is a fixed value in Redis.

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -14,19 +14,24 @@
  */
 package org.apache.geode.redis.internal;
 
+import org.apache.geode.cache.PartitionAttributesFactory;
 import org.apache.geode.cache.Region;
 import org.apache.geode.cache.RegionShortcut;
 import org.apache.geode.internal.cache.InternalCache;
 import org.apache.geode.internal.cache.InternalRegionFactory;
 import org.apache.geode.redis.internal.data.ByteArrayWrapper;
 import org.apache.geode.redis.internal.data.RedisData;
+import org.apache.geode.redis.internal.executor.cluster.RedisPartitionResolver;
 
 public class RegionProvider {
   /**
    * The name of the region that holds data stored in redis.
    */
-  private static final String REDIS_DATA_REGION = "__REDIS_DATA";
-  private static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+  public static final String REDIS_DATA_REGION = "__REDIS_DATA";
+  public static final String REDIS_CONFIG_REGION = "__REDIS_CONFIG";
+  public static final int REDIS_REGION_BUCKETS = Integer.getInteger("redis.region.buckets", 128);

Review comment:
       If this is configurable we should validate that the user provides a power of 2, probably between 2 and 16384. We could even do that by changing this to the exponent of the power of 2 rather than the value.

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/ByteArrayWrapper.java
##########
@@ -107,6 +115,39 @@ public int compareTo(ByteArrayWrapper other) {
     return arrayCmp(value, other.value);
   }
 
+  /**
+   * Used by the {@link RedisPartitionResolver} to map slots to buckets. Supports using hashtags
+   * in the same way that redis does.
+   *
+   * @see <a href="https://redis.io/topics/cluster-spec">Redis Cluster Spec</a>
+   */
+  public synchronized Object getRoutingId() {
+    if (routingId == null && value != null) {
+      int startHashtag = Integer.MAX_VALUE;
+      int endHashtag = 0;
+
+      for (int i = 0; i < value.length; i++) {
+        if (value[i] == '{' && startHashtag == Integer.MAX_VALUE) {
+          startHashtag = i;
+        }
+        if (value[i] == '}') {
+          endHashtag = i;
+          break;
+        }
+      }
+
+      if (endHashtag - startHashtag <= 1) {
+        startHashtag = -1;
+        endHashtag = value.length;
+      }
+
+      routingId = (CRC16.calculate(value, startHashtag + 1, endHashtag) % REDIS_SLOTS)

Review comment:
       I think we are ok right now for bucket size 128 but anything larger and the `Integer` cache won't kick in here. We should consider a local cache of `Integer` routing ids and ignore any built in `Integer` cache.
   

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/data/ByteArrayWrapper.java
##########
@@ -107,6 +115,39 @@ public int compareTo(ByteArrayWrapper other) {
     return arrayCmp(value, other.value);
   }
 
+  /**
+   * Used by the {@link RedisPartitionResolver} to map slots to buckets. Supports using hashtags
+   * in the same way that redis does.
+   *
+   * @see <a href="https://redis.io/topics/cluster-spec">Redis Cluster Spec</a>
+   */
+  public synchronized Object getRoutingId() {

Review comment:
       Do we really want this call to be synchronized? Given that the key is immutable and we will always be asking for the routingId then why not calculate at construction and make final.




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