[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6155: GEODE-9037: Do not expose unsupported commands in Geode compatibility with Redis

[GitBox <gi...@apache.org>]

jdeppe-pivotal commented on a change in pull request #6155:
URL: https://github.com/apache/geode/pull/6155#discussion_r597762157



##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/GeodeRedisServer.java
##########
@@ -46,34 +45,25 @@
  * sent back to the client. The default connection port is 6379 but that can be altered when run
  * through gfsh or started through the provided static main class.
  */
-
 public class GeodeRedisServer {
+
   /**
    * The default Redis port as specified by their protocol, {@code DEFAULT_REDIS_SERVER_PORT}
    */
   public static final int DEFAULT_REDIS_SERVER_PORT = 6379;
-
-  public static final String ENABLE_REDIS_UNSUPPORTED_COMMANDS_PARAM =
-      "enable-redis-unsupported-commands";
+  public static final String ENABLE_UNSUPPORTED_COMMANDS_SYSTEM_PROPERTY =

Review comment:
       It's somewhat conventional (maybe just in this codebase) to call system properties `..._PARAM`. Consider keeping things more idiomatic.

##########
File path: geode-redis/src/commonTest/java/org/apache/geode/redis/GeodeRedisServerRule.java
##########
@@ -40,15 +41,21 @@ public GeodeRedisServerRule() {
     cacheFactory.set(LOCATORS, "");
   }
 
+  public GeodeRedisServerRule(boolean enableUnsupportedCommands) {
+    cacheFactory = new CacheFactory();
+    cacheFactory.set(LOG_LEVEL, "warn");
+    cacheFactory.set(MCAST_PORT, "0");
+    cacheFactory.set(LOCATORS, "");

Review comment:
       These lines should be replaced by a call to the default constructor: `this()`

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/GeodeRedisServer.java
##########
@@ -142,16 +124,14 @@ protected Long getDataStoreBytesInUseForDataRegion() {
     return dataStoreBytesInUse;
   }
 
-  /**
-   * Precedence of the internal property overrides the global system property.
-   */
-  public boolean allowUnsupportedCommands() {
-    return (boolean) regionProvider.getConfigRegion()
-        .getOrDefault(ENABLE_REDIS_UNSUPPORTED_COMMANDS_PARAM, ENABLE_REDIS_UNSUPPORTED_COMMANDS);
+  @VisibleForTesting
+  public void setAllowUnsupportedCommandsSystemProperty(boolean allowUnsupportedCommands) {

Review comment:
       This is leaking the implementation into the method name - it's fine to have it as just `setAllowUnsupportedCommands`.

##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/GeodeRedisServer.java
##########
@@ -142,16 +124,14 @@ protected Long getDataStoreBytesInUseForDataRegion() {
     return dataStoreBytesInUse;
   }
 
-  /**
-   * Precedence of the internal property overrides the global system property.
-   */
-  public boolean allowUnsupportedCommands() {
-    return (boolean) regionProvider.getConfigRegion()
-        .getOrDefault(ENABLE_REDIS_UNSUPPORTED_COMMANDS_PARAM, ENABLE_REDIS_UNSUPPORTED_COMMANDS);
+  @VisibleForTesting
+  public void setAllowUnsupportedCommandsSystemProperty(boolean allowUnsupportedCommands) {
+    System.setProperty(ENABLE_UNSUPPORTED_COMMANDS_SYSTEM_PROPERTY,
+        String.valueOf(allowUnsupportedCommands));
   }
 
-  private void logUnsupportedCommandWarning() {
-    logger.warn("Unsupported commands enabled. Unsupported commands have not been fully tested.");
+  public boolean allowUnsupportedCommands() {
+    return Boolean.valueOf(System.getProperty("enable-unsupported-commands", "false"));

Review comment:
       I would suggest just having an `allowUnsupportedCommands` field which is initialized from the system property. Then use that field here. For testing situations, the `setUnsupportedCommands` method can be used to enable them.
   As an aside, retrieving a system property in a hot path (which this is on) would be a significant performance hit.




-- 
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