[GitHub] [geode] upthewaterspout commented on a change in pull request #6704: GEODE-9338: Remove strong guarantees for Radish PUBLISH command

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

upthewaterspout commented on a change in pull request #6704:
URL: https://github.com/apache/geode/pull/6704#discussion_r685574132



##########
File path: geode-apis-compatible-with-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/pubsub/PubSubDUnitTest.java
##########
@@ -244,18 +243,17 @@ public void shouldContinueToFunction_whenOneServerShutsDownAbruptly_givenTwoSubs
     Future<Void> subscriber2Future =
         executor.submit(() -> subscriber2.subscribe(mockSubscriber2, CHANNEL_NAME));
 
-    assertThat(latch.await(30, TimeUnit.SECONDS)).as("channel subscription was not received")
-        .isTrue();
+    assertThat(latch.await(30, TimeUnit.SECONDS))

Review comment:
       Maybe just use our default of 5 minutes then? GeodeAwaitility.DEFAULT_TIMEOUT.

##########
File path: geode-apis-compatible-with-redis/src/acceptanceTest/java/org/apache/geode/redis/internal/executor/pubsub/PubSubNativeRedisAcceptanceTest.java
##########
@@ -15,17 +15,65 @@
 
 package org.apache.geode.redis.internal.executor.pubsub;
 
+import java.io.BufferedReader;
+import java.io.InputStreamReader;
+import java.time.Duration;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.logging.log4j.Logger;
+import org.buildobjects.process.ProcBuilder;
+import org.buildobjects.process.StreamConsumer;
+import org.junit.AfterClass;
 import org.junit.ClassRule;
 
 import org.apache.geode.NativeRedisTestRule;
+import org.apache.geode.logging.internal.log4j.api.LogService;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
 
 public class PubSubNativeRedisAcceptanceTest extends AbstractPubSubIntegrationTest {
+
+  private static final Logger logger = LogService.getLogger();
+
   @ClassRule
   public static NativeRedisTestRule redis = new NativeRedisTestRule();
 
+  @AfterClass
+  public static void cleanup() {
+    // This test consumes a lot of sockets and any subsequent tests may fail because of spurious
+    // bind exceptions. Even though sockets are closed, they will remain in TIME_WAIT state so we
+    // need to wait for that to clear up. It shouldn't take more than a minute or so.
+    GeodeAwaitility.await().pollInterval(Duration.ofSeconds(10))
+        .until(() -> countTimeWait() < 100);

Review comment:
       This seems kinda sketchy. I know for example Dale is working on running tests in parallel - wouldn't that mean 100s of sockets in TIMED_WAIT all the time? I always get worried about tests that are OS dependent or trying to make general assertions about the state of the machine at the time the test is running.
   
   What sockets are getting bind exceptions? Geode uses SO_REUSEADDR. Is redis not doing that? I was able to start redis twice on the same port from the command line.




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