[GitHub] [geode-native] gaussianrecurrence opened a new pull request #919: GEODE-10017: Fix servers restart within new ITs

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

gaussianrecurrence opened a new pull request #919:
URL: https://github.com/apache/geode-native/pull/919


    - Due to a rare race-condition described on the Jira case, it could
      happen that some test cases that needed to restart a/some server(s)
      could end up getting stuck.
    - This change introduces a mechanism to verify the server process has
      been actually stopped.
    - Also, removed sleeps on some TCs which were put in place as a WA.
    - Modified all of the new IT TCs that were restarting servers in order
      to use the above mentioned mechanism, so the race-condition is
      avoided.


-- 
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-native] pdxcodemonkey commented on a change in pull request #919: GEODE-10017: Fix servers restart within new ITs

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

pdxcodemonkey commented on a change in pull request #919:
URL: https://github.com/apache/geode-native/pull/919#discussion_r817836093



##########
File path: cppcache/integration/test/FunctionExecutionTest.cpp
##########
@@ -324,7 +324,9 @@ TEST(FunctionExecutionTest, testThatFunctionExecutionThrowsExceptionNonHA) {
       .withType("PARTITION")
       .execute();
 
-  cluster.getServers()[2].stop();
+  auto &targetServer = cluster.getServers()[2];
+  targetServer.stop();
+  targetServer.wait();

Review comment:
       Why this separation of concerns between stop() and wait()?  Is there ever a circumstance where we call stop() and don't intend for the process to actually be terminated yet when the function returns?

##########
File path: cppcache/integration/test/PdxInstanceTest.cpp
##########
@@ -395,9 +395,8 @@ TEST(PdxInstanceTest, testInstancePutAfterRestart) {
     cv_status.wait(lock, [&status] { return !status; });
   }
 
-  std::this_thread::sleep_for(std::chrono::seconds{30});
-
   for (auto& server : cluster.getServers()) {
+    server.wait();

Review comment:
       Okay I see this used separately here in several places, but... would it be necessary to call wait() in the other places if stop() just actually made sure the process was terminated?




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