You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/10/08 15:52:47 UTC
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6961: GEODE-9654: add system properties for timeouts
jdeppe-pivotal commented on a change in pull request #6961:
URL: https://github.com/apache/geode/pull/6961#discussion_r725123256
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java
##########
@@ -102,10 +108,30 @@ 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
+ try {
+ tempTimeout = Integer.parseInt(System.getProperty(REDIS_CONNECT_TIMEOUT_MILLIS));
+
+ if (tempTimeout <= 0) {
+ tempTimeout = DEFAULT_REDIS_CONNECT_TIMEOUT_MILLIS;
+ }
+ } catch (NumberFormatException e) {
+ tempTimeout = DEFAULT_REDIS_CONNECT_TIMEOUT_MILLIS;
}
+ this.connectTimeoutMillis = tempTimeout;
+
+ // get write timeout from system property
+ try {
+ tempTimeout = Integer.parseInt(System.getProperty(REDIS_WRITE_TIMEOUT_SECONDS));
+
+ if (tempTimeout <= 0) {
+ tempTimeout = DEFAULT_REDIS_WRITE_TIMEOUT_SECONDS;
+ }
+ } catch (NumberFormatException e) {
+ tempTimeout = DEFAULT_REDIS_WRITE_TIMEOUT_SECONDS;
+ }
+ this.writeTimeoutSeconds = tempTimeout;
Review comment:
Same as above - use `Integer.getInteger` instead.
##########
File path: geode-for-redis/src/commonTest/java/org/apache/geode/test/dunit/rules/RedisClusterStartupRule.java
##########
@@ -109,6 +109,50 @@ public int getRedisPort(MemberVM vm) {
});
}
+ public int getRedisConnectTimeoutMillis(int vmNumber) {
+ return getRedisConnectTimeoutMillis(getMember(vmNumber));
+ }
+
+ public int getRedisConnectTimeoutMillis(MemberVM vm) {
+ return vm.invoke(() -> {
+ GeodeRedisService service = ClusterStartupRule.getCache().getService(GeodeRedisService.class);
+ return service.getRedisServer().getConnectTimeoutMillis();
+ });
+ }
+
+ public int getRedisWriteTimeoutSeconds(int vmNumber) {
+ return getRedisConnectTimeoutMillis(getMember(vmNumber));
+ }
+
+ public int getRedisWriteTimeoutSeconds(MemberVM vm) {
+ return vm.invoke(() -> {
+ GeodeRedisService service = ClusterStartupRule.getCache().getService(GeodeRedisService.class);
+ return service.getRedisServer().getWriteTimeoutSeconds();
+ });
+ }
+
+ public int getRedisInitialDelayMinutes(int vmNumber) {
+ return getRedisInitialDelayMinutes(getMember(vmNumber));
+ }
+
+ public int getRedisInitialDelayMinutes(MemberVM vm) {
+ return vm.invoke(() -> {
+ GeodeRedisService service = ClusterStartupRule.getCache().getService(GeodeRedisService.class);
+ return service.getRedisServer().getInitialDelayMinutes();
+ });
+ }
+
+ public int getRedisDelayMinutes(int vmNumber) {
+ return getRedisDelayMinutes(getMember(vmNumber));
+ }
+
+ public int getRedisDelayMinutes(MemberVM vm) {
+ return vm.invoke(() -> {
+ GeodeRedisService service = ClusterStartupRule.getCache().getService(GeodeRedisService.class);
+ return service.getRedisServer().getDelayMinutes();
+ });
+ }
+
Review comment:
Since these are all very specific to one test, I think they could just be helper methods in `GeodeRedisServerStartupDUnitTest`.
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java
##########
@@ -1984,6 +1984,51 @@
* <p/>
*/
String REDIS_PORT = "geode-for-redis-port";
+ /**
+ * The static String definition of the <i>"geode-for-redis-connect-timeout-millis"</i> property <a
+ * name="geode-for-redis-connect-timeout-millis"/a>
+ * </p>
+ * <U>Description</U>: Specifies the timeout for Geode for Redis connections</td>
+ * </p>
+ * <U>Default</U>: 1000
+ * </p>
+ * <U>Minimum value</U>: 1
+ */
+ String REDIS_CONNECT_TIMEOUT_MILLIS = "geode-for-redis-connect-timeout-millis";
+ /**
+ * The static String definition of the <i>"geode-for-redis-write-timeout-seconds"</i> property <a
+ * name="geode-for-redis-write-timeout-seconds"/a>
+ * </p>
+ * <U>Description</U>: Specifies the timeout for Geode for Redis writes</td>
+ * </p>
+ * <U>Default</U>: 10
+ * </p>
+ * <U>Minimum value</U>: 1
+ */
+ String REDIS_WRITE_TIMEOUT_SECONDS = "geode-for-redis-write-timeout-seconds";
+ /**
+ * The static String definition of the <i>"geode-for-redis-initial-delay-minutes"</i> property <a
+ * name="geode-for-redis-initial-delay-minutes"/a>
+ * </p>
+ * <U>Description</U>: Specifies the initial delay before Geode for Redis passive expiration
+ * starts</td>
+ * </p>
+ * <U>Default</U>: 3
+ * </p>
+ * <U>Minimum value</U>: 1
+ */
+ String REDIS_INITIAL_DELAY_MINUTES = "geode-for-redis-initial-delay-minutes";
+ /**
+ * The static String definition of the <i>"geode-for-redis-interval-minutes"</i> property <a
+ * name="geode-for-redis-interval-minutes"/a>
+ * </p>
+ * <U>Description</U>: Specifies the interval for Geode for Redis passive expiration</td>
+ * </p>
+ * <U>Default</U>: 3
+ * </p>
+ * <U>Minimum value</U>: 1
+ */
+ String REDIS_INTERVAL_MINUTES = "geode-for-redis-interval-minutes";
Review comment:
The parameters in here are official GemFire properties. Our system properties could probably just go into `RedisConstants`.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/PassiveExpirationManager.java
##########
@@ -38,14 +40,49 @@
private static final Logger logger = LogService.getLogger();
private final ScheduledExecutorService expirationExecutor;
+ private final int initialDelay;
+ private final int interval;
@VisibleForTesting
- public static final int INTERVAL = 3;
+ public static final int DEFAULT_REDIS_INITIAL_DELAY_MINUTES = 3;
+ @VisibleForTesting
+ public static final int DEFAULT_REDIS_INTERVAL_MINUTES = 3;
public PassiveExpirationManager(RegionProvider regionProvider) {
+ int tempTimeout;
+ try {
+ tempTimeout = Integer.parseInt(System.getProperty(REDIS_INITIAL_DELAY_MINUTES));
+
+ if (tempTimeout < 0) {
+ tempTimeout = DEFAULT_REDIS_INITIAL_DELAY_MINUTES;
+ }
+ } catch (NumberFormatException e) {
+ tempTimeout = DEFAULT_REDIS_INITIAL_DELAY_MINUTES;
+ }
+ this.initialDelay = tempTimeout;
+
+ try {
+ tempTimeout = Integer.parseInt(System.getProperty(REDIS_INTERVAL_MINUTES));
+
+ if (tempTimeout <= 0) {
+ tempTimeout = DEFAULT_REDIS_INTERVAL_MINUTES;
+ }
+ } catch (NumberFormatException e) {
+ tempTimeout = DEFAULT_REDIS_INTERVAL_MINUTES;
+ }
+ this.interval = tempTimeout;
+
Review comment:
If you use `Integer.getInteger(<property>, <default>)` it will handle all of this including the case for invalid formatting. So here you can just do:
```
initialDelay = Integer.getInteger(REDIS_INITIAL_DELAY_MINUTES, DEFAULT_REDIS_INITIAL_DELAY_MINUTES);
interval = Integer.getInteger(REDIS_INTERVAL_MINUTES, DEFAULT_REDIS_INTERVAL_MINUTES);
```
--
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