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 2 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
11 changes: 6 additions & 5 deletions geode-docs/tools_modules/geode_for_redis.html.md.erb
Expand Up @@ -35,13 +35,14 @@ 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> \
--J=-Dgemfire.geode-for-redis-password=<geodeForRedisPassword>
Copy link
Contributor

Choose a reason for hiding this comment

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

There doesn't seem to be a geode-for-redis-password property in the codebase, so references to this should probably be removed from docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, we switched to using a SecurityManager!

```

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 option `geode-for-redis-enabled`, is set to `true`, a <%=vars.product_name%> server with
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is wrong to call "geode-for-redis-enabled" an "option". It used to be a gfsh option but is no longer. Consider replacing "option" with "gemfire property".

<%=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,8 @@ 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-bind-address", "localhost",
"--J=-Dgemfire.geode-for-redis-port", String.valueOf(port));
GfshExecution execution;

try (ServerSocket interferingSocket = new ServerSocket()) {
Expand All @@ -66,7 +66,7 @@ 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-port", String.valueOf(port));
GfshExecution execution;

try (ServerSocket interferingSocket = new ServerSocket()) {
Expand All @@ -86,7 +86,7 @@ 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-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