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

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

sabbey37 commented on a change in pull request #6117:
URL: https://github.com/apache/geode/pull/6117#discussion_r592572832



##########
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:
       Do we need to have some check to ensure that if `redis.region.buckets` is set via a system property that it is a power of 2 and a factor of 16384? (maybe that's coming in the next PR?)

##########
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:
       I'm probably missing something obvious, but I was wondering why we try to get `redis.slots` via System.getProperty... will we allow it to be configured at some point? The Redis cluster docs seem to say there are always `16384` slots.




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