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

["ASF GitHub Bot (JIRA)" <ji...@apache.org>]

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