[GitHub] [geode] upthewaterspout commented on a change in pull request #6961: GEODE-9654: add system properties for timeouts

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

upthewaterspout commented on a change in pull request #6961:
URL: https://github.com/apache/geode/pull/6961#discussion_r725218933



##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java
##########
@@ -130,7 +145,7 @@ private Channel createChannel(int port) {
             .option(ChannelOption.SO_REUSEADDR, true)
             .option(ChannelOption.SO_RCVBUF, getBufferSize())
             .childOption(ChannelOption.SO_KEEPALIVE, true)
-            .childOption(ChannelOption.CONNECT_TIMEOUT_MILLIS, CONNECT_TIMEOUT_MILLIS)
+            .childOption(ChannelOption.CONNECT_TIMEOUT_MILLIS, DEFAULT_REDIS_CONNECT_TIMEOUT_MILLIS)

Review comment:
       This doesn't seem to be using the new property, just the default?

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java
##########
@@ -71,4 +72,16 @@
       "AUTH called without a Security Manager configured.";
   public static final String ERROR_KEY_REQUIRED =
       "at least 1 input key is needed for ZUNIONSTORE/ZINTERSTORE";
+
+  /** System Properties **/
+  public static final String CONNECT_TIMEOUT_MILLIS = "geode-for-redis-connect-timeout-millis";
+  public static final String WRITE_TIMEOUT_SECONDS = "geode-for-redis-write-timeout-seconds";
+  public static final String INITIAL_DELAY_MINUTES = "geode-for-redis-initial-delay-minutes";
+  public static final String INTERVAL_MINUTES = "geode-for-redis-interval-minutes";

Review comment:
       Are minutes a reasonable unit for this? I would suggest milliseconds or maybe seconds.

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java
##########
@@ -71,4 +72,16 @@
       "AUTH called without a Security Manager configured.";
   public static final String ERROR_KEY_REQUIRED =
       "at least 1 input key is needed for ZUNIONSTORE/ZINTERSTORE";
+
+  /** System Properties **/
+  public static final String CONNECT_TIMEOUT_MILLIS = "geode-for-redis-connect-timeout-millis";
+  public static final String WRITE_TIMEOUT_SECONDS = "geode-for-redis-write-timeout-seconds";
+  public static final String INITIAL_DELAY_MINUTES = "geode-for-redis-initial-delay-minutes";
+  public static final String INTERVAL_MINUTES = "geode-for-redis-interval-minutes";
+
+  /** Default Values for System Properties **/
+  public static final int DEFAULT_REDIS_CONNECT_TIMEOUT_MILLIS = 1000;

Review comment:
       This seems really low? For comparison, the geode cache server has a 60 second handshake timeout. That seems much more reasonable.

##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java
##########
@@ -102,10 +106,21 @@ public NettyRedisServer(Supplier<DistributionConfig> configSupplier,
     this.member = member;
     this.securityService = securityService;
 
-    if (port < RANDOM_PORT_INDICATOR) {
-      throw new IllegalArgumentException(
-          "The geode-for-redis-port cannot be less than " + RANDOM_PORT_INDICATOR);
+    int tempTimeout;
+    // get connect timeout from system property
+    tempTimeout = Integer.getInteger(RedisConstants.CONNECT_TIMEOUT_MILLIS,
+        DEFAULT_REDIS_CONNECT_TIMEOUT_MILLIS);
+    if (tempTimeout <= 0) {
+      tempTimeout = DEFAULT_REDIS_CONNECT_TIMEOUT_MILLIS;
+    }
+    this.connectTimeoutMillis = tempTimeout;
+
+    // get write timeout from system property
+    tempTimeout = Integer.getInteger(WRITE_TIMEOUT_SECONDS, DEFAULT_REDIS_WRITE_TIMEOUT_SECONDS);
+    if (tempTimeout <= 0) {
+      tempTimeout = DEFAULT_REDIS_WRITE_TIMEOUT_SECONDS;
     }
+    this.writeTimeoutSeconds = tempTimeout;

Review comment:
       I think this whole pattern could be a helper method something like this, rather than duplicating the same getInteger/ if / .. logic
   ```
   writeTimeoutSeconds = RedisProperties.getProperty(WRITE_TIMEOUT_SECONDS, DEFAULT_WRITE_TIMEOUT_SECONDS)
   ```
   
   I'm also not sure if we really need to validate that it is greater than 0? I'd be fine with just a constant initialized with Integer.getInteger




-- 
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: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org