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 2020/06/24 19:20:33 UTC
[GitHub] [geode] jchen21 commented on a change in pull request #5285: GEODE-8099: make those gfsh commands that updates cluster configurati…
jchen21 commented on a change in pull request #5285:
URL: https://github.com/apache/geode/pull/5285#discussion_r445095363
##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/cli/SingleGfshCommand.java
##########
@@ -39,7 +42,11 @@
* return value of your command method.
* @return a boolean indicating whether a change to the cluster configuration was persisted.
*/
- public boolean updateConfigForGroup(String group, CacheConfig config, Object configObject) {
- return false;
+ public abstract boolean updateConfigForGroup(String group, CacheConfig config,
Review comment:
This changes the public API, which could affect those gfsh commands that extend this class.
##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/remote/OnlineCommandProcessor.java
##########
@@ -53,14 +57,13 @@
private SecurityService securityService;
- private InternalCache cache;
-
public OnlineCommandProcessor() {}
@VisibleForTesting
- public OnlineCommandProcessor(Properties cacheProperties, SecurityService securityService,
- CommandExecutor commandExecutor, InternalCache cache) {
- this.gfshParser = new GfshParser(new CommandManager(cacheProperties, cache));
+ public OnlineCommandProcessor(GfshParser gfshParser,
Review comment:
This constructor is used only by tests. e.g. `MemberCommandServiceTest`. This also leads to adding a new constructor to a deprecated class `MemberCommandService`. And then changes to `MemberCommandServiceTest`. I wonder if it is necessary to make this change.
##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/cli/SingleGfshCommand.java
##########
@@ -39,7 +42,11 @@
* return value of your command method.
* @return a boolean indicating whether a change to the cluster configuration was persisted.
*/
- public boolean updateConfigForGroup(String group, CacheConfig config, Object configObject) {
- return false;
+ public abstract boolean updateConfigForGroup(String group, CacheConfig config,
+ Object configObject);
+
+ @Override
+ public boolean affectsClusterConfiguration() {
Review comment:
This changes the public API, which could affect those gfsh commands that extend this class.
----------------------------------------------------------------
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