[
https://issues.apache.org/jira/browse/KAFKA-5859?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16252935#comment-16252935
]
ASF GitHub Bot commented on KAFKA-5859:
---------------------------------------
GitHub user seglo opened a pull request:
https://github.com/apache/kafka/pull/4216
KAFKA-5859: Avoid retaining AbstractRequest in RequestChannel.Response
This PR removes the need to keep a reference to the parsed
`AbstractRequest` after it's been handled in `KafkaApis`. A reference to
`RequestAndSize` which holds the parsed `AbstractRequest` in
`RequestChannel.Request` was kept in scope as a consequence of being passed
into the `RequestChannel.Response` after being handled.
The Jira ticket
[KAFKA-5859](https://issues.apache.org/jira/browse/KAFKA-5859) suggests
removing this reference as soon as it's no longer needed. I considered several
implementations and I settled on creating a new type that contains all the
relevant information of the Request that is required after it has been handled.
I think this approach allows for the least amount of invasive changes in the
Request/Response lifecycle while retaining the immutability of the
`RequestChannel.Request`.
A new type called `RequestChannel.RequestSummary` now contains much of the
information that was in `RequestChannel.Request` before. The
`RequestChannel.Request` now generates a `RequestChannel.RequestSummary` that
is passed into the `RequestChannel.Response` after being handled in
`KafkaApis`. `RequestChannel.RequestSummary` contains information such as:
* A detailed and non-detailed description of the request
* Metrics associated with the request
* Helper methods to update various Request metrics
* A special case describing whether or not the original Request was a
`FetchRequest` and whether it was from a follower. This information is
required in the `updateRequestMetrics` metrics helper method.
This change does not make any behaviour changes so no additional tests were
added. I've verified that all unit and integration tests pass and no
regressions were introduced. I'm interested in seeing the before and after
results of the Confluent Kafka system tests as described in step 11 of the
[Contributing Code
Changes](https://cwiki.apache.org/confluence/display/KAFKA/Contributing+Code+Changes#ContributingCodeChanges-PullRequest)
section. I would like to request access to kick off this system tests suite
if you agree that it's relevant to this ticket.
This is my first contribution to this project. I picked up this issue
because it was marked with the newbie flag and it seemed like a good
opportunity to learn more about about the request and response lifecycle in the
Kafka broker.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/seglo/kafka to-request-summary-5859
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/kafka/pull/4216.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #4216
----
commit 9bd67ea7cf16077e20db8e1a87330176eb3772de
Author: seglo <se...@randonom.com>
Date: 2017-11-12T02:42:55Z
Use RequestSummary in RequestChannel.Response
----
> 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)
