You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2022/05/16 22:24:37 UTC

[GitHub] [geode] jinmeiliao opened a new pull request, #7697: GEODE-10304: locator thread should not exit after reconnecting

jinmeiliao opened a new pull request, #7697:
URL: https://github.com/apache/geode/pull/7697

   This flakiness shows up in a test. In a production environment, this is unlikely to happen.


-- 
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 diff in pull request #7697: GEODE-10304: locator thread should not exit after reconnecting

Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on code in PR #7697:
URL: https://github.com/apache/geode/pull/7697#discussion_r880891160


##########
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:
   `InternalLocator.attemptReconnect()` doesn't care about the return boolean, for `GemFireCacheImpl.waitUntilReconnected()`, I am not sure how it's using the return boolean. I need this `IDS.isReconnecting()` here specifically because `locator.waitToStop()` as the method name indicate, can't return unless the locator truly stops. So the call to the `IDS.waitUntilReconnected()` can't return false.



-- 
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 diff in pull request #7697: GEODE-10304: locator thread should not exit after reconnecting

Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7697:
URL: https://github.com/apache/geode/pull/7697#discussion_r888303887


##########
geode-core/src/distributedTest/java/org/apache/geode/cache30/ReconnectDUnitTest.java:
##########
@@ -320,143 +304,128 @@ public void doTestReconnectOnForcedDisconnect(final boolean createInAppToo) thro
     Invoke
         .invokeInEveryVM(() -> DistributedTestUtils.deleteLocatorStateFile(locPort, secondLocPort));
 
-
     final String xmlFileLoc = (new File(".")).getAbsolutePath();
 
-    SerializableCallable create1 =
-        new SerializableCallable("Create Cache and Regions from cache.xml") {
-          @Override
-          public Object call() throws CacheException {
-            locatorPort = locPort;
-            Properties props = getDistributedSystemProperties();
-            props.put(CACHE_XML_FILE, xmlFileLoc + fileSeparator + "MyDisconnect-cache.xml");
-            props.put(MAX_WAIT_TIME_RECONNECT, "1000");
-            cache = (InternalCache) new CacheFactory(props).create();
-            Region myRegion = cache.getRegion("root" + SEPARATOR + "myRegion");
-            ReconnectDUnitTest.savedSystem = cache.getDistributedSystem();
-            myRegion.put("MyKey1", "MyValue1");
-            return savedSystem.getDistributedMember();
-          }
-        };
+    SerializableCallableIF<DistributedMember> create1 = () -> {
+      locatorPort = locPort;
+      Properties props = getDistributedSystemProperties();
+      props.put(CACHE_XML_FILE, xmlFileLoc + fileSeparator + "MyDisconnect-cache.xml");
+      props.put(MAX_WAIT_TIME_RECONNECT, "1000");
+      cache = (InternalCache) new CacheFactory(props).create();
+      Region myRegion = cache.getRegion("root" + SEPARATOR + "myRegion");
+      ReconnectDUnitTest.savedSystem = cache.getDistributedSystem();
+      myRegion.put("MyKey1", "MyValue1");
+      return savedSystem.getDistributedMember();
+    };
 
-    SerializableCallable create2 =
-        new SerializableCallable("Create Cache and Regions from cache.xml") {
+    SerializableCallableIF<DistributedMember> create2 = () -> {
+      locatorPort = locPort;
+      final Properties props = getDistributedSystemProperties();
+      props.put(CACHE_XML_FILE, xmlFileLoc + fileSeparator + "MyDisconnect-cache.xml");
+      props.put(MAX_WAIT_TIME_RECONNECT, "5000");
+      props.put(START_LOCATOR, "localhost[" + secondLocPort + "]");
+      props.put(LOCATORS, props.get(LOCATORS) + ",localhost[" + secondLocPort + "]");
+      getSystem(props);
+      cache = getCache();
+      ReconnectDUnitTest.savedSystem = cache.getDistributedSystem();
+      Region myRegion = cache.getRegion("root" + SEPARATOR + "myRegion");
+      myRegion.put("Mykey2", "MyValue2");
+      assertNotNull(myRegion.get("MyKey1"));
+      if (createInAppToo) {
+        Thread recreateCacheThread = new Thread("ReconnectDUnitTest.createInAppThread") {
           @Override
-          public Object call() throws CacheException {
-            locatorPort = locPort;
-            final Properties props = getDistributedSystemProperties();
-            props.put(CACHE_XML_FILE, xmlFileLoc + fileSeparator + "MyDisconnect-cache.xml");
-            props.put(MAX_WAIT_TIME_RECONNECT, "5000");
-            props.put(START_LOCATOR, "localhost[" + secondLocPort + "]");
-            props.put(LOCATORS, props.get(LOCATORS) + ",localhost[" + secondLocPort + "]");
-            getSystem(props);
-            cache = getCache();
-            ReconnectDUnitTest.savedSystem = cache.getDistributedSystem();
-            Region myRegion = cache.getRegion("root" + SEPARATOR + "myRegion");
-            myRegion.put("Mykey2", "MyValue2");
-            assertNotNull(myRegion.get("MyKey1"));
-            if (createInAppToo) {
-              Thread recreateCacheThread = new Thread("ReconnectDUnitTest.createInAppThread") {
-                @Override
-                public void run() {
-                  while (!cache.isClosed()) {
-                    Wait.pause(100);
-                  }
-                  try {
-                    cache = (InternalCache) new CacheFactory(props).create();
-                    System.err.println(
-                        "testReconnectCollidesWithApplication failed - application thread was able to create a cache");
-                  } catch (IllegalStateException cacheExists) {
-                    // expected
-                  }
-                }
-              };
-              recreateCacheThread.setDaemon(true);
-              recreateCacheThread.start();
+          public void run() {
+            while (!cache.isClosed()) {
+              Wait.pause(100);
+            }
+            try {
+              cache = (InternalCache) new CacheFactory(props).create();
+              System.err.println(
+                  "testReconnectCollidesWithApplication failed - application thread was able to create a cache");
+            } catch (IllegalStateException cacheExists) {
+              // expected

Review Comment:
   If `cache = (InternalCache) new CacheFactory(props).create();` is supposed to throw `IllegalStateException`, then that `System.err.println` should change to `fail("testReconnectCollidesWithApplication failed - application thread was able to create a cache");` or you could change the whole try-catch block to use AssertJ `catchThrowable` with appropriate assertion.



-- 
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 diff in pull request #7697: GEODE-10304: locator thread should not exit after reconnecting

Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7697:
URL: https://github.com/apache/geode/pull/7697#discussion_r890433012


##########
geode-core/src/distributedTest/java/org/apache/geode/cache30/ReconnectDUnitTest.java:
##########
@@ -320,143 +304,128 @@ public void doTestReconnectOnForcedDisconnect(final boolean createInAppToo) thro
     Invoke
         .invokeInEveryVM(() -> DistributedTestUtils.deleteLocatorStateFile(locPort, secondLocPort));
 
-
     final String xmlFileLoc = (new File(".")).getAbsolutePath();
 
-    SerializableCallable create1 =
-        new SerializableCallable("Create Cache and Regions from cache.xml") {
-          @Override
-          public Object call() throws CacheException {
-            locatorPort = locPort;
-            Properties props = getDistributedSystemProperties();
-            props.put(CACHE_XML_FILE, xmlFileLoc + fileSeparator + "MyDisconnect-cache.xml");
-            props.put(MAX_WAIT_TIME_RECONNECT, "1000");
-            cache = (InternalCache) new CacheFactory(props).create();
-            Region myRegion = cache.getRegion("root" + SEPARATOR + "myRegion");
-            ReconnectDUnitTest.savedSystem = cache.getDistributedSystem();
-            myRegion.put("MyKey1", "MyValue1");
-            return savedSystem.getDistributedMember();
-          }
-        };
+    SerializableCallableIF<DistributedMember> create1 = () -> {
+      locatorPort = locPort;
+      Properties props = getDistributedSystemProperties();
+      props.put(CACHE_XML_FILE, xmlFileLoc + fileSeparator + "MyDisconnect-cache.xml");
+      props.put(MAX_WAIT_TIME_RECONNECT, "1000");
+      cache = (InternalCache) new CacheFactory(props).create();
+      Region myRegion = cache.getRegion("root" + SEPARATOR + "myRegion");
+      ReconnectDUnitTest.savedSystem = cache.getDistributedSystem();
+      myRegion.put("MyKey1", "MyValue1");
+      return savedSystem.getDistributedMember();
+    };
 
-    SerializableCallable create2 =
-        new SerializableCallable("Create Cache and Regions from cache.xml") {
+    SerializableCallableIF<DistributedMember> create2 = () -> {
+      locatorPort = locPort;
+      final Properties props = getDistributedSystemProperties();
+      props.put(CACHE_XML_FILE, xmlFileLoc + fileSeparator + "MyDisconnect-cache.xml");
+      props.put(MAX_WAIT_TIME_RECONNECT, "5000");
+      props.put(START_LOCATOR, "localhost[" + secondLocPort + "]");
+      props.put(LOCATORS, props.get(LOCATORS) + ",localhost[" + secondLocPort + "]");
+      getSystem(props);
+      cache = getCache();
+      ReconnectDUnitTest.savedSystem = cache.getDistributedSystem();
+      Region myRegion = cache.getRegion("root" + SEPARATOR + "myRegion");
+      myRegion.put("Mykey2", "MyValue2");
+      assertNotNull(myRegion.get("MyKey1"));
+      if (createInAppToo) {
+        Thread recreateCacheThread = new Thread("ReconnectDUnitTest.createInAppThread") {
           @Override
-          public Object call() throws CacheException {
-            locatorPort = locPort;
-            final Properties props = getDistributedSystemProperties();
-            props.put(CACHE_XML_FILE, xmlFileLoc + fileSeparator + "MyDisconnect-cache.xml");
-            props.put(MAX_WAIT_TIME_RECONNECT, "5000");
-            props.put(START_LOCATOR, "localhost[" + secondLocPort + "]");
-            props.put(LOCATORS, props.get(LOCATORS) + ",localhost[" + secondLocPort + "]");
-            getSystem(props);
-            cache = getCache();
-            ReconnectDUnitTest.savedSystem = cache.getDistributedSystem();
-            Region myRegion = cache.getRegion("root" + SEPARATOR + "myRegion");
-            myRegion.put("Mykey2", "MyValue2");
-            assertNotNull(myRegion.get("MyKey1"));
-            if (createInAppToo) {
-              Thread recreateCacheThread = new Thread("ReconnectDUnitTest.createInAppThread") {
-                @Override
-                public void run() {
-                  while (!cache.isClosed()) {
-                    Wait.pause(100);
-                  }
-                  try {
-                    cache = (InternalCache) new CacheFactory(props).create();
-                    System.err.println(
-                        "testReconnectCollidesWithApplication failed - application thread was able to create a cache");
-                  } catch (IllegalStateException cacheExists) {
-                    // expected
-                  }
-                }
-              };
-              recreateCacheThread.setDaemon(true);
-              recreateCacheThread.start();
+          public void run() {
+            while (!cache.isClosed()) {
+              Wait.pause(100);
+            }
+            try {
+              cache = (InternalCache) new CacheFactory(props).create();
+              System.err.println(
+                  "testReconnectCollidesWithApplication failed - application thread was able to create a cache");
+            } catch (IllegalStateException cacheExists) {
+              // expected

Review Comment:
   I know it's hassle but this sort of thing is probably important. But I'm also worried that it may have been dumbed down to a println because it might intermittently fail (due to timing). I'd probably look at the history of those lines to see if that happened. If not, then consider changing it to an assertion.



-- 
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 diff in pull request #7697: GEODE-10304: locator thread should not exit after reconnecting

Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on code in PR #7697:
URL: https://github.com/apache/geode/pull/7697#discussion_r888580622


##########
geode-core/src/distributedTest/java/org/apache/geode/cache30/ReconnectDUnitTest.java:
##########
@@ -320,143 +304,128 @@ public void doTestReconnectOnForcedDisconnect(final boolean createInAppToo) thro
     Invoke
         .invokeInEveryVM(() -> DistributedTestUtils.deleteLocatorStateFile(locPort, secondLocPort));
 
-
     final String xmlFileLoc = (new File(".")).getAbsolutePath();
 
-    SerializableCallable create1 =
-        new SerializableCallable("Create Cache and Regions from cache.xml") {
-          @Override
-          public Object call() throws CacheException {
-            locatorPort = locPort;
-            Properties props = getDistributedSystemProperties();
-            props.put(CACHE_XML_FILE, xmlFileLoc + fileSeparator + "MyDisconnect-cache.xml");
-            props.put(MAX_WAIT_TIME_RECONNECT, "1000");
-            cache = (InternalCache) new CacheFactory(props).create();
-            Region myRegion = cache.getRegion("root" + SEPARATOR + "myRegion");
-            ReconnectDUnitTest.savedSystem = cache.getDistributedSystem();
-            myRegion.put("MyKey1", "MyValue1");
-            return savedSystem.getDistributedMember();
-          }
-        };
+    SerializableCallableIF<DistributedMember> create1 = () -> {
+      locatorPort = locPort;
+      Properties props = getDistributedSystemProperties();
+      props.put(CACHE_XML_FILE, xmlFileLoc + fileSeparator + "MyDisconnect-cache.xml");
+      props.put(MAX_WAIT_TIME_RECONNECT, "1000");
+      cache = (InternalCache) new CacheFactory(props).create();
+      Region myRegion = cache.getRegion("root" + SEPARATOR + "myRegion");
+      ReconnectDUnitTest.savedSystem = cache.getDistributedSystem();
+      myRegion.put("MyKey1", "MyValue1");
+      return savedSystem.getDistributedMember();
+    };
 
-    SerializableCallable create2 =
-        new SerializableCallable("Create Cache and Regions from cache.xml") {
+    SerializableCallableIF<DistributedMember> create2 = () -> {
+      locatorPort = locPort;
+      final Properties props = getDistributedSystemProperties();
+      props.put(CACHE_XML_FILE, xmlFileLoc + fileSeparator + "MyDisconnect-cache.xml");
+      props.put(MAX_WAIT_TIME_RECONNECT, "5000");
+      props.put(START_LOCATOR, "localhost[" + secondLocPort + "]");
+      props.put(LOCATORS, props.get(LOCATORS) + ",localhost[" + secondLocPort + "]");
+      getSystem(props);
+      cache = getCache();
+      ReconnectDUnitTest.savedSystem = cache.getDistributedSystem();
+      Region myRegion = cache.getRegion("root" + SEPARATOR + "myRegion");
+      myRegion.put("Mykey2", "MyValue2");
+      assertNotNull(myRegion.get("MyKey1"));
+      if (createInAppToo) {
+        Thread recreateCacheThread = new Thread("ReconnectDUnitTest.createInAppThread") {
           @Override
-          public Object call() throws CacheException {
-            locatorPort = locPort;
-            final Properties props = getDistributedSystemProperties();
-            props.put(CACHE_XML_FILE, xmlFileLoc + fileSeparator + "MyDisconnect-cache.xml");
-            props.put(MAX_WAIT_TIME_RECONNECT, "5000");
-            props.put(START_LOCATOR, "localhost[" + secondLocPort + "]");
-            props.put(LOCATORS, props.get(LOCATORS) + ",localhost[" + secondLocPort + "]");
-            getSystem(props);
-            cache = getCache();
-            ReconnectDUnitTest.savedSystem = cache.getDistributedSystem();
-            Region myRegion = cache.getRegion("root" + SEPARATOR + "myRegion");
-            myRegion.put("Mykey2", "MyValue2");
-            assertNotNull(myRegion.get("MyKey1"));
-            if (createInAppToo) {
-              Thread recreateCacheThread = new Thread("ReconnectDUnitTest.createInAppThread") {
-                @Override
-                public void run() {
-                  while (!cache.isClosed()) {
-                    Wait.pause(100);
-                  }
-                  try {
-                    cache = (InternalCache) new CacheFactory(props).create();
-                    System.err.println(
-                        "testReconnectCollidesWithApplication failed - application thread was able to create a cache");
-                  } catch (IllegalStateException cacheExists) {
-                    // expected
-                  }
-                }
-              };
-              recreateCacheThread.setDaemon(true);
-              recreateCacheThread.start();
+          public void run() {
+            while (!cache.isClosed()) {
+              Wait.pause(100);
+            }
+            try {
+              cache = (InternalCache) new CacheFactory(props).create();
+              System.err.println(
+                  "testReconnectCollidesWithApplication failed - application thread was able to create a cache");
+            } catch (IllegalStateException cacheExists) {
+              // expected

Review Comment:
   I didn't change anything here except on line 321 to use lamda instead of `new SerializableCallable`. Github comparison viewer seems to have messed up 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


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

Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on code in PR #7697:
URL: https://github.com/apache/geode/pull/7697#discussion_r878477658


##########
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
         logger.info("waiting for distributed system to reconnect...");
+        while (!system.isReconnecting()) {

Review Comment:
   All other changes are just refactoring to use lamda instead of Runnable interface. Only this change is what matters.



##########
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
         logger.info("waiting for distributed system to reconnect...");
+        while (!system.isReconnecting()) {

Review Comment:
   All other changes are just refactoring to use lamda instead of Runnable interface. Only this change is what matters.



-- 
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 #7697: GEODE-10304: locator thread should not exit after reconnecting

Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on PR #7697:
URL: https://github.com/apache/geode/pull/7697#issuecomment-1145549256

   > I'm not sure what changes here were actually necessary to fix the bug. In general, I would recommend either making just the minimal change to fix the bug or do a more sweeting fixup of the test (ie, getting rid of all catch blocks, use ErrorCollector for anything that's an async callback if those exist, convert all JUnit Asserts and Hamcrest usages to AssertJ, etc). Either way, you should call out which lines actually fix the bug.
   > 
   > You might also consider stating if this fixes a product bug or a flaky test. `GEODE-10304: locator thread should not exit after reconnecting` makes it sound like you're fixing a product bug.
   
   The only product change is the change I made in `InternalLocator`. The changes in the test are just using lamda to replace `Callable` and `Runnable` interfaces. I will update the comment to reflect that.
   
   


-- 
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 merged pull request #7697: GEODE-10304: locator thread should not exit after reconnecting

Posted by GitBox <gi...@apache.org>.
jinmeiliao merged PR #7697:
URL: https://github.com/apache/geode/pull/7697


-- 
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] Bill commented on a diff in pull request #7697: GEODE-10304: locator thread should not exit after reconnecting

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


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

Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on code in PR #7697:
URL: https://github.com/apache/geode/pull/7697#discussion_r880891160


##########
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:
   `InternalLocator.attemptReconnect()` doesn't care about the return boolean, for `GemFireCacheImpl.waitUntilReconnected()`, I am not sure how it's using the return boolean. I need this `IDS.isReconnecting()` here specifically because `locator.waitToStop()` as the method name indicate, can't return unless the locator truly stops. So the call to the `IDS.waitUntilReconnected()` can't return false, thus it needs this call there to prevent that.



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