dschneider-pivotal commented on a change in pull request #6960:
URL: https://github.com/apache/geode/pull/6960#discussion_r727280716
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -75,7 +76,7 @@
/**
* The name of the region that holds data stored in redis.
*/
- public static final String REDIS_DATA_REGION = "REDIS_DATA";
+ public static final String DEFAULT_REDIS_DATA_REGION =
GeodeRedisServer.DEFAULT_REDIS_REGION_NAME;
Review comment:
This seems confusing. We should not have two DEFAULT_REDIS_DATA_REGION
constants (one on RegionProvider and one on GeodeRedisServer). I think it
should just be on RegionProvider but you could move it to RedisConstants
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/GeodeRedisServer.java
##########
@@ -132,6 +133,10 @@ public int getPort() {
return nettyRedisServer.getPort();
}
+ public String getRegionName() {
+ return nettyRedisServer.getRegionName();
Review comment:
ask the RegionProvider for the region name; not the netty server
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java
##########
@@ -102,6 +106,13 @@ public NettyRedisServer(Supplier<DistributionConfig>
configSupplier,
this.member = member;
this.securityService = securityService;
+ String regionName = System.getProperty(REDIS_REGION_NAME);
Review comment:
This should be done in RegionProvider not NettyRedisServer. The
NettyRedisServer should focus on netty stuff.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java
##########
@@ -156,6 +167,10 @@ public int getPort() {
return serverPort;
}
+ public String getRegionName() {
Review comment:
remove this method
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java
##########
@@ -89,10 +89,10 @@
static {
String resourcePermission = System.getProperty("redis.resource-permission",
- "DATA:WRITE:" + RegionProvider.REDIS_DATA_REGION);
+ "DATA:WRITE:" + RegionProvider.DEFAULT_REDIS_DATA_REGION);
Review comment:
This seems wrong. Shouldn't this be the real redis data region name? If
they do customize it I think they would expect to so their customized region
name in their security handler not the hard coded default
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java
##########
@@ -72,4 +72,7 @@
public static final String ERROR_KEY_REQUIRED =
"at least 1 input key is needed for ZUNIONSTORE/ZINTERSTORE";
public static final String INTERNAL_SERVER_ERROR = "Internal server error: ";
+
+ /** System Properties **/
+ public static final String REDIS_REGION_NAME = "geode-for-redis-region-name";
Review comment:
I think this name is misleading (it makes me think the value is the name
of the redis region). What about changing it to "GEODE_FOR_REDIS_REGION_NAME"
or "REDIS_REGION_NAME_SYSTEM_PROPERTY"?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
