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 00:45:35 UTC
[GitHub] [geode] nonbinaryprogrammer opened a new pull request #6961: GEODE-9654: add system properties for timeouts
nonbinaryprogrammer opened a new pull request #6961:
URL: https://github.com/apache/geode/pull/6961
Adds system properties for the following:
- geode-for-redis-connect-timeout-millis
- geode-for-redis-write-timeout-seconds
- geode-for-redis-initial-delay-minutes
- geode-for-redis-interval-minutes
If they are set to non-integer, negative, or zero (except initial delay, for which 0 is valid), the default values will quietly be used.
<!-- Thank you for submitting a contribution to Apache Geode. -->
<!-- In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
-->
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
<!-- Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
-->
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6961: GEODE-9654: add system properties for timeouts
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6961:
URL: https://github.com/apache/geode/pull/6961#discussion_r732287530
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/RedisProperties.java
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal;
+
+
+public class RedisProperties {
+ /** System Property Names **/
+ public static final String WRITE_TIMEOUT_SECONDS = "geode-for-redis-write-timeout-seconds";
+ public static final String EXPIRATION_INTERVAL_SECONDS =
+ "geode-for-redis-expiration-interval-seconds";
+
+
+ /** assumes that default is greater than or equal to minValue **/
+ public static int getIntegerSystemProperty(String propName, int defaultValue, int minValue) {
+ int geodeValue = Integer.getInteger("geode." + propName, defaultValue);
+ int gemfireValue = Integer.getInteger("gemfire." + propName, defaultValue);
+
+ if (geodeValue != defaultValue && geodeValue > minValue) {
Review comment:
indeed it should, thanks for catching that. I added the test for that
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6961: GEODE-9654: add system properties for timeouts
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6961:
URL: https://github.com/apache/geode/pull/6961#discussion_r725202552
##########
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:
oh that's cool, I didn't know about that
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6961: GEODE-9654: add system properties for timeouts
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6961:
URL: https://github.com/apache/geode/pull/6961#discussion_r725312250
##########
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:
yeah that was a copy/paste mistake. thanks for catching that
--
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
[GitHub] [geode] nonbinaryprogrammer merged pull request #6961: GEODE-9654: add system properties for timeouts
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer merged pull request #6961:
URL: https://github.com/apache/geode/pull/6961
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6961: GEODE-9654: add system properties for timeouts
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6961:
URL: https://github.com/apache/geode/pull/6961#discussion_r728342835
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/server/AbstractHitsMissesIntegrationTest.java
##########
@@ -134,7 +134,8 @@ public void testExpire() {
public void testPassiveExpiration() {
runCommandAndAssertNoStatUpdates(HASH_KEY, (k) -> {
jedis.expire(k, 1L);
- GeodeAwaitility.await().atMost(Duration.ofMinutes(PassiveExpirationManager.INTERVAL * 2))
+ GeodeAwaitility.await().atMost(
+ Duration.ofMinutes(DEFAULT_REDIS_EXPIRATION_INTERVAL_SECONDS * 2))
Review comment:
indeed it should, thank you.
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6961: GEODE-9654: add system properties for timeouts
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6961:
URL: https://github.com/apache/geode/pull/6961#discussion_r725207006
##########
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:
that's fair
--
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
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6961: GEODE-9654: add system properties for timeouts
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6961:
URL: https://github.com/apache/geode/pull/6961#discussion_r728206045
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisProperties.java
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.data;
Review comment:
I don't think this new class belongs in the "data" package. I think it should be in "org.apache.geode.redis.internal" just like RedisConstants
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisProperties.java
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.data;
+
+public class RedisProperties {
+ public static int getIntegerSystemProperty(String propName, int defaultValue, int minValue) {
+ int timeout = Integer.getInteger(propName, defaultValue);
Review comment:
I noticed no support was added for a "geode." or "gemfire." prefix on the sys prop name. I thought we had decided to do that but could be remembering wrong. Or it could be done in its own PR instead of this one.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java
##########
@@ -130,7 +135,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, connectTimeoutSeconds * 1000)
Review comment:
I thought we had decided to remove this childOption instead of adding a sys prop for it. Check with @jdeppe-pivotal
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java
##########
@@ -72,4 +72,10 @@
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 CONNECT_TIMEOUT_SECONDS = "geode-for-redis-connect-timeout-seconds";
Review comment:
I think all these system property names should be moved to the new RedisProperties class.
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6961: GEODE-9654: add system properties for timeouts
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6961:
URL: https://github.com/apache/geode/pull/6961#discussion_r725313359
##########
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:
but a negative timeout doesn't make any sense. and a zero timeout would cause it to check constantly, which would have some interesting performance impacts. and the problem with doing a helper method in RedisProperties is the validation of >/>= 0
--
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6961: GEODE-9654: add system properties for timeouts
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6961:
URL: https://github.com/apache/geode/pull/6961#discussion_r728189657
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/server/AbstractHitsMissesIntegrationTest.java
##########
@@ -134,7 +134,8 @@ public void testExpire() {
public void testPassiveExpiration() {
runCommandAndAssertNoStatUpdates(HASH_KEY, (k) -> {
jedis.expire(k, 1L);
- GeodeAwaitility.await().atMost(Duration.ofMinutes(PassiveExpirationManager.INTERVAL * 2))
+ GeodeAwaitility.await().atMost(
+ Duration.ofMinutes(DEFAULT_REDIS_EXPIRATION_INTERVAL_SECONDS * 2))
Review comment:
Should this be `ofSeconds`?
--
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
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6961: GEODE-9654: add system properties for timeouts
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6961:
URL: https://github.com/apache/geode/pull/6961#discussion_r725314438
##########
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:
actually, I take that back, validation isn't an issue. I'll do this
--
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
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6961: GEODE-9654: add system properties for timeouts
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6961:
URL: https://github.com/apache/geode/pull/6961#discussion_r728206045
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisProperties.java
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.data;
Review comment:
I don't think this new class belongs in the "data" package. I think it should be in "org.apache.geode.redis.internal" just like RedisConstants
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisProperties.java
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.data;
+
+public class RedisProperties {
+ public static int getIntegerSystemProperty(String propName, int defaultValue, int minValue) {
+ int timeout = Integer.getInteger(propName, defaultValue);
Review comment:
I noticed no support was added for a "geode." or "gemfire." prefix on the sys prop name. I thought we had decided to do that but could be remembering wrong. Or it could be done in its own PR instead of this one.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java
##########
@@ -130,7 +135,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, connectTimeoutSeconds * 1000)
Review comment:
I thought we had decided to remove this childOption instead of adding a sys prop for it. Check with @jdeppe-pivotal
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java
##########
@@ -72,4 +72,10 @@
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 CONNECT_TIMEOUT_SECONDS = "geode-for-redis-connect-timeout-seconds";
Review comment:
I think all these system property names should be moved to the new RedisProperties class.
--
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6961: GEODE-9654: add system properties for timeouts
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6961:
URL: https://github.com/apache/geode/pull/6961#discussion_r727299463
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisProperties.java
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.data;
+
+public class RedisProperties {
+ public static int getTimeoutProperty(String propName, int defaultTimeout, int minValue) {
Review comment:
Since there is nothing timeout-specific here, this method might be better named `getSystemProperty`
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/PassiveExpirationManager.java
##########
@@ -38,14 +40,27 @@
private static final Logger logger = LogService.getLogger();
private final ScheduledExecutorService expirationExecutor;
-
- @VisibleForTesting
- public static final int INTERVAL = 3;
+ private final int initialDelay;
+ private final int interval;
public PassiveExpirationManager(RegionProvider regionProvider) {
+ this.initialDelay =
Review comment:
I don't like having all of these getters just to facilitate testing. In reality these values are being derived from a static context (system properties) so they could just be `static final`s.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java
##########
@@ -72,4 +72,15 @@
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 CONNECT_TIMEOUT_SECONDS = "geode-for-redis-connect-timeout-seconds";
+ public static final String WRITE_TIMEOUT_SECONDS = "geode-for-redis-write-timeout-seconds";
+ public static final String EXPIRATION_INTERVAL_SECONDS =
+ "geode-for-redis-expiration-interval-seconds";
+
+ /** Default Values for System Properties **/
+ public static final int DEFAULT_REDIS_CONNECT_TIMEOUT_SECONDS = 1;
+ public static final int DEFAULT_REDIS_WRITE_TIMEOUT_SECONDS = 10;
+ public static final int DEFAULT_REDIS_EXPIRATION_INTERVAL_SECONDS = 180;
Review comment:
I think it's more appropriate and idiomatic to place these defaults right in the classes that use them.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java
##########
@@ -101,11 +106,10 @@ public NettyRedisServer(Supplier<DistributionConfig> configSupplier,
this.redisStats = redisStats;
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);
- }
+ this.connectTimeoutSeconds =
+ getTimeoutProperty(CONNECT_TIMEOUT_SECONDS, DEFAULT_REDIS_CONNECT_TIMEOUT_SECONDS, 1);
+ this.writeTimeoutSeconds =
+ getTimeoutProperty(WRITE_TIMEOUT_SECONDS, DEFAULT_REDIS_WRITE_TIMEOUT_SECONDS, 1);
Review comment:
Similar comment to what I had in `PassiveExpirationManager`. I think it's a really good idea to use a helper method to retrieve these system properties. If you just test the helper method then I don't think there should be such a big burden to test every system property.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/server/AbstractHitsMissesIntegrationTest.java
##########
@@ -134,7 +134,8 @@ public void testExpire() {
public void testPassiveExpiration() {
runCommandAndAssertNoStatUpdates(HASH_KEY, (k) -> {
jedis.expire(k, 1L);
- GeodeAwaitility.await().atMost(Duration.ofMinutes(PassiveExpirationManager.INTERVAL * 2))
+ GeodeAwaitility.await().atMost(
+ Duration.ofMinutes(DEFAULT_REDIS_EXPIRATION_INTERVAL_SECONDS * 2))
Review comment:
Should this be `ofSeconds`?
--
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6961: GEODE-9654: add system properties for timeouts
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6961: GEODE-9654: add system properties for timeouts
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6961:
URL: https://github.com/apache/geode/pull/6961#discussion_r726299931
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/GeodeRedisServer.java
##########
@@ -132,6 +132,22 @@ public int getPort() {
return nettyRedisServer.getPort();
}
+ public int getConnectTimeoutMillis() {
Review comment:
do we need to expose all of these as attributes on GeodeRedisServer? I'm just thinking that this could end up adding a bunch of gettors on GeodeRedisServer. Are they only being exposed for testing? Could we instead ask GeodeRedisServer for the underlying component (for example nettyRedisServer) and then ask the component for its timeout?
--
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
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6961: GEODE-9654: add system properties for timeouts
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6961:
URL: https://github.com/apache/geode/pull/6961#discussion_r732280253
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/RedisProperties.java
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal;
+
+
+public class RedisProperties {
+ /** System Property Names **/
+ public static final String WRITE_TIMEOUT_SECONDS = "geode-for-redis-write-timeout-seconds";
+ public static final String EXPIRATION_INTERVAL_SECONDS =
+ "geode-for-redis-expiration-interval-seconds";
+
+
+ /** assumes that default is greater than or equal to minValue **/
+ public static int getIntegerSystemProperty(String propName, int defaultValue, int minValue) {
+ int geodeValue = Integer.getInteger("geode." + propName, defaultValue);
+ int gemfireValue = Integer.getInteger("gemfire." + propName, defaultValue);
+
+ if (geodeValue != defaultValue && geodeValue > minValue) {
Review comment:
it seems like both the ">" in this method should be ">=". Otherwise you would not be able to set you sys prop to the minValue. If I'm right about this update the unit test first to cover this case
--
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
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6961: GEODE-9654: add system properties for timeouts
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6961:
URL: https://github.com/apache/geode/pull/6961#discussion_r727299463
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisProperties.java
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.redis.internal.data;
+
+public class RedisProperties {
+ public static int getTimeoutProperty(String propName, int defaultTimeout, int minValue) {
Review comment:
Since there is nothing timeout-specific here, this method might be better named `getSystemProperty`
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/PassiveExpirationManager.java
##########
@@ -38,14 +40,27 @@
private static final Logger logger = LogService.getLogger();
private final ScheduledExecutorService expirationExecutor;
-
- @VisibleForTesting
- public static final int INTERVAL = 3;
+ private final int initialDelay;
+ private final int interval;
public PassiveExpirationManager(RegionProvider regionProvider) {
+ this.initialDelay =
Review comment:
I don't like having all of these getters just to facilitate testing. In reality these values are being derived from a static context (system properties) so they could just be `static final`s.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/RedisConstants.java
##########
@@ -72,4 +72,15 @@
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 CONNECT_TIMEOUT_SECONDS = "geode-for-redis-connect-timeout-seconds";
+ public static final String WRITE_TIMEOUT_SECONDS = "geode-for-redis-write-timeout-seconds";
+ public static final String EXPIRATION_INTERVAL_SECONDS =
+ "geode-for-redis-expiration-interval-seconds";
+
+ /** Default Values for System Properties **/
+ public static final int DEFAULT_REDIS_CONNECT_TIMEOUT_SECONDS = 1;
+ public static final int DEFAULT_REDIS_WRITE_TIMEOUT_SECONDS = 10;
+ public static final int DEFAULT_REDIS_EXPIRATION_INTERVAL_SECONDS = 180;
Review comment:
I think it's more appropriate and idiomatic to place these defaults right in the classes that use them.
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java
##########
@@ -101,11 +106,10 @@ public NettyRedisServer(Supplier<DistributionConfig> configSupplier,
this.redisStats = redisStats;
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);
- }
+ this.connectTimeoutSeconds =
+ getTimeoutProperty(CONNECT_TIMEOUT_SECONDS, DEFAULT_REDIS_CONNECT_TIMEOUT_SECONDS, 1);
+ this.writeTimeoutSeconds =
+ getTimeoutProperty(WRITE_TIMEOUT_SECONDS, DEFAULT_REDIS_WRITE_TIMEOUT_SECONDS, 1);
Review comment:
Similar comment to what I had in `PassiveExpirationManager`. I think it's a really good idea to use a helper method to retrieve these system properties. If you just test the helper method then I don't think there should be such a big burden to test every 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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nonbinaryprogrammer commented on a change in pull request #6961: GEODE-9654: add system properties for timeouts
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6961:
URL: https://github.com/apache/geode/pull/6961#discussion_r725314082
##########
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:
all of the default values I used were what was already default in our code. I do think it's worth having a conversation about what reasonable values are for all these. that might be better as a synchronous conversation on Monday (or whenever people have time)
--
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
[GitHub] [geode] upthewaterspout commented on a change in pull request #6961: GEODE-9654: add system properties for timeouts
Posted by 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