[jira] [Comment Edited] (KAFKA-5859) Avoid retaining AbstractRequest in RequestChannel.Request

["Sean Glover (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/KAFKA-5859?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16224279#comment-16224279 ] 

Sean Glover edited comment on KAFKA-5859 at 10/30/17 1:15 AM:
--------------------------------------------------------------

Hey [~ijuma].  I've been analyzing the work required for this ticket.  I'm new to Kafka, so I'm new to the codebase.  I understand that the parsed request is mostly just used in `KafkaApis.handle`, but to [~huxi_2b]'s point there are a few places in `RequestChannel.Request` that need a parsed request.

* `updateRequestMetrics` - Checks to see if the parsed request is a `FetchRequest` to access the `isFromFollower` value.  [~huxi_2b] already pointed this out.  What do you mean that "it needs the `ApiKeys` instance"?  
* `updateRequestMetrics`  - Requires the parsed size of the request in bytes to update the `requestBytesHist` metric
* `updateRequestMetrics` - Calls `requestDesc(true)` or `requestDesc(false)` depending on whether trace logging enabled.  It looks like it's used when request logging is enabled.
* `Request` constructor has a trace logging line that calls `requestDesc(true)`

I could parse the request in the `Request` constructor and generate a detailed/non-detailed request representations (equivalent to `requestTrue(true|false)`), but not keep the actual `AbstractRequest`.  What do you think?


was (Author: seglo):
Hey [~ijuma].  I've been analyzing the work required for this ticket.  I'm new to Kafka, so I'm new to the codebase.  I understand that the parsed request is mostly just used in `KafkaApis.handle`, but to [~huxi_2b]'s point there are a few places in `RequestChannel.Request` that need a parsed request.

* `updateRequestMetrics`
  * Checks to see if the parsed request is a `FetchRequest` to access the `isFromFollower` value.  [~huxi_2b] already pointed this out.  What do you mean that "it needs the `ApiKeys` instance"?  
  * Requires the parsed size of the request in bytes to update the `requestBytesHist` metric
  * Calls `requestDesc(true)` or `requestDesc(false)` depending on whether trace logging enabled.  It looks like it's used when request logging is enabled.
* `Request` constructor has a trace logging line that calls `requestDesc(true)`

I could parse the request in the `Request` constructor and generate a detailed/non-detailed request representations (equivalent to `requestTrue(true|false)`), but not keep the actual `AbstractRequest`.  What do you think?

> Avoid retaining AbstractRequest in RequestChannel.Request
> ---------------------------------------------------------
>
>                 Key: KAFKA-5859
>                 URL: https://issues.apache.org/jira/browse/KAFKA-5859
>             Project: Kafka
>          Issue Type: Improvement
>            Reporter: Ismael Juma
>            Assignee: Sean Glover
>            Priority: Minor
>              Labels: newbie
>
> We currently store AbstractRequest in RequestChannel.Request.bodyAndSize. RequestChannel.Request is, in turn, stored in RequestChannel.Response. We keep the latter until the response is sent to the client.
> However, after KafkaApis.handle, we no longer need AbstractRequest apart from its string representation for logging. We could potentially replace AbstractRequest with a String representation (if the relevant logging is enabled). The String representation is generally small while some AbstractRequest subclasses can be pretty large. The largest one is ProduceRequest and we clear the underlying ByteBuffer explicitly in KafkaApis.handleProduceRequest. We could potentially remove that special case if AbstractRequest subclasses were not retained.
> This was originally suggested by [~hachikuji] in the following PR https://github.com/apache/kafka/pull/3801#discussion_r137592277



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)