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 2020/05/20 22:01:04 UTC
[GitHub] [geode] sabbeyPivotal opened a new pull request #5142: GEODE-8138: Improve semantics of the redis-port option
sabbeyPivotal opened a new pull request #5142:
URL: https://github.com/apache/geode/pull/5142
- Introduce a Geode parameter - `redis-service-enabled` which is `false` by default. This parameter determines whether the service is started or not.
- Set the default `redis-port` value to `6379`
- `redis-port` range is 0..65535
- When `redis-port=0`, and `redis-service-enabled=true` the service will start on an ephemeral port.
- When `gfsh` is used with `--redis-port` (or `--redis-bind-address`) it should automatically set `redis-service-enabled=true`
`gemfire_properties.html.md.erb` updated as part of this PR: https://github.com/apache/geode/pull/5130
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5142: GEODE-8138: Improve semantics of the redis-port option
Posted by GitBox <gi...@apache.org>.
sabbeyPivotal commented on a change in pull request #5142:
URL: https://github.com/apache/geode/pull/5142#discussion_r429255221
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java
##########
@@ -1943,6 +1931,36 @@
* <U>Default</U>: ""
*/
String REDIS_PASSWORD = "redis-password";
+ /**
+ * The static String definition of the <i>"redis-port"</i> property <a name="redis-port"/a>
+ * </p>
+ * <U>Description</U>: Specifies the port used by Redis API for Geode which enables Redis
+ * clients to connect and store data in GemFire distributed system. A value of 0 will select a
+ * random port.
+ * </p>
+ * <U>Default</U>: "6379"
+ * </p>
+ * <U>Allowed values</U>: 0..65535
+ */
+ String REDIS_PORT = "redis-port";
+ /**
+ * The static String definition of the <i>"redis-service-enabled"</i> property <a
+ * name="redis-service-enabled"/a>
+ * </p>
+ * <U>Description</U>: When the default value of false, the Redis API for Geode is not available.
+ * Set to true to enable the Redis API for Geode.</td>
+ * </p>
+ * <U>Default</U>: false
+ * <td>redis-service-enabled</td>
+ * <td>When the default value of false, the Redis API for <%=vars.product_name%> is not available.
+ * Set
+ * to true to enable the Redis API for <%=vars.product_name%>.</td>
+ * <td>S</td>
+ * <td>false</td>
+ * </tr>
+ * </p>
+ */
+ String REDIS_SERVICE_ENABLED = "redis-service-enabled";
Review comment:
Yeah, we can make it `redis-enabled`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5142: GEODE-8138: Improve semantics of the redis-port option
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5142: GEODE-8138: Improve semantics of the redis-port option
Posted by GitBox <gi...@apache.org>.
sabbeyPivotal commented on a change in pull request #5142:
URL: https://github.com/apache/geode/pull/5142#discussion_r428639899
##########
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:
Definitely.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5142: GEODE-8138: Improve semantics of the redis-port option
Posted by GitBox <gi...@apache.org>.
sabbeyPivotal commented on a change in pull request #5142:
URL: https://github.com/apache/geode/pull/5142#discussion_r428635821
##########
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:
Yes!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5142: GEODE-8138: Improve semantics of the redis-port option
Posted by GitBox <gi...@apache.org>.
sabbeyPivotal commented on a change in pull request #5142:
URL: https://github.com/apache/geode/pull/5142#discussion_r428640516
##########
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:
That is currently what happens. I like what you're suggesting though.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5142: GEODE-8138: Improve semantics of the redis-port option
Posted by GitBox <gi...@apache.org>.
sabbeyPivotal commented on a change in pull request #5142:
URL: https://github.com/apache/geode/pull/5142#discussion_r428653882
##########
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:
Updated it!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #5142: GEODE-8138: Improve semantics of the redis-port option
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #5142:
URL: https://github.com/apache/geode/pull/5142#discussion_r429007050
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java
##########
@@ -1943,6 +1931,36 @@
* <U>Default</U>: ""
*/
String REDIS_PASSWORD = "redis-password";
+ /**
+ * The static String definition of the <i>"redis-port"</i> property <a name="redis-port"/a>
+ * </p>
+ * <U>Description</U>: Specifies the port used by Redis API for Geode which enables Redis
+ * clients to connect and store data in GemFire distributed system. A value of 0 will select a
+ * random port.
+ * </p>
+ * <U>Default</U>: "6379"
+ * </p>
+ * <U>Allowed values</U>: 0..65535
+ */
+ String REDIS_PORT = "redis-port";
+ /**
+ * The static String definition of the <i>"redis-service-enabled"</i> property <a
+ * name="redis-service-enabled"/a>
+ * </p>
+ * <U>Description</U>: When the default value of false, the Redis API for Geode is not available.
+ * Set to true to enable the Redis API for Geode.</td>
+ * </p>
+ * <U>Default</U>: false
+ * <td>redis-service-enabled</td>
+ * <td>When the default value of false, the Redis API for <%=vars.product_name%> is not available.
+ * Set
+ * to true to enable the Redis API for <%=vars.product_name%>.</td>
+ * <td>S</td>
+ * <td>false</td>
+ * </tr>
+ * </p>
+ */
+ String REDIS_SERVICE_ENABLED = "redis-service-enabled";
Review comment:
Why add in the word "service" here? Why not just "redis-enabled". None of the other redis properties use the word "service". I think consistent naming conventions are really important to avoid confusion.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal merged pull request #5142: GEODE-8138: Improve semantics of the redis-port option
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal merged pull request #5142:
URL: https://github.com/apache/geode/pull/5142
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5142: GEODE-8138: Improve semantics of the redis-port option
Posted by GitBox <gi...@apache.org>.
sabbeyPivotal commented on a change in pull request #5142:
URL: https://github.com/apache/geode/pull/5142#discussion_r428632736
##########
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:
Yes. Thank you for finding 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on a change in pull request #5142: GEODE-8138: Improve semantics of the redis-port option
Posted by GitBox <gi...@apache.org>.
sabbeyPivotal commented on a change in pull request #5142:
URL: https://github.com/apache/geode/pull/5142#discussion_r428637710
##########
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:
You are right. What do you think? Maybe it would make sense to enable it if any redis option is specified?
----------------------------------------------------------------
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:
users@infra.apache.org