[GitHub] [geode] Bill commented on a diff in pull request #7697: GEODE-10304: locator thread should not exit after reconnecting

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

Bill commented on code in PR #7697:
URL: https://github.com/apache/geode/pull/7697#discussion_r879980214


##########
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalLocator.java:
##########
@@ -1043,7 +1046,15 @@ public void waitToStop() throws InterruptedException {
         while (system.isConnected()) {
           Thread.sleep(5000);
         }
+        // there would be a gap between stoppedForReconnect being to true and attemptingToReconnect
+        // being true, if system.waitUntilReconnected happened in between, this method would return
+        // with "restarted" being false, so we need also to wait till system is reconnecting

Review Comment:
   I kind of understand what's going on. Could this comment be rephrased so that instead of talking about private fields of `InternalDistributedSystem`, it talked about the contract provided by the `DistributedSystem` interface?
   
   Also I wonder about the other two places in production code where we call `IDS.waitUntilReconnected()`. How do those two places avoid the same problem we see here (before this PR)?
   
   The other two places are: `InternalLocator.attemptReconnect()` and `GemFireCacheImpl.waitUntilReconnected()`. Neither of those call `IDS.isReconnecting()` so it makes me think that this method should follow suit. If not, I think the comment should explain why we're doing it differently here.



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