[GitHub] [geode] albertogpz opened a new pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

albertogpz opened a new pull request #7108:
URL: https://github.com/apache/geode/pull/7108


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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] jinmeiliao commented on a change in pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

jinmeiliao commented on a change in pull request #7108:
URL: https://github.com/apache/geode/pull/7108#discussion_r773446783



##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommand.java
##########
@@ -36,7 +44,30 @@
 import org.apache.geode.security.ResourcePermission;
 
 public class StopGatewaySenderCommand extends GfshCommand {
+  private final ExecutorService executorService;
+  private SystemManagementService managementService;
+  private BiFunction<String[], String[], Set<DistributedMember>> findMembers;
+  private final Supplier<StopGatewaySenderOnMember> stopperOnMemberFactory;
+
+  @SuppressWarnings("unused") // invoked by spring shell
+  public StopGatewaySenderCommand() {
+    this(newCachedThreadPool("Stop Sender Command Thread ", true),
+        StopGatewaySenderOnMemberWithBeanImpl::new, null, null);

Review comment:
       "Spying an object you are testing has been communicated to me as bad practice, but these warnings come with little to no explanation backing it up. I'm sure, like anything else, it can definitely be abused.". 
   To me, in this case, spying on these methods to pass in a mock object is definitely better than passing null objects in the product calling code.




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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

jinmeiliao commented on pull request #7108:
URL: https://github.com/apache/geode/pull/7108#issuecomment-998378921


   Normally the command classes are stateless, we don't usually have member variables for command classes. I guess I don't quite understand why you would want to have a delegate class to do all the work instead, why can't you just do all the work inside the command class, like every other command is doing? I hate to introduce another pattern for commands for no obvious reasons.


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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] jinmeiliao commented on a change in pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

jinmeiliao commented on a change in pull request #7108:
URL: https://github.com/apache/geode/pull/7108#discussion_r752675185



##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMemberFactory.java
##########
@@ -0,0 +1,20 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+interface StopGatewaySenderOnMemberFactory {

Review comment:
       Is it really necessary to add those interfaces? Personally, I don't like adding more interfaces/implementations only for testing purposes. I thought change set was good, just missing a few more tests to cover all branches.  You can easily test your previous command changes by mocking up some dependencies.




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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] albertogpz commented on a change in pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

albertogpz commented on a change in pull request #7108:
URL: https://github.com/apache/geode/pull/7108#discussion_r753429827



##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMemberFactory.java
##########
@@ -0,0 +1,20 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+interface StopGatewaySenderOnMemberFactory {

Review comment:
       @kirklund I went for the `Supplier` option.
   @jinmeiliao I have removed some interfaces (by using the `Supplier` one as Kirk suggested) and I have moved the ones remaining to the class using them.




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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] albertogpz commented on a change in pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

albertogpz commented on a change in pull request #7108:
URL: https://github.com/apache/geode/pull/7108#discussion_r753436317



##########
File path: geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommandTest.java
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.management.internal.SystemManagementService;
+import org.apache.geode.management.internal.cli.result.model.ResultModel;
+import org.apache.geode.management.internal.functions.CliFunctionResult;
+import org.apache.geode.test.junit.rules.GfshParserRule;
+
+public class StopGatewaySenderCommandTest {
+  @ClassRule
+  public static GfshParserRule gfsh = new GfshParserRule();
+
+  private StopGatewaySenderCommand command;
+
+  @Before
+  public void before() {
+    command = spy(StopGatewaySenderCommand.class);
+    List<CliFunctionResult> functionResults = new ArrayList<>();
+    doReturn(functionResults).when(command).executeAndGetFunctionResult(any(), any(), any());

Review comment:
       @kirklund Thanks a lot for the suggestion. I finally have not needed to modify `GfshCommand`.
   Nevertheless, I had to inject the command delegate and the `findMembers` test behavior using the constructor.
   The outcome has not been very clean. If you have any suggestions to make it cleaner, please let me know.
   
   Given that `findMembers` is not a static method, I could not pass it when calling the last constructor. Instead, I had to set it after I called it.
   On the other hand, the `StopGatewaySenderCommandDelegateParallelImpl` constructor needed the a `ManagementSystem` which is obtained by calling the instance `getManagementSystem()` method. As a result, I had to set it to null in the Spring shell constructor and later set it to the right value if it was found null.




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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] albertogpz commented on a change in pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

albertogpz commented on a change in pull request #7108:
URL: https://github.com/apache/geode/pull/7108#discussion_r753436596



##########
File path: geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMemberWithBeanImplTest.java
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.ArrayList;
+
+import junitparams.Parameters;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.distributed.DistributedSystem;
+import org.apache.geode.management.GatewaySenderMXBean;
+import org.apache.geode.management.internal.SystemManagementService;
+import org.apache.geode.test.junit.runners.GeodeParamsRunner;
+
+@RunWith(GeodeParamsRunner.class)
+public class StopGatewaySenderOnMemberWithBeanImplTest {
+  String senderId = "sender1";
+  String memberId = "member1";
+  String remoteMemberId = "remoteMember1";
+  Cache cache;
+  SystemManagementService managementService;
+  DistributedMember distributedMember;
+  DistributedMember remoteDistributedMember;
+  DistributedSystem distributedSystem;
+  GatewaySenderMXBean bean;
+
+  @Before
+  public void setUp() {
+    cache = mock(Cache.class);
+    bean = mock(GatewaySenderMXBean.class);
+    managementService = mock(SystemManagementService.class);
+    distributedMember = mock(DistributedMember.class);
+    remoteDistributedMember = mock(DistributedMember.class);
+    when(distributedMember.getId()).thenReturn(memberId);
+    when(remoteDistributedMember.getId()).thenReturn(remoteMemberId);
+    distributedSystem = mock(DistributedSystem.class);
+    doReturn(distributedSystem).when(cache).getDistributedSystem();
+  }
+
+  @Test
+  @Parameters({"true", "false"})
+  public void executeStopGatewaySenderOnMemberNotRunningReturnsNotRunningError(
+      boolean isLocalMember) {
+    StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl();
+    setUpMocks(isLocalMember, false);
+    doReturn(false).when(bean).isRunning();
+    ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache,
+        managementService, distributedMember);
+    assertThat(result).containsExactly(memberId, "Error",
+        "GatewaySender sender1 is not running on member " + memberId + ".");
+  }
+
+  @Test
+  @Parameters({"true", "false"})
+  public void executeStopGatewaySenderOnMemberNotAvailableReturnsNotAvailableError(
+      boolean isLocalMember) {
+    StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl();
+    setUpMocks(isLocalMember, true);
+    ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache,
+        managementService, distributedMember);
+    assertThat(result).containsExactly(memberId, "Error",
+        "GatewaySender sender1 is not available on member " + memberId);
+  }
+
+  @Test
+  @Parameters({"true", "false"})
+  public void executeStopGatewaySenderOnMemberRunningReturnsOk(boolean isLocalMember) {
+    StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl();
+    setUpMocks(isLocalMember, false);
+    doReturn(true).when(bean).isRunning();
+    ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache,
+        managementService, distributedMember);
+    assertThat(result).containsExactly(memberId, "OK",
+        "GatewaySender sender1 is stopped on member " + memberId);
+  }
+
+  private void setUpMocks(boolean isLocalMember, boolean beanMustBeNull) {

Review comment:
       Great suggestion!




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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] jinmeiliao commented on a change in pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

jinmeiliao commented on a change in pull request #7108:
URL: https://github.com/apache/geode/pull/7108#discussion_r773352708



##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommand.java
##########
@@ -36,7 +44,30 @@
 import org.apache.geode.security.ResourcePermission;
 
 public class StopGatewaySenderCommand extends GfshCommand {
+  private final ExecutorService executorService;
+  private SystemManagementService managementService;
+  private BiFunction<String[], String[], Set<DistributedMember>> findMembers;
+  private final Supplier<StopGatewaySenderOnMember> stopperOnMemberFactory;
+
+  @SuppressWarnings("unused") // invoked by spring shell
+  public StopGatewaySenderCommand() {
+    this(newCachedThreadPool("Stop Sender Command Thread ", true),
+        StopGatewaySenderOnMemberWithBeanImpl::new, null, null);

Review comment:
       `GfshCommand` has `findMember()` and `getManagementService()` you can stubbed out in your tests, I don't think you need the last two params in your "test constructor". Also, for the first two parameters, one you are passing the object itself, one is a supplier, I suggest you make it consistent, I would pass in both in object form (change the supplier to be an instance of `StopGatewaySenderOnMember`. 

##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommand.java
##########
@@ -36,7 +44,30 @@
 import org.apache.geode.security.ResourcePermission;
 
 public class StopGatewaySenderCommand extends GfshCommand {
+  private final ExecutorService executorService;
+  private SystemManagementService managementService;
+  private BiFunction<String[], String[], Set<DistributedMember>> findMembers;
+  private final Supplier<StopGatewaySenderOnMember> stopperOnMemberFactory;
+
+  @SuppressWarnings("unused") // invoked by spring shell
+  public StopGatewaySenderCommand() {
+    this(newCachedThreadPool("Stop Sender Command Thread ", true),
+        StopGatewaySenderOnMemberWithBeanImpl::new, null, null);
+    findMembers = this::findMembers;
+  }
+
+  StopGatewaySenderCommand(

Review comment:
       annotate this as `VisisbleForTesting`




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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] albertogpz commented on a change in pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

albertogpz commented on a change in pull request #7108:
URL: https://github.com/apache/geode/pull/7108#discussion_r773750399



##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommand.java
##########
@@ -36,7 +44,30 @@
 import org.apache.geode.security.ResourcePermission;
 
 public class StopGatewaySenderCommand extends GfshCommand {
+  private final ExecutorService executorService;
+  private SystemManagementService managementService;
+  private BiFunction<String[], String[], Set<DistributedMember>> findMembers;
+  private final Supplier<StopGatewaySenderOnMember> stopperOnMemberFactory;
+
+  @SuppressWarnings("unused") // invoked by spring shell
+  public StopGatewaySenderCommand() {
+    this(newCachedThreadPool("Stop Sender Command Thread ", true),
+        StopGatewaySenderOnMemberWithBeanImpl::new, null, null);

Review comment:
       This is probably one of those cases in which using a spy is justified. I cannot give you strong arguments in this case that make the previous implementation without spy better, so I will follow your proposal.




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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] albertogpz commented on a change in pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

albertogpz commented on a change in pull request #7108:
URL: https://github.com/apache/geode/pull/7108#discussion_r753431300



##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommandDelegateParallelImpl.java
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+
+import org.apache.geode.annotations.VisibleForTesting;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.logging.internal.executors.LoggingExecutors;
+import org.apache.geode.management.internal.SystemManagementService;
+import org.apache.geode.management.internal.cli.result.model.ResultModel;
+import org.apache.geode.management.internal.cli.result.model.TabularResultModel;
+import org.apache.geode.management.internal.i18n.CliStrings;
+
+class StopGatewaySenderCommandDelegateParallelImpl
+    implements StopGatewaySenderCommandDelegate {
+
+  private final ExecutorService executorService;
+  private final SystemManagementService managementService;
+  private final StopGatewaySenderOnMemberFactory stopperOnMemberFactory;
+
+  @VisibleForTesting
+  public StopGatewaySenderCommandDelegateParallelImpl(ExecutorService executorService,
+      SystemManagementService managementService,
+      StopGatewaySenderOnMemberFactory stopperOnMemberFactory) {
+    this.executorService = executorService;
+    this.managementService = managementService;
+    this.stopperOnMemberFactory = stopperOnMemberFactory;
+  }
+
+  public StopGatewaySenderCommandDelegateParallelImpl(SystemManagementService managementService) {
+    executorService = LoggingExecutors.newCachedThreadPool("Stop Sender Command Thread ", true);;
+    stopperOnMemberFactory = new StopGatewaySenderOnMemberFactoryWithBeanImpl();
+    this.managementService = managementService;
+  }
+
+  public ResultModel executeStopGatewaySender(String id, Cache cache,
+      Set<DistributedMember> dsMembers) {
+    List<Callable<List<String>>> callables = new ArrayList<>();
+    for (final DistributedMember member : dsMembers) {
+      callables.add(() -> stopperOnMemberFactory.create().executeStopGatewaySenderOnMember(id,
+          cache, managementService, member));
+    }
+
+    List<Future<List<String>>> futures;
+    try {
+      futures = executorService.invokeAll(callables);
+    } catch (InterruptedException ite) {
+      Thread.currentThread().interrupt();
+      return ResultModel.createError(
+          CliStrings.format(CliStrings.GATEWAY_SENDER_STOP_0_COULD_NOT_BE_INVOKED_DUE_TO_1, id,
+              ite.getMessage()));
+    } finally {
+      executorService.shutdown();
+    }
+
+    return buildResultModelFromMembersResponses(id, dsMembers, futures);
+  }
+
+  private ResultModel buildResultModelFromMembersResponses(String id,
+      Set<DistributedMember> dsMembers, List<Future<List<String>>> futures) {
+    ResultModel resultModel = new ResultModel();
+    TabularResultModel resultData = resultModel.addTable(CliStrings.STOP_GATEWAYSENDER);
+    Iterator<DistributedMember> memberIterator = dsMembers.iterator();
+    for (Future<List<String>> future : futures) {
+      DistributedMember member = memberIterator.next();

Review comment:
       I have created a `List` from the `dsMembers` `Set` and I have passed it to the method so that we can be sure that the order between the `dsMembers` and the `futures` is the same.




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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] albertogpz commented on pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

albertogpz commented on pull request #7108:
URL: https://github.com/apache/geode/pull/7108#issuecomment-1016303958


   @kirklund Do you agree with the changes done?


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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] albertogpz edited a comment on pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

albertogpz edited a comment on pull request #7108:
URL: https://github.com/apache/geode/pull/7108#issuecomment-970408879


   > same comments as @mhansonp . Would need some unit tests for the refactor.
   
   @mhansonp and @jinmeiliao I have added unit test cases. Anyhow, I was not very fond of adding them because in order to add them, I have had to open up the implementation by adding a new public method (getExecutorService()) to be able to override it in the spy I am using in the test.
   I would have preferred to add a new constructor for testing to the class in which I could inject the dependencies for testing but it was not possible with gfsh command classes (at least, I did not find a way to do it).


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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] albertogpz commented on pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

albertogpz commented on pull request #7108:
URL: https://github.com/apache/geode/pull/7108#issuecomment-998973289


   > Normally the command classes are stateless, we don't usually have member variables for command classes. I guess I don't quite understand why you would want to have a delegate class to do all the work instead, why can't you just do all the work inside the command class, like every other command is doing? I hate to introduce another pattern for commands for no obvious reasons.
   
   @jinmeiliao I have removed the delegate class. I agree with you, it is not needed. Regarding member variables, the ones added correspond to the dependencies that can be passed in the constructor to be able to unit test the 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.

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] albertogpz commented on pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

albertogpz commented on pull request #7108:
URL: https://github.com/apache/geode/pull/7108#issuecomment-998973968


   > I don't see any unit tests of this functionality. The existing serial tests would not be sufficient.
   > 
   > Tests that show the threads get executed and cleaned up properly would be good regardless of the actual outcomes of the calls (failure, success).
   
   @mhansonp Unit tests added.


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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] albertogpz commented on pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

albertogpz commented on pull request #7108:
URL: https://github.com/apache/geode/pull/7108#issuecomment-970408879


   > same comments as @mhansonp . Would need some unit tests for the refactor.
   
   @mhansonp and @jinmeiliao I have added unit test cases. Anyhow, I was not very fond of adding them because in order to add them, I have open up the implementation by adding a new public method (getExecutorService()) to be able to override it in the spy I am using in the test.
   I would have preferred to add a new constructor for testing to the class in which I could inject the dependencies for testing but it was not possible with gfsh command classes (at least, I did not find a way to do it).


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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] albertogpz merged pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

albertogpz merged pull request #7108:
URL: https://github.com/apache/geode/pull/7108


   


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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] jinmeiliao commented on a change in pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

jinmeiliao commented on a change in pull request #7108:
URL: https://github.com/apache/geode/pull/7108#discussion_r772619057



##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommand.java
##########
@@ -16,27 +16,40 @@
 package org.apache.geode.management.internal.cli.commands;
 
 import java.util.Set;
-
-import javax.management.ObjectName;
+import java.util.function.BiFunction;
+import java.util.function.Supplier;
 
 import org.springframework.shell.core.annotation.CliCommand;
 import org.springframework.shell.core.annotation.CliOption;
 
+import org.apache.geode.cache.Cache;
 import org.apache.geode.distributed.DistributedMember;
-import org.apache.geode.internal.cache.InternalCache;
-import org.apache.geode.management.GatewaySenderMXBean;
 import org.apache.geode.management.cli.CliMetaData;
 import org.apache.geode.management.cli.ConverterHint;
 import org.apache.geode.management.cli.GfshCommand;
-import org.apache.geode.management.internal.SystemManagementService;
 import org.apache.geode.management.internal.cli.result.model.ResultModel;
-import org.apache.geode.management.internal.cli.result.model.TabularResultModel;
 import org.apache.geode.management.internal.i18n.CliStrings;
 import org.apache.geode.management.internal.security.ResourceOperation;
 import org.apache.geode.security.ResourcePermission;
 
 public class StopGatewaySenderCommand extends GfshCommand {
+  private Supplier<StopGatewaySenderCommandDelegate> commandDelegateSupplier;
+  private BiFunction<String[], String[], Set<DistributedMember>> findMembers;
+
+  @SuppressWarnings("unused") // invoked by spring shell
+  public StopGatewaySenderCommand() {
+    this(null, null);
+    findMembers = this::findMembers;
+    commandDelegateSupplier = this::getCommandDelegate;
+  }
+
+  StopGatewaySenderCommand(Supplier<StopGatewaySenderCommandDelegate> commandDelegateSupplier,

Review comment:
       is this test only?




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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] kirklund commented on a change in pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

kirklund commented on a change in pull request #7108:
URL: https://github.com/apache/geode/pull/7108#discussion_r752689490



##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMemberFactory.java
##########
@@ -0,0 +1,20 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+interface StopGatewaySenderOnMemberFactory {

Review comment:
       `@FunctionalInterface`
   
   Since this interface is so simple (no arguments to `create`), you could simply use `java.util.function.Supplier` instead.
   
   If you do keep the custom interface, then I recommend making it an inner-class within the class that uses it. If it's going to be used by more than one class, then it's good to have it be its own top-level 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.

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] jinmeiliao commented on a change in pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

jinmeiliao commented on a change in pull request #7108:
URL: https://github.com/apache/geode/pull/7108#discussion_r751543759



##########
File path: geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommandTest.java
##########
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.management.ManagementService;
+import org.apache.geode.management.internal.SystemManagementService;
+import org.apache.geode.management.internal.functions.CliFunctionResult;
+import org.apache.geode.test.junit.rules.GfshParserRule;
+
+public class StopGatewaySenderCommandTest {

Review comment:
       Nice test. Would be nice to add more tests to cover all branches. like `executorService.invokeAll` throws an exception, and mock up the list of future results to make sure it got displayed correctly ..




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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] kirklund commented on a change in pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

kirklund commented on a change in pull request #7108:
URL: https://github.com/apache/geode/pull/7108#discussion_r752688419



##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMember.java
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import java.util.ArrayList;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.management.internal.SystemManagementService;
+
+interface StopGatewaySenderOnMember {
+  ArrayList<String> executeStopGatewaySenderOnMember(String id, Cache cache,

Review comment:
       Always declare with Interfaces instead of implementations:
   ```
   List<String> executeStopGatewaySenderOnMember(...);
   ```

##########
File path: geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommandTest.java
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.management.internal.SystemManagementService;
+import org.apache.geode.management.internal.cli.result.model.ResultModel;
+import org.apache.geode.management.internal.functions.CliFunctionResult;
+import org.apache.geode.test.junit.rules.GfshParserRule;
+
+public class StopGatewaySenderCommandTest {
+  @ClassRule
+  public static GfshParserRule gfsh = new GfshParserRule();
+
+  private StopGatewaySenderCommand command;
+
+  @Before
+  public void before() {
+    command = spy(StopGatewaySenderCommand.class);
+    List<CliFunctionResult> functionResults = new ArrayList<>();
+    doReturn(functionResults).when(command).executeAndGetFunctionResult(any(), any(), any());

Review comment:
       Anytime you override an instance method on the component being tested, you're either not testing some part of the component or it suggests that the method represents a responsibility that should not be on this component.
   
   You can avoid using `spy` and overriding the instance method by using a field of type `java.util.function.Function`, or a custom interface, that is injected into the constructor(s).
   
   1. Add cleanResults field of type `java.util.function.Function` or custom inner-interface
   ```
   private final java.util.function.Function<List<?>, List<CliFunctionResult>> cleanResults;
   ```
   2. Add constructors to `GfshCommand`
   ```
     public GfshCommand() {
       this(CliFunctionResult::cleanResults);
     }
   
     protected GfshCommand(java.util.function.Function<List<?>, List<CliFunctionResult>> cleanResults) {
       this.cleanResults = cleanResults;
     }
   ```
   3. Add constructors to `StopGatewaySenderCommand`
   ```
     @SuppressWarnings("unused") // invoked by spring shell
     public StopGatewaySenderCommand() {
       this(CliFunctionResult::cleanResults);
     }
   
     StopGatewaySenderCommand(Function<List<?>, List<CliFunctionResult>> cleanResults) {
       super(cleanResults);
     }
   ```
   4. Change `GfshCommand.executeAndGetFunctionResult` to use the field `cleanResults`
   ```
     public List<CliFunctionResult> executeAndGetFunctionResult(Function<?> function, Object args,
         Set<DistributedMember> targetMembers) {
       ResultCollector<?, ?> rc = executeFunction(function, args, targetMembers);
       return cleanResults.apply((List<?>) rc.getResult());
     }
   ```
   5. Pass in simple mock when constructing `StopGatewaySenderCommand` in test
   ```
   Function<List<?>, List<CliFunctionResult>> cleanResults = mock(Function.class);
   command = new StopGatewaySenderCommand(cleanResults);
   ```
   If you need to observe `cleanResults` to validate interaction with that dependency, you simply move it from a local variable to a field:
   ```
   private Function<List<?>, List<CliFunctionResult>> cleanResults;
   // ...
   cleanResults = mock(Function.class);
   command = new StopGatewaySenderCommand(cleanResults);
   ```
   Then you can perform any Mockito validation calls against `cleanResults` that you need to.

##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommandDelegate.java
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import java.util.Set;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.management.internal.cli.result.model.ResultModel;
+
+interface StopGatewaySenderCommandDelegate {

Review comment:
       Remember to add `@FunctionalInterface` to any interface that will always define just one abstract method.

##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMemberWithBeanImpl.java
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import java.util.ArrayList;
+
+import javax.management.ObjectName;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.management.GatewaySenderMXBean;
+import org.apache.geode.management.internal.SystemManagementService;
+import org.apache.geode.management.internal.i18n.CliStrings;
+
+class StopGatewaySenderOnMemberWithBeanImpl implements StopGatewaySenderOnMember {
+
+  public ArrayList<String> executeStopGatewaySenderOnMember(String id, Cache cache,

Review comment:
       Return `List<String>` instead of `ArrayList<String>`

##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommandDelegateParallelImpl.java
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+
+import org.apache.geode.annotations.VisibleForTesting;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.logging.internal.executors.LoggingExecutors;
+import org.apache.geode.management.internal.SystemManagementService;
+import org.apache.geode.management.internal.cli.result.model.ResultModel;
+import org.apache.geode.management.internal.cli.result.model.TabularResultModel;
+import org.apache.geode.management.internal.i18n.CliStrings;
+
+class StopGatewaySenderCommandDelegateParallelImpl
+    implements StopGatewaySenderCommandDelegate {
+
+  private final ExecutorService executorService;
+  private final SystemManagementService managementService;
+  private final StopGatewaySenderOnMemberFactory stopperOnMemberFactory;
+
+  @VisibleForTesting
+  public StopGatewaySenderCommandDelegateParallelImpl(ExecutorService executorService,
+      SystemManagementService managementService,
+      StopGatewaySenderOnMemberFactory stopperOnMemberFactory) {
+    this.executorService = executorService;
+    this.managementService = managementService;
+    this.stopperOnMemberFactory = stopperOnMemberFactory;
+  }
+
+  public StopGatewaySenderCommandDelegateParallelImpl(SystemManagementService managementService) {
+    executorService = LoggingExecutors.newCachedThreadPool("Stop Sender Command Thread ", true);;
+    stopperOnMemberFactory = new StopGatewaySenderOnMemberFactoryWithBeanImpl();
+    this.managementService = managementService;
+  }
+
+  public ResultModel executeStopGatewaySender(String id, Cache cache,
+      Set<DistributedMember> dsMembers) {
+    List<Callable<List<String>>> callables = new ArrayList<>();
+    for (final DistributedMember member : dsMembers) {
+      callables.add(() -> stopperOnMemberFactory.create().executeStopGatewaySenderOnMember(id,
+          cache, managementService, member));
+    }
+
+    List<Future<List<String>>> futures;
+    try {
+      futures = executorService.invokeAll(callables);
+    } catch (InterruptedException ite) {
+      Thread.currentThread().interrupt();
+      return ResultModel.createError(
+          CliStrings.format(CliStrings.GATEWAY_SENDER_STOP_0_COULD_NOT_BE_INVOKED_DUE_TO_1, id,
+              ite.getMessage()));
+    } finally {
+      executorService.shutdown();
+    }
+
+    return buildResultModelFromMembersResponses(id, dsMembers, futures);
+  }
+
+  private ResultModel buildResultModelFromMembersResponses(String id,
+      Set<DistributedMember> dsMembers, List<Future<List<String>>> futures) {
+    ResultModel resultModel = new ResultModel();
+    TabularResultModel resultData = resultModel.addTable(CliStrings.STOP_GATEWAYSENDER);
+    Iterator<DistributedMember> memberIterator = dsMembers.iterator();
+    for (Future<List<String>> future : futures) {
+      DistributedMember member = memberIterator.next();

Review comment:
       Iterator doesn't guarantee order unless the declared collection type guarantees ordering of `iterator()`.
   
   It's also not clear what the relation ship is between dsMembers or futures in this method. I can't tell if the code is correct. I think you should make changes here that don't depend on ordering or some implied relationship between the two collections.

##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMember.java
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import java.util.ArrayList;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.management.internal.SystemManagementService;
+
+interface StopGatewaySenderOnMember {

Review comment:
       `@FunctionalInterface`

##########
File path: geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMemberWithBeanImplTest.java
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.ArrayList;
+
+import junitparams.Parameters;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.distributed.DistributedSystem;
+import org.apache.geode.management.GatewaySenderMXBean;
+import org.apache.geode.management.internal.SystemManagementService;
+import org.apache.geode.test.junit.runners.GeodeParamsRunner;
+
+@RunWith(GeodeParamsRunner.class)
+public class StopGatewaySenderOnMemberWithBeanImplTest {
+  String senderId = "sender1";
+  String memberId = "member1";
+  String remoteMemberId = "remoteMember1";
+  Cache cache;
+  SystemManagementService managementService;
+  DistributedMember distributedMember;
+  DistributedMember remoteDistributedMember;
+  DistributedSystem distributedSystem;
+  GatewaySenderMXBean bean;
+
+  @Before
+  public void setUp() {
+    cache = mock(Cache.class);
+    bean = mock(GatewaySenderMXBean.class);
+    managementService = mock(SystemManagementService.class);
+    distributedMember = mock(DistributedMember.class);
+    remoteDistributedMember = mock(DistributedMember.class);
+    when(distributedMember.getId()).thenReturn(memberId);
+    when(remoteDistributedMember.getId()).thenReturn(remoteMemberId);
+    distributedSystem = mock(DistributedSystem.class);
+    doReturn(distributedSystem).when(cache).getDistributedSystem();
+  }
+
+  @Test
+  @Parameters({"true", "false"})
+  public void executeStopGatewaySenderOnMemberNotRunningReturnsNotRunningError(
+      boolean isLocalMember) {
+    StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl();
+    setUpMocks(isLocalMember, false);
+    doReturn(false).when(bean).isRunning();
+    ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache,

Review comment:
       `List<String>`

##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommandDelegateParallelImpl.java
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+
+import org.apache.geode.annotations.VisibleForTesting;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.logging.internal.executors.LoggingExecutors;
+import org.apache.geode.management.internal.SystemManagementService;
+import org.apache.geode.management.internal.cli.result.model.ResultModel;
+import org.apache.geode.management.internal.cli.result.model.TabularResultModel;
+import org.apache.geode.management.internal.i18n.CliStrings;
+
+class StopGatewaySenderCommandDelegateParallelImpl
+    implements StopGatewaySenderCommandDelegate {
+
+  private final ExecutorService executorService;
+  private final SystemManagementService managementService;
+  private final StopGatewaySenderOnMemberFactory stopperOnMemberFactory;
+
+  @VisibleForTesting
+  public StopGatewaySenderCommandDelegateParallelImpl(ExecutorService executorService,
+      SystemManagementService managementService,
+      StopGatewaySenderOnMemberFactory stopperOnMemberFactory) {
+    this.executorService = executorService;
+    this.managementService = managementService;
+    this.stopperOnMemberFactory = stopperOnMemberFactory;
+  }
+
+  public StopGatewaySenderCommandDelegateParallelImpl(SystemManagementService managementService) {
+    executorService = LoggingExecutors.newCachedThreadPool("Stop Sender Command Thread ", true);;
+    stopperOnMemberFactory = new StopGatewaySenderOnMemberFactoryWithBeanImpl();
+    this.managementService = managementService;
+  }

Review comment:
       Remember to always chain constructors. Only one constructor should write to the fields. The other constructors call each other and finally the one true constructor.
   
   You generally want to order the constructors such that the widest scope is at the top and the one true constructor is the last one.
   
   Neither constructor needs to be public -- both can be package-private in this case.
   
   You can also delete `StopGatewaySenderOnMemberFactoryWithBeanImpl` and just inline `StopGatewaySenderOnMemberWithBeanImpl::new` for the factory argument.
   
   I recommend:
   ```
     import static org.apache.geode.logging.internal.executors.LoggingExecutors.newCachedThreadPool;
   
     StopGatewaySenderCommandDelegateParallelImpl(SystemManagementService managementService) {
       this(managementService,
           newCachedThreadPool("Stop Sender Command Thread ", true),
           StopGatewaySenderOnMemberWithBeanImpl::new);
     }
   
    StopGatewaySenderCommandDelegateParallelImpl(SystemManagementService managementService,
           ExecutorService executorService,
           StopGatewaySenderOnMemberFactory stopperOnMemberFactory) {
       this.managementService = managementService;
       this.executorService = executorService;
       this.stopperOnMemberFactory = stopperOnMemberFactory;
     }
   ```

##########
File path: geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMemberWithBeanImplTest.java
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.ArrayList;
+
+import junitparams.Parameters;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.distributed.DistributedSystem;
+import org.apache.geode.management.GatewaySenderMXBean;
+import org.apache.geode.management.internal.SystemManagementService;
+import org.apache.geode.test.junit.runners.GeodeParamsRunner;
+
+@RunWith(GeodeParamsRunner.class)
+public class StopGatewaySenderOnMemberWithBeanImplTest {
+  String senderId = "sender1";
+  String memberId = "member1";
+  String remoteMemberId = "remoteMember1";
+  Cache cache;
+  SystemManagementService managementService;
+  DistributedMember distributedMember;
+  DistributedMember remoteDistributedMember;
+  DistributedSystem distributedSystem;
+  GatewaySenderMXBean bean;
+
+  @Before
+  public void setUp() {
+    cache = mock(Cache.class);
+    bean = mock(GatewaySenderMXBean.class);
+    managementService = mock(SystemManagementService.class);
+    distributedMember = mock(DistributedMember.class);
+    remoteDistributedMember = mock(DistributedMember.class);
+    when(distributedMember.getId()).thenReturn(memberId);
+    when(remoteDistributedMember.getId()).thenReturn(remoteMemberId);
+    distributedSystem = mock(DistributedSystem.class);
+    doReturn(distributedSystem).when(cache).getDistributedSystem();
+  }
+
+  @Test
+  @Parameters({"true", "false"})
+  public void executeStopGatewaySenderOnMemberNotRunningReturnsNotRunningError(
+      boolean isLocalMember) {
+    StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl();
+    setUpMocks(isLocalMember, false);
+    doReturn(false).when(bean).isRunning();
+    ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache,
+        managementService, distributedMember);
+    assertThat(result).containsExactly(memberId, "Error",
+        "GatewaySender sender1 is not running on member " + memberId + ".");
+  }
+
+  @Test
+  @Parameters({"true", "false"})
+  public void executeStopGatewaySenderOnMemberNotAvailableReturnsNotAvailableError(
+      boolean isLocalMember) {
+    StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl();
+    setUpMocks(isLocalMember, true);
+    ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache,
+        managementService, distributedMember);
+    assertThat(result).containsExactly(memberId, "Error",
+        "GatewaySender sender1 is not available on member " + memberId);
+  }
+
+  @Test
+  @Parameters({"true", "false"})
+  public void executeStopGatewaySenderOnMemberRunningReturnsOk(boolean isLocalMember) {
+    StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl();
+    setUpMocks(isLocalMember, false);
+    doReturn(true).when(bean).isRunning();
+    ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache,
+        managementService, distributedMember);
+    assertThat(result).containsExactly(memberId, "OK",
+        "GatewaySender sender1 is stopped on member " + memberId);
+  }
+
+  private void setUpMocks(boolean isLocalMember, boolean beanMustBeNull) {

Review comment:
       How about more descriptive names like:
   ```
   private GatewaySenderMXBean gatewaySenderMXBean(boolean isLocalMember, boolean mustBeNull) {
     GatewaySenderMXBean gatewaySenderMXBean = mustBeNull 
         ? null 
         : mock(GatewaySenderMXBean.class);
     // ...
     return gatewaySenderMXBean;
   }
   ```
   You can then delete the field `private GatewaySenderMXBean bean;` and change the tests do something like:
   ```
   @Test
     @Parameters({"true", "false"})
     public void executeStopGatewaySenderOnMemberRunningReturnsOk(boolean isLocalMember) {
       GatewaySenderMXBean gatewaySenderMXBean = gatewaySenderMXBean(isLocalMember, false);
       when(gatewaySenderMXBean.isRunning()).thenReturn(true);
   
       StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl();
   
       List<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache, managementService, distributedMember);
   
       assertThat(result)
           .containsExactly(memberId, "OK", "GatewaySender sender1 is stopped on member " + memberId);
     }
   ```

##########
File path: geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommandDelegateParallelImplTest.java
##########
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.management.internal.SystemManagementService;
+import org.apache.geode.management.internal.cli.result.model.ResultModel;
+import org.apache.geode.management.internal.cli.result.model.TabularResultModel;
+import org.apache.geode.management.internal.i18n.CliStrings;
+
+public class StopGatewaySenderCommandDelegateParallelImplTest {
+  private final String senderId = "sender1";
+  private Cache cache;
+  Set<DistributedMember> members;
+  ExecutorService executorService;
+  SystemManagementService managementService;
+  StopGatewaySenderOnMemberFactory stopperOnMemberFactory;

Review comment:
       These should be `private` unless you subclass the test itself in which case `protected` might be appropriate.

##########
File path: geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMemberWithBeanImplTest.java
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.ArrayList;
+
+import junitparams.Parameters;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.distributed.DistributedSystem;
+import org.apache.geode.management.GatewaySenderMXBean;
+import org.apache.geode.management.internal.SystemManagementService;
+import org.apache.geode.test.junit.runners.GeodeParamsRunner;
+
+@RunWith(GeodeParamsRunner.class)
+public class StopGatewaySenderOnMemberWithBeanImplTest {
+  String senderId = "sender1";
+  String memberId = "member1";
+  String remoteMemberId = "remoteMember1";
+  Cache cache;
+  SystemManagementService managementService;
+  DistributedMember distributedMember;
+  DistributedMember remoteDistributedMember;
+  DistributedSystem distributedSystem;
+  GatewaySenderMXBean bean;
+
+  @Before
+  public void setUp() {
+    cache = mock(Cache.class);
+    bean = mock(GatewaySenderMXBean.class);
+    managementService = mock(SystemManagementService.class);
+    distributedMember = mock(DistributedMember.class);
+    remoteDistributedMember = mock(DistributedMember.class);
+    when(distributedMember.getId()).thenReturn(memberId);
+    when(remoteDistributedMember.getId()).thenReturn(remoteMemberId);
+    distributedSystem = mock(DistributedSystem.class);
+    doReturn(distributedSystem).when(cache).getDistributedSystem();
+  }
+
+  @Test
+  @Parameters({"true", "false"})
+  public void executeStopGatewaySenderOnMemberNotRunningReturnsNotRunningError(
+      boolean isLocalMember) {
+    StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl();
+    setUpMocks(isLocalMember, false);
+    doReturn(false).when(bean).isRunning();
+    ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache,
+        managementService, distributedMember);
+    assertThat(result).containsExactly(memberId, "Error",
+        "GatewaySender sender1 is not running on member " + memberId + ".");
+  }
+
+  @Test
+  @Parameters({"true", "false"})
+  public void executeStopGatewaySenderOnMemberNotAvailableReturnsNotAvailableError(
+      boolean isLocalMember) {
+    StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl();
+    setUpMocks(isLocalMember, true);
+    ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache,
+        managementService, distributedMember);
+    assertThat(result).containsExactly(memberId, "Error",
+        "GatewaySender sender1 is not available on member " + memberId);
+  }
+
+  @Test
+  @Parameters({"true", "false"})
+  public void executeStopGatewaySenderOnMemberRunningReturnsOk(boolean isLocalMember) {
+    StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl();
+    setUpMocks(isLocalMember, false);
+    doReturn(true).when(bean).isRunning();
+    ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache,

Review comment:
       `List<String>`

##########
File path: geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMemberWithBeanImplTest.java
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.ArrayList;
+
+import junitparams.Parameters;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.distributed.DistributedSystem;
+import org.apache.geode.management.GatewaySenderMXBean;
+import org.apache.geode.management.internal.SystemManagementService;
+import org.apache.geode.test.junit.runners.GeodeParamsRunner;
+
+@RunWith(GeodeParamsRunner.class)
+public class StopGatewaySenderOnMemberWithBeanImplTest {
+  String senderId = "sender1";
+  String memberId = "member1";
+  String remoteMemberId = "remoteMember1";
+  Cache cache;
+  SystemManagementService managementService;
+  DistributedMember distributedMember;
+  DistributedMember remoteDistributedMember;
+  DistributedSystem distributedSystem;
+  GatewaySenderMXBean bean;
+
+  @Before
+  public void setUp() {
+    cache = mock(Cache.class);
+    bean = mock(GatewaySenderMXBean.class);
+    managementService = mock(SystemManagementService.class);
+    distributedMember = mock(DistributedMember.class);
+    remoteDistributedMember = mock(DistributedMember.class);
+    when(distributedMember.getId()).thenReturn(memberId);
+    when(remoteDistributedMember.getId()).thenReturn(remoteMemberId);
+    distributedSystem = mock(DistributedSystem.class);
+    doReturn(distributedSystem).when(cache).getDistributedSystem();
+  }
+
+  @Test
+  @Parameters({"true", "false"})
+  public void executeStopGatewaySenderOnMemberNotRunningReturnsNotRunningError(
+      boolean isLocalMember) {
+    StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl();
+    setUpMocks(isLocalMember, false);
+    doReturn(false).when(bean).isRunning();
+    ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache,
+        managementService, distributedMember);
+    assertThat(result).containsExactly(memberId, "Error",
+        "GatewaySender sender1 is not running on member " + memberId + ".");
+  }
+
+  @Test
+  @Parameters({"true", "false"})
+  public void executeStopGatewaySenderOnMemberNotAvailableReturnsNotAvailableError(
+      boolean isLocalMember) {
+    StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl();
+    setUpMocks(isLocalMember, true);
+    ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache,

Review comment:
       `List<String>`

##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMemberFactory.java
##########
@@ -0,0 +1,20 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+interface StopGatewaySenderOnMemberFactory {

Review comment:
       `@FunctionalInterface`
   
   Since this interface is so simple (no arguments to `create`), you could simply use `java.util.function.Supplier` instead.




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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] albertogpz commented on a change in pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

albertogpz commented on a change in pull request #7108:
URL: https://github.com/apache/geode/pull/7108#discussion_r773332443



##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommand.java
##########
@@ -16,27 +16,40 @@
 package org.apache.geode.management.internal.cli.commands;
 
 import java.util.Set;
-
-import javax.management.ObjectName;
+import java.util.function.BiFunction;
+import java.util.function.Supplier;
 
 import org.springframework.shell.core.annotation.CliCommand;
 import org.springframework.shell.core.annotation.CliOption;
 
+import org.apache.geode.cache.Cache;
 import org.apache.geode.distributed.DistributedMember;
-import org.apache.geode.internal.cache.InternalCache;
-import org.apache.geode.management.GatewaySenderMXBean;
 import org.apache.geode.management.cli.CliMetaData;
 import org.apache.geode.management.cli.ConverterHint;
 import org.apache.geode.management.cli.GfshCommand;
-import org.apache.geode.management.internal.SystemManagementService;
 import org.apache.geode.management.internal.cli.result.model.ResultModel;
-import org.apache.geode.management.internal.cli.result.model.TabularResultModel;
 import org.apache.geode.management.internal.i18n.CliStrings;
 import org.apache.geode.management.internal.security.ResourceOperation;
 import org.apache.geode.security.ResourcePermission;
 
 public class StopGatewaySenderCommand extends GfshCommand {
+  private Supplier<StopGatewaySenderCommandDelegate> commandDelegateSupplier;
+  private BiFunction<String[], String[], Set<DistributedMember>> findMembers;
+
+  @SuppressWarnings("unused") // invoked by spring shell
+  public StopGatewaySenderCommand() {
+    this(null, null);
+    findMembers = this::findMembers;
+    commandDelegateSupplier = this::getCommandDelegate;
+  }
+
+  StopGatewaySenderCommand(Supplier<StopGatewaySenderCommandDelegate> commandDelegateSupplier,

Review comment:
       I removed the delegate class. Now all unit tests are in `StopGatewaySenderCommandTest.java`.




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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] albertogpz commented on a change in pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

albertogpz commented on a change in pull request #7108:
URL: https://github.com/apache/geode/pull/7108#discussion_r752459231



##########
File path: geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommandTest.java
##########
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.management.ManagementService;
+import org.apache.geode.management.internal.SystemManagementService;
+import org.apache.geode.management.internal.functions.CliFunctionResult;
+import org.apache.geode.test.junit.rules.GfshParserRule;
+
+public class StopGatewaySenderCommandTest {

Review comment:
       I refactored the code quite a bit (not sure if I over-engineered) in order to be able to test everything without breaking encapsulation.




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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] albertogpz commented on a change in pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

albertogpz commented on a change in pull request #7108:
URL: https://github.com/apache/geode/pull/7108#discussion_r753436317



##########
File path: geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommandTest.java
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.management.internal.SystemManagementService;
+import org.apache.geode.management.internal.cli.result.model.ResultModel;
+import org.apache.geode.management.internal.functions.CliFunctionResult;
+import org.apache.geode.test.junit.rules.GfshParserRule;
+
+public class StopGatewaySenderCommandTest {
+  @ClassRule
+  public static GfshParserRule gfsh = new GfshParserRule();
+
+  private StopGatewaySenderCommand command;
+
+  @Before
+  public void before() {
+    command = spy(StopGatewaySenderCommand.class);
+    List<CliFunctionResult> functionResults = new ArrayList<>();
+    doReturn(functionResults).when(command).executeAndGetFunctionResult(any(), any(), any());

Review comment:
       @kirklund Thanks a lot for the suggestion. I finally have not needed to modify `GfshCommand`.
   Nevertheless, I had to inject the command delegate and the `findMembers` test behavior using the constructor.
   The outcome has not been very clean. If you have any suggestions to make it cleaner, please let me know.
   
   Given that `findMembers` is not a static method, I could not pass it when calling the last constructor. Instead, I had to set it after I called it.
   Something similar happened when trying to inject the delegate to the command.




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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] albertogpz commented on a change in pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

albertogpz commented on a change in pull request #7108:
URL: https://github.com/apache/geode/pull/7108#discussion_r753436317



##########
File path: geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommandTest.java
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.management.internal.SystemManagementService;
+import org.apache.geode.management.internal.cli.result.model.ResultModel;
+import org.apache.geode.management.internal.functions.CliFunctionResult;
+import org.apache.geode.test.junit.rules.GfshParserRule;
+
+public class StopGatewaySenderCommandTest {
+  @ClassRule
+  public static GfshParserRule gfsh = new GfshParserRule();
+
+  private StopGatewaySenderCommand command;
+
+  @Before
+  public void before() {
+    command = spy(StopGatewaySenderCommand.class);
+    List<CliFunctionResult> functionResults = new ArrayList<>();
+    doReturn(functionResults).when(command).executeAndGetFunctionResult(any(), any(), any());

Review comment:
       @kirklund Thanks a lot for the suggestion. I finally have not needed to modify `GfshCommand`.
   Nevertheless, I had to inject the command delegate and the `findMembers` test behavior using the constructor.
   The outcome has not been very clean (I was not able to just chain the constructors). If you have any suggestions to make it cleaner, please let me know.
   
   Given that `findMembers` is not a static method, I could not pass it when calling the last constructor. Instead, I had to set it after I called it.
   Something similar happened when trying to inject the delegate to the command.




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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] albertogpz commented on a change in pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

albertogpz commented on a change in pull request #7108:
URL: https://github.com/apache/geode/pull/7108#discussion_r773389266



##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommand.java
##########
@@ -36,7 +44,30 @@
 import org.apache.geode.security.ResourcePermission;
 
 public class StopGatewaySenderCommand extends GfshCommand {
+  private final ExecutorService executorService;
+  private SystemManagementService managementService;
+  private BiFunction<String[], String[], Set<DistributedMember>> findMembers;
+  private final Supplier<StopGatewaySenderOnMember> stopperOnMemberFactory;
+
+  @SuppressWarnings("unused") // invoked by spring shell
+  public StopGatewaySenderCommand() {
+    this(newCachedThreadPool("Stop Sender Command Thread ", true),
+        StopGatewaySenderOnMemberWithBeanImpl::new, null, null);

Review comment:
       I am passing the `SystemManagmentService` instance and the `findMembers()` function to avoid using a spy, which is not considered a good practice unless you are forced to because you are adding tests to legacy code.
   I do not know in this case, given that the methods are provided by the base class, using a spy would be justified.
   
   I have changed the code to pass to the constructor an instance of `StopGatewaySenderOnMember` instead of the supplier as you have suggested.




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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #7108: GEODE-9807: gfsh gateway sender stop in parallel (like start)

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

jinmeiliao commented on pull request #7108:
URL: https://github.com/apache/geode/pull/7108#issuecomment-969509003


   same comments as @mhansonp . Would need some unit tests for the refactor.


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

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org