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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
