gesterzhou commented on a change in pull request #5348:
URL: https://github.com/apache/geode/pull/5348#discussion_r482630830



##########
File path: 
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StartGatewaySenderCommand.java
##########
@@ -77,6 +82,9 @@ public ResultModel startGatewaySender(@CliOption(key = 
CliStrings.START_GATEWAYS
       return ResultModel.createError(CliStrings.NO_MEMBERS_FOUND_MESSAGE);
     }
 
+    // TODO handle error situations when invoking sendAllStartingMessage
+    setNotAllGatewaySenderInstancesStopped(dsMembers, id);

Review comment:
       This method and the latter setAllGatewaySenderInstancesStopped() should 
be merged into one. And the name is confusing. 
   
   BTW, since you modified gfsh commands, you need to add some dunit tests for 
gfsh commands. I did not see them yet. 
   
   Is it ok to change gfsh command this way (i.e. option-1), I feel it's OK. 
But let Jinmei to confirm. 




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


Reply via email to