Cleanup a couple tests and simplify the ClientConnectionPool's shutdo…

…wnNow() method.

One test is disabled because it's actually a terrible idea in practice (see disabled description).
Another test was broken because a trailing newline didn't follow request headers, which caused the server to get the content of two requests in one pass.  The count for the requestConnecting value was also off.

Signed-off-by: Greg Schohn <[email protected]>

TrafficCapture/trafficReplayer/src/main/java/org/opensearch/migrations/replay/ClientConnectionPool.java

@@ -122,26 +122,7 @@ public void closeConnection(IReplayContexts.IChannelKeyContext ctx, int sessionN
     public CompletableFuture<Void> shutdownNow() {
         log.atInfo().setMessage("Shutting down ClientConnectionPool").log();
         var rval = NettyFutureBinders.bindNettyFutureToCompletableFuture(eventLoopGroup.shutdownGracefully());
-        try {
-            connectionId2ChannelCache.asMap().forEach((k, v) -> {
-                log.atDebug().setMessage("closing client connection for " + k + " -> " + v).log();
-                try {
-                    closeClientConnectionChannel(v);
-                    log.atTrace().setMessage("done closing client connection for " + k + " -> " + v).log();
-                } catch (Exception t) {
-                    log.atError().setCause(t).setMessage("Caught while calling closeClientConnectionChannel").log();
-                }
-            });
-        } catch (Throwable t) {
-            log.atError().setCause(t).setMessage("Caught while calling closeClientConnectionChannel, " +
-                "propagating Throwable (exceptionally) through the returned CompletableFuture").log();
-            rval.completeExceptionally(t);
-            return rval;
-        } finally {
-            connectionId2ChannelCache.invalidateAll();
-            log.atDebug().setMessage(() -> "Invalidated cache and returning eventLoopGroup shutdown future").log();
-        }
-        log.atDebug().setMessage(() -> "Done closing client connections for all cached entries").log();
+        connectionId2ChannelCache.invalidateAll();
         return rval;
     }
 

TrafficCapture/trafficReplayer/src/test/java/org/opensearch/migrations/replay/e2etests/FullReplayerWithTracingChecksTest.java

@@ -80,7 +80,7 @@ public void testStreamWithRequestsWithCloseIsCommittedOnce(int numRequests) thro
                                             + i
                                             + " HTTP/1.1\r\n"
                                             + "Connection: Keep-Alive\r\n"
-                                            + "Host: localhost\r\n").getBytes(StandardCharsets.UTF_8)
+                                            + "Host: localhost\r\n\r\n").getBytes(StandardCharsets.UTF_8)
                                     )
                                 )
                                 .build()
@@ -191,7 +191,7 @@ private void checkSpansForSimpleReplayedTransactions(
         Assertions.assertEquals(numRequests, traceProcessor.getCountAndRemoveSpan("targetTransaction"));
         Assertions.assertEquals(numRequests * 2, traceProcessor.getCountAndRemoveSpan("scheduled"));
         Assertions.assertEquals(numRequests, traceProcessor.getCountAndRemoveSpan("requestSending"));
-        Assertions.assertEquals(numRequests, traceProcessor.getCountAndRemoveSpan("requestConnecting"));
+        Assertions.assertEquals(1, traceProcessor.getCountAndRemoveSpan("requestConnecting"));
         Assertions.assertEquals(numRequests, traceProcessor.getCountAndRemoveSpan("comparingResults"));
 
         Assertions.assertTrue(traceProcessor.getCountAndRemoveSpan("waitingForResponse") > 0);

TrafficCapture/trafficReplayer/src/test/java/org/opensearch/migrations/replay/http/retries/HttpRetryTest.java

@@ -10,6 +10,7 @@
 import java.util.concurrent.atomic.AtomicInteger;
 
 import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Tag;
 import org.junit.jupiter.api.Test;
 
@@ -200,6 +201,11 @@ static long checkHttpRetryConsistency(TestContext rootContext) {
         return retryMetricCount;
     }
 
+    @Disabled(value = "This policy opens the replayer up to easy DOS patterns that would halt the replayer.  " +
+        "Specifically, if a request is ill-defined and that causes a prematurely closed connection from the server," +
+        "that single request would cause the replayer to spin on it indefinitely.  Clearly, malformed requests " +
+        "shouldn't have such a great impact.  Other options need to be considered to compensate for a other " +
+        "premature connection closed errors.")
     @Tag("longTest")
     @Test
     @WrapWithNettyLeakDetection(disableLeakChecks = true) // code is forcibly terminated so leaks are expected