Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GEODE-9869: Remove geode-for-redis properties from gfsh options #7205

Merged
merged 7 commits into from Jan 10, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 7 additions & 5 deletions ci/scripts/execute_redis_tests.sh
Expand Up @@ -33,9 +33,10 @@ export JAVA_HOME=${JAVA_TEST_PATH}
../geode-assembly/build/install/apache-geode/bin/gfsh start server \
--J=-Denable-unsupported-commands=true \
--name=server1 \
--geode-for-redis-port=6380 \
--geode-for-redis-bind-address=127.0.0.1 \
--geode-for-redis-username=foobar \
--J=-Dgemfire.geode-for-redis-enabled=true \
--J=-Dgemfire.geode-for-redis-port=6380 \
--J=-Dgemfire.geode-for-redis-bind-address=127.0.0.1 \
--J=-Dgemfire.geode-for-redis-username=foobar \
--server-port=0 \
--J=-Dgemfire.security-manager=org.apache.geode.examples.SimpleSecurityManager \
--J=-Dgemfire.jmx-manager=true \
Expand All @@ -58,8 +59,9 @@ failCount=0
--J=-Denable-unsupported-commands=true \
--name=server2 \
--server-port=0 \
--geode-for-redis-port=6379 \
--geode-for-redis-bind-address=127.0.0.1 \
--J=-Dgemfire.geode-for-redis-enabled=true \
--J=-Dgemfire.geode-for-redis-port=6379 \
--J=-Dgemfire.geode-for-redis-bind-address=127.0.0.1 \
--J=-Dgemfire.jmx-manager=true \
--J=-Dgemfire.jmx-manager-start=true \
--J=-Dgemfire.jmx-manager-port=1099
Expand Down
Expand Up @@ -2626,20 +2626,6 @@ public class CliStrings {
public static final String START_SERVER__PROPERTIES = "properties-file";
public static final String START_SERVER__PROPERTIES__HELP =
"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_BIND_ADDRESS =
ConfigurationProperties.GEODE_FOR_REDIS_BIND_ADDRESS;
public static final String START_SERVER__REDIS_BIND_ADDRESS__HELP =
"Specifies the address on which the Redis API for Geode is listening. "
+ "If set to the empty string or this property is not specified, the server listens on all local addresses.";
public static final String START_SERVER__REDIS_USERNAME =
ConfigurationProperties.GEODE_FOR_REDIS_USERNAME;
public static final String START_SERVER__REDIS_USERNAME__HELP =
"Specifies the username that the server uses when a client attempts to authenticate using only a password."
+ " This option also requires a SecurityManager to be configured.";
public static final String START_SERVER__REDIS_PORT =
ConfigurationProperties.GEODE_FOR_REDIS_PORT;
public static final String START_SERVER__REDIS_PORT__HELP =
"Specifies the port on which the server listens for Redis API for Geode connections. A value of 0 selects a random port. Default is 6379.";
public static final String START_SERVER__SECURITY_PROPERTIES = "security-properties-file";
public static final String START_SERVER__SECURITY_PROPERTIES__HELP =
"The gfsecurity.properties file for configuring the Server's security configuration in the distributed system. The file's path can be absolute or relative to gfsh directory.";
Expand Down
Expand Up @@ -2572,7 +2572,6 @@ SYNTAX\n\
\ \ \ \ [--lock-memory(=value)?] [--log-level=value] [--max-connections=value] [--max-heap=value]\n\
\ \ \ \ [--max-message-count=value] [--max-threads=value] [--mcast-address=value] [--mcast-port=value]\n\
\ \ \ \ [--memcached-port=value] [--memcached-protocol=value] [--memcached-bind-address=value]\n\
\ \ \ \ [--geode-for-redis-port=value] [--geode-for-redis-bind-address=value] [--geode-for-redis-password=value]\n\
\ \ \ \ [--message-time-to-live=value] [--off-heap-memory-size=value] [--properties-file=value]\n\
\ \ \ \ [--rebalance(=value)?] [--security-properties-file=value] [--server-bind-address=value]\n\
\ \ \ \ [--server-port=value] [--socket-buffer-size=value] [--spring-xml-location=value]\n\
Expand Down Expand Up @@ -2719,16 +2718,6 @@ PARAMETERS\n\
\ \ \ \ \ \ \ \ Sets the IP address the Geode memcached service listens on for memcached clients. The\n\
\ \ \ \ \ \ \ \ default is to bind to the first non-loopback address for this machine.\n\
\ \ \ \ \ \ \ \ Required: false\n\
\ \ \ \ geode-for-redis-port\n\
\ \ \ \ \ \ \ \ Sets the port that the Geode for Redis service listens on for Redis clients.\n\
\ \ \ \ \ \ \ \ Required: false\n\
\ \ \ \ geode-for-redis-bind-address\n\
\ \ \ \ \ \ \ \ Sets the IP address the Geode for Redis service listens on for Redis clients. The default is to\n\
\ \ \ \ \ \ \ \ bind to the first non-loopback address for this machine.\n\
\ \ \ \ \ \ \ \ Required: false\n\
\ \ \ \ geode-for-redis-password\n\
\ \ \ \ \ \ \ \ Sets the authentication password for GeodeRedisServer\n\
\ \ \ \ \ \ \ \ Required: false\n\
\ \ \ \ message-time-to-live\n\
\ \ \ \ \ \ \ \ Sets the time (in seconds ) after which a message in the client queue will expire\n\
\ \ \ \ \ \ \ \ Required: false\n\
Expand Down
6 changes: 0 additions & 6 deletions geode-docs/reference/topics/gemfire_properties.html.md.erb
Expand Up @@ -501,12 +501,6 @@ to true to enable <%=vars.product_name%> for Redis.</td>
<td>false</td>
</tr>
<tr>
<td>geode-for-redis-password</td>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove this one but not the bind address and username?

<td>Specifies the password that the server uses when a client attempts to authenticate.</td>
<td>S</td>
<td>no password set</td>
</tr>
<tr>
<td>geode-for-redis-port</td>
<td>Specifies the port on which the server listens for <%=vars.product_name%> for Redis connections. A value of 0 selects a random port.</td>
<td>S</td>
Expand Down
10 changes: 5 additions & 5 deletions geode-docs/tools_modules/geode_for_redis.html.md.erb
Expand Up @@ -35,13 +35,13 @@ Use gfsh to start at least one server with a command of the form:
start server \
--name=<serverName> \
--locators=<locatorPort> \
--geode-for-redis-port=<geodeForRedisPort> \
--geode-for-redis-bind-address=<geodeForRedisBindAddress> \
--geode-for-redis-password=<geodeForRedisPassword>
--J=-Dgemfire.geode-for-redis-enabled=true \
--J=-Dgemfire.geode-for-redis-port=<geodeForRedisPort> \
--J=-Dgemfire.geode-for-redis-bind-address=<geodeForRedisBindAddress>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do these have to be gemfire. or could they be geode.? just since we generally avoid referencing gemfire in geode

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it makes sense since other properties use that format, I just wanted us to think about if that is the right approach or if we're following a pattern that doesn't make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that was discussed and 'gemfire.' won, mostly on 'principle of least surprise' grounds.

```

If any of the options `geode-for-redis-bind-address`, `geode-for-redis-password`, or `geode-for-redis-port`
are included, a <%=vars.product_name%> server with <%=vars.product_name%> for Redis will be started.
If the gemfire property `geode-for-redis-enabled`, is set to `true`, a <%=vars.product_name%>
server with <%=vars.product_name%> for Redis will be started.

Replace `<serverName>` with the name of your server.

Expand Down
14 changes: 8 additions & 6 deletions geode-for-redis/README.md
Expand Up @@ -34,11 +34,12 @@ Once the locator has started, start at least one server with a command of the fo
gfsh> start server \
--name=<serverName> \
--locators=localhost[<locatorPort>] \
--geode-for-redis-port=<geodeForRedisPort> \
--geode-for-redis-bind-address=<geodeForRedisBindAddress>
--J=-Dgemfire.geode-for-redis-enabled=true \
--J=-Dgemfire.geode-for-redis-port=<geodeForRedisPort> \
--J=-Dgemfire.geode-for-redis-bind-address=<geodeForRedisBindAddress>
```

If any of the options `geode-for-redis-bind-address`, `geode-for-redis-username`, or `geode-for-redis-port` are included, a Geode server with Geode for Redis will be started.
If the option `geode-for-redis-enabled` is set to `true`, a Geode server with Geode for Redis will be started.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

option once again

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this needs a fix. Must be explicitly enabled now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except we're getting rid of the Redis README.md now, I think.


- Replace `<serverName>` with the name of your server.

Expand Down Expand Up @@ -105,9 +106,10 @@ The following gfsh command will configure a `SimpleSecurityManager`:
gfsh> start server \
--name=<serverName> \
--locators=<locatorPort> \
--geode-for-redis-port=<geodeForRedisPort> \
--geode-for-redis-bind-address=<geodeForRedisBindAddress> \
--geode-for-redis-username=<geodeForRedisUsername> \
--J=-Dgemfire.geode-for-redis-enabled=true \
--J=-Dgemfire.geode-for-redis-port=<geodeForRedisPort> \
--J=-Dgemfire.geode-for-redis-bind-address=<geodeForRedisBindAddress> \
--J=-Dgemfire.geode-for-redis-username=<geodeForRedisUsername> \
--J=-Dgemfire.security-manager=org.apache.geode.examples.SimpleSecurityManager
```

Expand Down
Expand Up @@ -43,8 +43,9 @@ public void shouldReturnErrorMessage_givenSamePortAndAddress() throws IOExceptio
"start server",
"--server-port", "0",
"--name", "same-port-and-address-server",
"--geode-for-redis-bind-address", "localhost",
"--geode-for-redis-port", String.valueOf(port));
"--J=-Dgemfire.geode-for-redis-enabled=true",
"--J=-Dgemfire.geode-for-redis-bind-address=localhost",
"--J=-Dgemfire.geode-for-redis-port=" + port);
GfshExecution execution;

try (ServerSocket interferingSocket = new ServerSocket()) {
Expand All @@ -66,7 +67,8 @@ public void shouldReturnErrorMessage_givenSamePortAndAllAddresses() throws IOExc
"start server",
"--server-port", "0",
"--name", "same-port-all-addresses-server",
"--geode-for-redis-port", String.valueOf(port));
"--J=-Dgemfire.geode-for-redis-enabled=true",
"--J=-Dgemfire.geode-for-redis-port=" + port);
GfshExecution execution;

try (ServerSocket interferingSocket = new ServerSocket()) {
Expand All @@ -86,7 +88,8 @@ public void shouldReturnErrorMessage_givenInvalidBindAddress() {
"start server",
"--server-port", "0",
"--name", "invalid-bind-server",
"--geode-for-redis-bind-address", "1.1.1.1");
"--J=-Dgemfire.geode-for-redis-enabled=true",
"--J=-Dgemfire.geode-for-redis-bind-address=1.1.1.1");
GfshExecution execution;

execution = GfshScript.of(startServerCommand)
Expand Down
4 changes: 2 additions & 2 deletions geode-for-redis/src/performanceTest/environment-setup.sh
Expand Up @@ -101,8 +101,8 @@ if [ ${SERVER_TYPE} == "geode" ]; then
--log-level=none
--locators=localhost[10334]
--server-port=0
--geode-for-redis-port=6379
--geode-for-redis-bind-address=127.0.0.1"
--J=-Dgemfire.geode-for-redis-port=6379
--J=-Dgemfire.geode-for-redis-bind-address=127.0.0.1"
else
if [ ${SERVER_NOT_FOUND} -eq 1 ]; then
echo "No server compatible with Redis detected on host '${REDIS_HOST}' at port '${REDIS_PORT}'"
Expand Down
Expand Up @@ -132,12 +132,6 @@ public ResultModel startServer(
help = CliStrings.START_SERVER__MEMCACHED_PROTOCOL__HELP) final String memcachedProtocol,
@CliOption(key = CliStrings.START_SERVER__MEMCACHED_BIND_ADDRESS,
help = CliStrings.START_SERVER__MEMCACHED_BIND_ADDRESS__HELP) final String memcachedBindAddress,
@CliOption(key = CliStrings.START_SERVER__REDIS_PORT,
help = CliStrings.START_SERVER__REDIS_PORT__HELP) final Integer redisPort,
@CliOption(key = CliStrings.START_SERVER__REDIS_BIND_ADDRESS,
help = CliStrings.START_SERVER__REDIS_BIND_ADDRESS__HELP) final String redisBindAddress,
@CliOption(key = CliStrings.START_SERVER__REDIS_USERNAME,
help = CliStrings.START_SERVER__REDIS_USERNAME__HELP) final String redisUsername,
@CliOption(key = CliStrings.START_SERVER__MESSAGE__TIME__TO__LIVE,
help = CliStrings.START_SERVER__MESSAGE__TIME__TO__LIVE__HELP) final Integer messageTimeToLive,
@CliOption(key = CliStrings.START_SERVER__OFF_HEAP_MEMORY_SIZE,
Expand Down Expand Up @@ -210,8 +204,8 @@ public ResultModel startServer(
evictionOffHeapPercentage, force, group, hostNameForClients, jmxManagerHostnameForClients,
includeSystemClasspath, initialHeap, jvmArgsOpts, locators, locatorWaitTime, lockMemory,
logLevel, maxConnections, maxHeap, maxMessageCount, maxThreads, mcastBindAddress, mcastPort,
memcachedPort, memcachedProtocol, memcachedBindAddress, redisPort, redisBindAddress,
redisUsername, messageTimeToLive, offHeapMemorySize, gemfirePropertiesFile, rebalance,
memcachedPort, memcachedProtocol, memcachedBindAddress, messageTimeToLive,
offHeapMemorySize, gemfirePropertiesFile, rebalance,
gemfireSecurityPropertiesFile, serverBindAddress, serverPort, socketBufferSize,
springXmlLocation, statisticsArchivePathname, requestSharedConfiguration, startRestApi,
httpServicePort, httpServiceBindAddress, userName, passwordToUse, redirectOutput);
Expand All @@ -227,7 +221,7 @@ ResultModel doStartServer(String memberName, Boolean assignBuckets, String bindA
Integer locatorWaitTime, Boolean lockMemory, String logLevel, Integer maxConnections,
String maxHeap, Integer maxMessageCount, Integer maxThreads, String mcastBindAddress,
Integer mcastPort, Integer memcachedPort, String memcachedProtocol,
String memcachedBindAddress, Integer redisPort, String redisBindAddress, String redisUsername,
String memcachedBindAddress,
Integer messageTimeToLive, String offHeapMemorySize, File gemfirePropertiesFile,
Boolean rebalance, File gemfireSecurityPropertiesFile, String serverBindAddress,
Integer serverPort, Integer socketBufferSize, String springXmlLocation,
Expand Down Expand Up @@ -286,14 +280,6 @@ ResultModel doStartServer(String memberName, Boolean assignBuckets, String bindA
ConfigurationProperties.MEMCACHED_PROTOCOL, memcachedProtocol);
StartMemberUtils.setPropertyIfNotNull(gemfireProperties,
ConfigurationProperties.MEMCACHED_BIND_ADDRESS, memcachedBindAddress);
StartMemberUtils.setPropertyIfNotNull(gemfireProperties,
ConfigurationProperties.GEODE_FOR_REDIS_PORT,
redisPort);
StartMemberUtils.setPropertyIfNotNull(gemfireProperties,
ConfigurationProperties.GEODE_FOR_REDIS_BIND_ADDRESS, redisBindAddress);
StartMemberUtils.setPropertyIfNotNull(gemfireProperties,
ConfigurationProperties.GEODE_FOR_REDIS_USERNAME,
redisUsername);
StartMemberUtils.setPropertyIfNotNull(gemfireProperties,
ConfigurationProperties.STATISTIC_ARCHIVE_FILE, statisticsArchivePathname);
StartMemberUtils.setPropertyIfNotNull(gemfireProperties,
Expand All @@ -309,17 +295,6 @@ ResultModel doStartServer(String memberName, Boolean assignBuckets, String bindA
StartMemberUtils.setPropertyIfNotNull(gemfireProperties,
ConfigurationProperties.HTTP_SERVICE_BIND_ADDRESS, httpServiceBindAddress);

// if geode-for-redis-port, geode-for-redis-bind-address, or
// geode-for-redis-username are specified in the command line, REDIS_ENABLED should be set
// to true
String stringRedisPort;
stringRedisPort = redisPort == null ? "" : redisPort.toString();

if (StringUtils.isNotBlank(stringRedisPort) || StringUtils.isNotBlank(redisUsername)
|| StringUtils.isNotBlank(redisBindAddress)) {
gemfireProperties.setProperty(ConfigurationProperties.GEODE_FOR_REDIS_ENABLED, "true");
}

// if username is specified in the command line, it will overwrite what's set in the
// properties file
if (StringUtils.isNotBlank(userName)) {
Expand Down
Expand Up @@ -228,10 +228,10 @@ public void optionWithMoreThanOneHyphen() {
@Test
public void optionWithOneHyphenAfterOneJOption() {
String cmd =
"start server --name=me3 --J=-Dgemfire.jmx-manager=true --geode-for-redis-port=8080";
"start server --name=me3 --J=-Dgemfire.jmx-manager=true --http-service-port=8080";
String formattedCmd = this.formatter.formatCommand(cmd);
String expected =
"start server --name=me3 --J=\"-Dgemfire.jmx-manager=true\" --geode-for-redis-port=8080";
"start server --name=me3 --J=\"-Dgemfire.jmx-manager=true\" --http-service-port=8080";
assertThat(formattedCmd).as(cmd).isEqualTo(expected);
}

Expand All @@ -247,10 +247,10 @@ public void optionWithMoreThanOneHyphenAfterOneJOption() {
@Test
public void optionWithOneHyphenAfterTwoJOptions() {
String cmd =
"start server --name=me3 --J=-Dgemfire.jmx-manager=true --J=-Dgemfire.jmx-manager-start=true --geode-for-redis-port=8080";
"start server --name=me3 --J=-Dgemfire.jmx-manager=true --J=-Dgemfire.jmx-manager-start=true --http-service-port=8080";
String formattedCmd = this.formatter.formatCommand(cmd);
String expected =
"start server --name=me3 --J=\"-Dgemfire.jmx-manager=true\" --J=\"-Dgemfire.jmx-manager-start=true\" --geode-for-redis-port=8080";
"start server --name=me3 --J=\"-Dgemfire.jmx-manager=true\" --J=\"-Dgemfire.jmx-manager-start=true\" --http-service-port=8080";
assertThat(formattedCmd).as(cmd).isEqualTo(expected);
}

Expand Down