GEODE-9869: Remove geode-for-redis properties from gfsh options #7205
Conversation
ringles
changed the title
ringles
requested review from
dickcav,
DonalEvans,
dschneider-pivotal,
jdeppe-pivotal,
jinmeiliao,
kirklund,
mhansonp,
nonbinaryprogrammer,
onichols-pivotal,
rhoughton-pivot,
smgoller and
upthewaterspout
as code owners
| --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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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".
| ``` | ||
|
|
||
| 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. |
There was a problem hiding this comment.
option once again
There was a problem hiding this comment.
Yeah, this needs a fix. Must be explicitly enabled now.
There was a problem hiding this comment.
Except we're getting rid of the Redis README.md now, I think.
| @@ -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> | |||
There was a problem hiding this comment.
why remove this one but not the bind address and username?
| --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> |
There was a problem hiding this comment.
do these have to be gemfire. or could they be geode.? just since we generally avoid referencing gemfire in geode
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, that was discussed and 'gemfire.' won, mostly on 'principle of least surprise' grounds.
|
Addressed! |
Remove the GFSH options related to geode-for-redis and update docs, examples, and tests to set such options via Java properties.