[
https://issues.apache.org/jira/browse/GEODE-8138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17112696#comment-17112696
]
ASF GitHub Bot commented on GEODE-8138:
---------------------------------------
dschneider-pivotal commented on a change in pull request #5142:
URL: https://github.com/apache/geode/pull/5142#discussion_r428390890
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/internal/AbstractDistributionConfig.java
##########
@@ -1312,11 +1313,13 @@ static Class _getAttributeType(String attName) {
m.put(MEMCACHED_BIND_ADDRESS,
"The address the GemFireMemcachedServer will listen on for remote
connections. Default is \"\" which causes the GemFireMemcachedServer to listen
on the host's default address. This property is ignored if memcached-port is
\"0\".");
m.put(REDIS_PORT,
- "The port GeodeRedisServer will listen on. Default is 0. Set to zero
to disable GeodeRedisServer.");
+ "The port Redis API for Geode will listen on. Default is 0");
Review comment:
Is the default now 6379? Also I think you want a "." at the end of the
sentence.
##########
File path:
geode-core/src/main/java/org/apache/geode/management/internal/i18n/CliStrings.java
##########
@@ -2557,14 +2557,14 @@
"The gemfire.properties file for configuring the Cache Server's
distributed system. The file's path can be absolute or relative to the gfsh
working directory.";
public static final String START_SERVER__REDIS_PORT =
ConfigurationProperties.REDIS_PORT;
public static final String START_SERVER__REDIS_PORT__HELP =
- "Sets the port that the Geode Redis service listens on for Redis
clients.";
+ "Sets the port that the Redis API for Geode server listens on for Redis
clients. The default is an ephemeral port";
Review comment:
For gfsh start server I thought the default would be 6379
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/GeodeRedisServer.java
##########
@@ -275,7 +275,8 @@ public GeodeRedisServer(String bindAddress, int port) {
*
* @param bindAddress The address to which the server will attempt to bind to
* @param port The port the server will bind to, will use {@value
#DEFAULT_REDIS_SERVER_PORT}
- * by default if argument is less than -1. If the port is {@value
#RANDOM_PORT_INDICATOR}
+ * by default if argument is less than {@value
#RANDOM_PORT_INDICATOR}. If the port is
Review comment:
Is it correct that if it is < RANDOM_PORT_INDICATOR it will use the
default? I couldn't find the code that made that happen but it seems odd if I
say -33456 it will listen on 6379. I think I'd prefer an
IllegalArgumentException
##########
File path:
geode-redis/src/main/java/org/apache/geode/redis/internal/GeodeRedisServer.java
##########
@@ -241,7 +241,7 @@ private int setNumWorkerThreads() {
* to the first non-loopback address
*/
public GeodeRedisServer() {
- this(null, -1, null);
+ this(null, 0, null);
Review comment:
Should this use RANDOM_PORT_INDICATOR instead of 0?
##########
File path:
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StartServerCommand.java
##########
@@ -301,6 +301,13 @@ ResultModel doStartServer(String memberName, Boolean
assignBuckets, String bindA
ConfigurationProperties.HTTP_SERVICE_PORT, httpServicePort);
StartMemberUtils.setPropertyIfNotNull(gemfireProperties,
ConfigurationProperties.HTTP_SERVICE_BIND_ADDRESS,
httpServiceBindAddress);
+
+ // if redis-port or redis-bind-address are specified in the command line,
+ // REDIS_SERVICE_ENABLED should be set to true
+ if (StringUtils.isNotBlank(redisPassword) ||
StringUtils.isNotBlank(redisBindAddress)) {
Review comment:
I see this checking the password and bindAddress setting. But shouldn't
it also be checking the redisPort? The comment says port and bind-address.
Perhaps any redis option (including password) should enable redis.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> Improve semantics of the redis-port option
> ------------------------------------------
>
> Key: GEODE-8138
> URL: https://issues.apache.org/jira/browse/GEODE-8138
> Project: Geode
> Issue Type: New Feature
> Components: redis
> Reporter: Jens Deppe
> Priority: Major
>
> Currently when {{redis-port=0}} the Redis service is not enabled. Recently we
> added the ability to pick an ephemeral port by setting {{redis-port=-1}}.
> However the typical norm is that a port of {{0}} should indicate the use of
> an ephemeral port.
> If we allow this then we need a different way of enabling/disabling the
> service.
> One possibility is that the service is disabled when {{redis-port=-1}}. (or <
> 0). However anyone already setting the property to {{0}} would then suddenly
> activate the service.
> Another possibility is to add a new Geode property to explicitly
> enable/disable the service. (Off by default). That way users, switching to
> this version, would still experience the same behavior regardless of their
> {{redis-port}} setting.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
