dschneider-pivotal commented on a change in pull request #6960:
URL: https://github.com/apache/geode/pull/6960#discussion_r732009517
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -55,7 +57,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 =
DEFAULT_REDIS_REGION_NAME;
Review comment:
RegionProvider should know the default region name. It should not need
to ask NettyRedisServer what it is (by importing its static).
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java
##########
@@ -94,11 +97,13 @@
private Subject subject;
static {
- String resourcePermission = System.getProperty("redis.resource-permission",
- "DATA:WRITE:" + RegionProvider.REDIS_DATA_REGION);
+ String regionName =
+ getStringSystemProperty(REDIS_REGION_NAME_PROPERTY,
DEFAULT_REDIS_DATA_REGION, false);
Review comment:
add a static method on RegionProvider that calls getStringSystemProperty
like this. Then all this class needs to do is call
RegionProvider.getRegionName().
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/ExecutionHandlerContext.java
##########
@@ -94,11 +97,13 @@
private Subject subject;
static {
- String resourcePermission = System.getProperty("redis.resource-permission",
- "DATA:WRITE:" + RegionProvider.REDIS_DATA_REGION);
+ String regionName =
+ getStringSystemProperty(REDIS_REGION_NAME_PROPERTY,
DEFAULT_REDIS_DATA_REGION, false);
+ String resourcePermission =
System.getProperty("redis.resource-permission", "DATA:WRITE:"
+ + regionName);
String[] parts = resourcePermission.split(":");
if (parts.length != 3) {
- parts = new String[] {"DATA", "WRITE", RegionProvider.REDIS_DATA_REGION};
+ parts = new String[] {"DATA", "WRITE", regionName};
}
RESOURCE_PERMISSION = new ResourcePermission(parts[0], parts[1], parts[2]);
Review comment:
ResourcePermission will throw IllegalArgumentException if the first two
strings are not in the corresponding enum. Since this code wants to revert to
defaults if the sys prop gave an illegal value, this constructor call should
catch IllegalArgumentException and then create a permission with the defaults.
You could simplify the error handling by changing the if body on parts.length
to just throw IllegalArgumentException. Then in the catch of
IlegalArgumentException just call new ResourcePermission("DATA", "WRITE",
regionName). You will probably need your own local "resourcePermission"
variable in this static block that you set to a value and then at the end of
the block set the static final RESOURCE_PERMISSION to resourcePermission.
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/RegionProvider.java
##########
@@ -89,7 +92,14 @@ public RegionProvider(InternalCache cache,
StripedCoordinator stripedCoordinator
attributesFactory.setTotalNumBuckets(REDIS_REGION_BUCKETS);
redisDataRegionFactory.setPartitionAttributes(attributesFactory.create());
- dataRegion = redisDataRegionFactory.create(REDIS_DATA_REGION);
+ String regionName = System.getProperty(REDIS_REGION_NAME_PROPERTY);
Review comment:
I think this should use getStringSystemProperty
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java
##########
@@ -68,6 +71,7 @@
public class NettyRedisServer {
private static final int RANDOM_PORT_INDICATOR = 0;
+ public static final String DEFAULT_REDIS_REGION_NAME = "GEODE_FOR_REDIS";
Review comment:
move this to RegionProvider
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java
##########
@@ -88,6 +92,7 @@
private final int serverPort;
private final DistributedMember member;
private final SecurityService securityService;
+ private final String regionName;
Review comment:
remove this field
--
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]
