From 14e9acda3f39f6fedf867f4a4125032e6c99cbcf Mon Sep 17 00:00:00 2001 From: David Turner Date: Fri, 26 Jan 2024 13:52:29 +0000 Subject: [PATCH] Dedicated logger for Netty4HttpPipeliningHandler (#104806) Better to use a logger for the class that's doing the logging rather than deferring back to the owning transport's logger. --- .../http/netty4/Netty4HttpPipeliningHandler.java | 7 +++---- .../http/netty4/Netty4HttpServerTransport.java | 2 +- .../http/netty4/Netty4HttpPipeliningHandlerTests.java | 6 +++--- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java index a7ecb85bda47c..b872909e9b7ed 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java @@ -25,6 +25,7 @@ import io.netty.util.concurrent.Future; import io.netty.util.concurrent.PromiseCombiner; +import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.common.bytes.ReleasableBytesReference; @@ -52,7 +53,7 @@ */ public class Netty4HttpPipeliningHandler extends ChannelDuplexHandler { - private final Logger logger; + private static final Logger logger = LogManager.getLogger(Netty4HttpPipeliningHandler.class); private final int maxEventsHeld; private final PriorityQueue> outboundHoldingQueue; @@ -86,12 +87,10 @@ private record ChunkedWrite(PromiseCombiner combiner, ChannelPromise onDone, Chu /** * Construct a new pipelining handler; this handler should be used downstream of HTTP decoding/aggregation. * - * @param logger for logging unexpected errors * @param maxEventsHeld the maximum number of channel events that will be retained prior to aborting the channel connection; this is * required as events cannot queue up indefinitely */ - public Netty4HttpPipeliningHandler(Logger logger, final int maxEventsHeld, final Netty4HttpServerTransport serverTransport) { - this.logger = logger; + public Netty4HttpPipeliningHandler(final int maxEventsHeld, final Netty4HttpServerTransport serverTransport) { this.maxEventsHeld = maxEventsHeld; this.outboundHoldingQueue = new PriorityQueue<>(1, Comparator.comparingInt(t -> t.v1().getSequence())); this.serverTransport = serverTransport; diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java index 4bee7cd52a214..274240a40bd46 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java @@ -430,7 +430,7 @@ protected boolean isContentAlwaysEmpty(HttpResponse msg) { if (handlingSettings.compression()) { ch.pipeline().addLast("encoder_compress", new HttpContentCompressor(handlingSettings.compressionLevel())); } - ch.pipeline().addLast("pipelining", new Netty4HttpPipeliningHandler(logger, transport.pipeliningMaxEvents, transport)); + ch.pipeline().addLast("pipelining", new Netty4HttpPipeliningHandler(transport.pipeliningMaxEvents, transport)); transport.serverAcceptedChannel(nettyHttpChannel); } diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandlerTests.java b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandlerTests.java index 05f9f1bbb19f0..9e0f30caec755 100644 --- a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandlerTests.java +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandlerTests.java @@ -119,7 +119,7 @@ public void testThatPipeliningWorksWithFastSerializedRequests() throws Interrupt } private EmbeddedChannel makeEmbeddedChannelWithSimulatedWork(int numberOfRequests) { - return new EmbeddedChannel(new Netty4HttpPipeliningHandler(logger, numberOfRequests, null) { + return new EmbeddedChannel(new Netty4HttpPipeliningHandler(numberOfRequests, null) { @Override protected void handlePipelinedRequest(ChannelHandlerContext ctx, Netty4HttpRequest pipelinedRequest) { ctx.fireChannelRead(pipelinedRequest); @@ -185,7 +185,7 @@ public void testThatPipeliningClosesConnectionWithTooManyEvents() throws Interru public void testPipeliningRequestsAreReleased() { final int numberOfRequests = 10; - final EmbeddedChannel embeddedChannel = new EmbeddedChannel(new Netty4HttpPipeliningHandler(logger, numberOfRequests + 1, null)); + final EmbeddedChannel embeddedChannel = new EmbeddedChannel(new Netty4HttpPipeliningHandler(numberOfRequests + 1, null)); for (int i = 0; i < numberOfRequests; i++) { embeddedChannel.writeInbound(createHttpRequest("/" + i)); @@ -493,7 +493,7 @@ private static void assertDoneWithClosedChannel(ChannelPromise chunkedWritePromi } private Netty4HttpPipeliningHandler getTestHttpHandler() { - return new Netty4HttpPipeliningHandler(logger, Integer.MAX_VALUE, mock(Netty4HttpServerTransport.class)) { + return new Netty4HttpPipeliningHandler(Integer.MAX_VALUE, mock(Netty4HttpServerTransport.class)) { @Override protected void handlePipelinedRequest(ChannelHandlerContext ctx, Netty4HttpRequest pipelinedRequest) { ctx.fireChannelRead(pipelinedRequest);