dschneider-pivotal commented on a change in pull request #6960:
URL: https://github.com/apache/geode/pull/6960#discussion_r734725815



##########
File path: 
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/connection/AuthIntegrationTest.java
##########
@@ -99,6 +105,14 @@ private void setupCache(String username, boolean 
withSecurityManager) throws Exc
     }
   }
 
+  private void setupCacheWithRegionName(String username, String regionName,
+      boolean withSecurityManager) throws Exception {
+    System.setProperty("io.netty.eventLoopThreads", "1");
+    System.setProperty(REDIS_REGION_NAME_PROPERTY, regionName);

Review comment:
       a way exists (I think it is a rule) so safely set system properties in 
our tests have have the test framework automatically change it back at the end 
of the test. I'm unable to find it right now. @kirklund knows what it is.
   If you can't find that or use it for some reason then you should put the 
"clearProperty" call in a finally block just in case setupCache throws an 
exception. This will prevent one test failure for causing other tests to fail

##########
File path: 
geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/connection/AuthIntegrationTest.java
##########
@@ -99,6 +105,14 @@ private void setupCache(String username, boolean 
withSecurityManager) throws Exc
     }
   }
 
+  private void setupCacheWithRegionName(String username, String regionName,
+      boolean withSecurityManager) throws Exception {
+    System.setProperty("io.netty.eventLoopThreads", "1");

Review comment:
       why is io.netty.eventLoopThreads being set?

##########
File path: 
geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java
##########
@@ -104,9 +107,16 @@ public NettyRedisServer(Supplier<DistributionConfig> 
configSupplier,
     this.member = member;
     this.securityService = securityService;
 
+    this.regionName =
+        getStringSystemProperty(REDIS_REGION_NAME_PROPERTY, 
DEFAULT_REDIS_REGION_NAME);
     this.writeTimeoutSeconds =
         getIntegerSystemProperty(WRITE_TIMEOUT_SECONDS, 
DEFAULT_REDIS_WRITE_TIMEOUT_SECONDS, 1);
 
+    if (port < RANDOM_PORT_INDICATOR) {

Review comment:
       why is this port stuff added in this pr? I think geode already did this 
validation because of what is defined in DistributionConfig:  
     @ConfigAttribute(type = Integer.class, min = 0, max = 65535)
     String REDIS_PORT_NAME = REDIS_PORT;
     int DEFAULT_REDIS_PORT = 6379;
   




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


Reply via email to