[GitHub] [beam] TheNeuralBit commented on a diff in pull request #17191: [BEAM-14157] GrpcWindmillServer: Use stream specific boolean to do client closed check

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

TheNeuralBit commented on code in PR #17191:
URL: https://github.com/apache/beam/pull/17191#discussion_r841946902


##########
runners/google-cloud-dataflow-java/worker/src/test/java/org/apache/beam/runners/dataflow/worker/windmill/GrpcWindmillServerTest.java:
##########
@@ -695,8 +705,24 @@ public void onCompleted() {
     }
     stream.flush();
     stream.close();
+    isClientClosed.set(true);
 
-    Thread.sleep(100);
+    long deadline = System.currentTimeMillis() + 60_000; // 1 min
+    while (true) {
+      Thread.sleep(100);
+      int tmpErrorsAfterClose = errorsAfterClose.get();
+      int tmpErrorsBeforeClose = errorsBeforeClose.get();
+      // wait for at least 2 errors before and after
+      if (tmpErrorsAfterClose > 1 && tmpErrorsBeforeClose > 1) {
+        break;
+      }
+      if (System.currentTimeMillis() > deadline) {
+        fail(

Review Comment:
   Sorry this is a bit of nit, but this check is actually verifying the test code itself, right? That is, if we get here there's likely a bug in the test code, not in the implementation? If so, it would be nice to clarify this in a comment in case this ever starts flaking.



-- 
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: github-unsubscribe@beam.apache.org

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