KAFKA-5976: RequestChannel.sendResponse should record correct size for NetworkSend. #3961
Conversation
…r NetworkSend. When TRACE logging is enabled, RequestChannel.sendResponse records incorrect size for `Send` due to the fact that `response.responseSend` is of scala.Option type.
|
@ijuma Please review this tiny patch. Thanks. |
| @@ -247,7 +247,7 @@ class RequestChannel(val numProcessors: Int, val queueSize: Int) extends KafkaMe | |||
| if (isTraceEnabled) { | |||
| val requestHeader = response.request.header | |||
| trace(s"Sending ${requestHeader.apiKey} response to client ${requestHeader.clientId} of " + | |||
| s"${response.responseSend.size} bytes.") | |||
| s"${response.responseSend.get.size} bytes.") | |||
There was a problem hiding this comment.
Nice catch. However, this could be None, so we should probably log something different if responseAction is NoOp or Close.
There was a problem hiding this comment.
Already added code to handle such situation. Please review.
| case Some(send) => send.size | ||
| case None => 0 | ||
| } | ||
| trace(s"Sending ${requestHeader.apiKey} response[action=${response.responseAction}, size=$size] to client") |
There was a problem hiding this comment.
But we are not necessarily sending a response. I was thinking of something like:
response.responseAction match {
case SendAction => // like now
case NoOpAction => // log that no response is being sent
case CloseConnectionAction => // log that the connection is being closedDoes that make sense?
There was a problem hiding this comment.
Agreed. It's clear to do trace logs for different response actions.
| trace(s"No need to send ${requestHeader.apiKey} response to client ${requestHeader.clientId}.") | ||
| case CloseConnectionAction => | ||
| trace("The connection is being closed since request handler encountered an error.") | ||
| case _ => // it should not happen |
There was a problem hiding this comment.
not clear why you shadow impossible case? I would remove it or throw something like IllegalArgurmentException
There was a problem hiding this comment.
Yes, we should remove it and make ResponseAction sealed.
| trace(s"No need to send ${requestHeader.apiKey} response to client ${requestHeader.clientId}.") | ||
| case CloseConnectionAction => | ||
| trace("The connection is being closed since request handler encountered an error.") | ||
| case _ => // it should not happen |
There was a problem hiding this comment.
val message = response.responseAction match ...
trace(message)
There was a problem hiding this comment.
That would compute the message even if trace logging is not enabled.
There was a problem hiding this comment.
not clear how it possible, it with in if (isTraceEnabled) scope.
There was a problem hiding this comment.
My bad, I had missed the if. In that case, your suggestion sounds good.
There was a problem hiding this comment.
Thanks, LGTM. I tweaked the message a little:
case SendAction =>
s"Sending ${requestHeader.apiKey} response to client ${requestHeader.clientId} of ${response.responseSend.get.size} bytes."
case NoOpAction =>
s"Not sending ${requestHeader.apiKey} response to client ${requestHeader.clientId} as it's not required."
case CloseConnectionAction =>
s"Closing connection for client ${requestHeader.clientId} due to error during handling of ${requestHeader.apiKey}."
When TRACE logging is enabled, RequestChannel.sendResponse records incorrect size for
Senddue to the fact thatresponse.responseSendis of scala.Option type.