functioner commented on a change in pull request #11504: URL: https://github.com/apache/kafka/pull/11504#discussion_r753687329
########## File path: core/src/main/scala/kafka/network/SocketServer.scala ########## @@ -734,6 +734,10 @@ private[kafka] class Acceptor(val endPoint: EndPoint, val endThrottleTimeMs = e.startThrottleTimeMs + e.throttleTimeMs throttledSockets += DelayedCloseSocket(socketChannel, endThrottleTimeMs) None + case e: IOException => + info(s"Encounter IOException", e) + closeSocket(socketChannel) Review comment: Since the existing test cases don't have scaffold for testing `Acceptor`, we need to prepare lots of parameters for `Acceptor` constructor https://github.com/apache/kafka/blob/074a03cca162f91ccdecc12eb84c6a45af75f6bf/core/src/main/scala/kafka/network/SocketServer.scala#L550-L558 We also need to prepare a `Processor` for the `Acceptor` https://github.com/apache/kafka/blob/074a03cca162f91ccdecc12eb84c6a45af75f6bf/core/src/main/scala/kafka/network/SocketServer.scala#L687-L703 https://github.com/apache/kafka/blob/074a03cca162f91ccdecc12eb84c6a45af75f6bf/core/src/main/scala/kafka/network/SocketServer.scala#L574-L578 https://github.com/apache/kafka/blob/074a03cca162f91ccdecc12eb84c6a45af75f6bf/core/src/main/scala/kafka/network/SocketServer.scala#L790-L806 However, those logic are implemented in `SocketServer` https://github.com/apache/kafka/blob/074a03cca162f91ccdecc12eb84c6a45af75f6bf/core/src/main/scala/kafka/network/SocketServer.scala#L268-L283 https://github.com/apache/kafka/blob/074a03cca162f91ccdecc12eb84c6a45af75f6bf/core/src/main/scala/kafka/network/SocketServer.scala#L424-L445 Therefore, I think testing Acceptor requires much more code. Instead, testing `SocketServer` is simpler. ########## File path: core/src/main/scala/kafka/network/SocketServer.scala ########## @@ -734,6 +734,10 @@ private[kafka] class Acceptor(val endPoint: EndPoint, val endThrottleTimeMs = e.startThrottleTimeMs + e.throttleTimeMs throttledSockets += DelayedCloseSocket(socketChannel, endThrottleTimeMs) None + case e: IOException => + info(s"Encounter IOException", e) + closeSocket(socketChannel) Review comment: Since the existing test cases don't have scaffold for testing `Acceptor`, we need to prepare lots of parameters for `Acceptor` constructor https://github.com/apache/kafka/blob/074a03cca162f91ccdecc12eb84c6a45af75f6bf/core/src/main/scala/kafka/network/SocketServer.scala#L550-L558 We also need to prepare lots of parameters for creating a `Processor` for the `Acceptor` https://github.com/apache/kafka/blob/074a03cca162f91ccdecc12eb84c6a45af75f6bf/core/src/main/scala/kafka/network/SocketServer.scala#L687-L703 https://github.com/apache/kafka/blob/074a03cca162f91ccdecc12eb84c6a45af75f6bf/core/src/main/scala/kafka/network/SocketServer.scala#L574-L578 https://github.com/apache/kafka/blob/074a03cca162f91ccdecc12eb84c6a45af75f6bf/core/src/main/scala/kafka/network/SocketServer.scala#L790-L806 However, those logic are implemented in `SocketServer` https://github.com/apache/kafka/blob/074a03cca162f91ccdecc12eb84c6a45af75f6bf/core/src/main/scala/kafka/network/SocketServer.scala#L268-L283 https://github.com/apache/kafka/blob/074a03cca162f91ccdecc12eb84c6a45af75f6bf/core/src/main/scala/kafka/network/SocketServer.scala#L424-L445 Therefore, I think testing Acceptor requires much more code. Instead, testing `SocketServer` is simpler. -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org