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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
