[GitHub] [geode] ringles commented on a change in pull request #5067: GEODE-8130: use single region for Sets and Hashes

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

ringles commented on a change in pull request #5067:
URL: https://github.com/apache/geode/pull/5067#discussion_r425234480



##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/KeyRegistrar.java
##########
@@ -14,56 +14,147 @@
  */
 package org.apache.geode.redis.internal;
 
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
 import java.util.Map;
 import java.util.Set;
 
+import org.apache.geode.DataSerializer;
+import org.apache.geode.InvalidDeltaException;
 import org.apache.geode.cache.Region;
 
 public class KeyRegistrar {
-  private Region<String, RedisDataType> redisMetaRegion;
+  private Region<ByteArrayWrapper, RedisData> redisDataRegion;
 
-  public KeyRegistrar(Region<String, RedisDataType> redisMetaRegion) {
-    this.redisMetaRegion = redisMetaRegion;
+  public KeyRegistrar(Region<ByteArrayWrapper, RedisData> redisDataRegion) {
+    this.redisDataRegion = redisDataRegion;
   }
 
   /**
    * Checks if the givenKey is associated with the passed data type. If an entry doesn't exist,
    * store the key:datatype association in the metadataRegion
    */
   public void register(ByteArrayWrapper key, RedisDataType type) {
-    RedisDataType existingType = this.redisMetaRegion.putIfAbsent(key.toString(), type);
-    if (!isValidDataType(existingType, type)) {
-      throwDataTypeException(key, existingType);
+    RedisData existingValue = this.redisDataRegion.putIfAbsent(key, transformType(type));
+    if (!isValidDataType(existingValue, type)) {
+      throwDataTypeException(key, existingValue);
+    }
+  }
+
+  /**
+   * This class should go away once all data types implement RedisData.

Review comment:
       Probably a TODO here, so it's more obvious.

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/GeodeRedisServer.java
##########
@@ -526,20 +499,28 @@ private void registerLockServiceMBean() {
   }
 
   private void checkForRegions() {
-    Collection<Entry<String, RedisDataType>> entrySet = keyRegistrar.keyInfos();
-    for (Entry<String, RedisDataType> entry : entrySet) {
-      String regionName = entry.getKey();
-      RedisDataType type = entry.getValue();
-      Region<?, ?> newRegion = cache.getRegion(regionName);
-      if (newRegion == null && type != RedisDataType.REDIS_STRING && type != RedisDataType.REDIS_HLL
-          && type != RedisDataType.REDIS_PROTECTED) {
-        try {
-          regionCache
-              .createRemoteRegionReferenceLocally(Coder.stringToByteArrayWrapper(regionName), type);
-        } catch (Exception e) {
-          if (logger.errorEnabled()) {
-            logger.error(e);
-          }
+    Collection<Entry<ByteArrayWrapper, RedisData>> entrySet = keyRegistrar.keyInfos();
+    for (Entry<ByteArrayWrapper, RedisData> entry : entrySet) {
+      ByteArrayWrapper key = entry.getKey();
+      RedisDataType type = entry.getValue().getType();
+      if (!regionCache.typeUsesDynamicRegions(type)) {

Review comment:
       Should "regionCache" be renamed "regionProvider"? Line 506 talks about a 'regionCache', and then 509 has a 'cache'. I can see someone looking over the code wondering, "why two caches"?




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