[
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)
