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 2021/03/18 21:41:28 UTC
[GitHub] [geode] jhutchison opened a new pull request #6155: GEODE-9037: Do not expose unsupported commands in Geode compatibility with Redis
jhutchison opened a new pull request #6155:
URL: https://github.com/apache/geode/pull/6155
-update handling of unsupported commands
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
--
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] jhutchison commented on a change in pull request #6155: GEODE-9037: Do not expose unsupported commands in Geode compatibility with Redis
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #6155:
URL: https://github.com/apache/geode/pull/6155#discussion_r597865800
##########
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:
yeah, I think it's an expression of my uncertainty about how we had it implemented. it's just setting the field now (which I feel better about).
--
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] jdeppe-pivotal commented on a change in pull request #6155: GEODE-9037: Do not expose unsupported commands in Geode compatibility with Redis
Posted by 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
[GitHub] [geode] jhutchison commented on a change in pull request #6155: GEODE-9037: Do not expose unsupported commands in Geode compatibility with Redis
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #6155:
URL: https://github.com/apache/geode/pull/6155#discussion_r597816960
##########
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:
ok- will change back
--
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] ringles merged pull request #6155: GEODE-9037: Do not expose unsupported commands in Geode compatibility with Redis
Posted by GitBox <gi...@apache.org>.
ringles merged pull request #6155:
URL: https://github.com/apache/geode/pull/6155
--
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] jhutchison commented on a change in pull request #6155: GEODE-9037: Do not expose unsupported commands in Geode compatibility with Redis
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #6155:
URL: https://github.com/apache/geode/pull/6155#discussion_r597865096
##########
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:
thanks. changed
--
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] jhutchison commented on a change in pull request #6155: GEODE-9037: Do not expose unsupported commands in Geode compatibility with Redis
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #6155:
URL: https://github.com/apache/geode/pull/6155#discussion_r597817990
##########
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:
thx. will change
--
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