[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5728: GEODE-8696: Fix synchronization in FederatingManager

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

dschneider-pivotal commented on a change in pull request #5728:
URL: https://github.com/apache/geode/pull/5728#discussion_r535537581



##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/FederatingManager.java
##########
@@ -79,26 +82,39 @@
    * unbounded in practical situation as number of members will be a finite set at any given point
    * of time
    */
-  private ExecutorService executorService;
+  private final AtomicReference<ExecutorService> executorService = new AtomicReference<>();
+  private boolean pendingStartup = true;

Review comment:
       The concern I have with pendingStartup being set to true when FederatingManager is created is that removeMember could build up a big list of pending tasks if the call of startManager is delayed.
   In some cases we are careful to call start right after create. But in others, based on how the gemfire properties configure jmx, it can be delayed. If you look in ManagementAdapter this code:`      if (cache.getInternalDistributedSystem().getConfig().getJmxManager()) {
           service.createManager();
           if (cache.getInternalDistributedSystem().getConfig().getJmxManagerStart()) {
             service.startManager();
           }
         }
   `
   will not always call startManager. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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