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:
us...@infra.apache.org
