[GitHub] [kafka] divijvaidya opened a new pull request, #12917: KAFKA-14414: Modify approach to calculate size of request header

[GitBox <gi...@apache.org>]

divijvaidya opened a new pull request, #12917:
URL: https://github.com/apache/kafka/pull/12917

   https://github.com/apache/kafka/pull/12890 introduced the notion of calculating (and caching) the size for the request header during the first parse operation. This prevents re-calculation and hence, improves CPU usage for produce workload. 
   
   However, @showuon found a bug in calculation of size in that PR mentioned at https://github.com/apache/kafka/pull/12916
   
   This PR modifies the (incorrect!) approach we took earlier to calculate the header size and replaces it with another approach while still avoiding re-calculation of request header size.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] showuon commented on a diff in pull request #12917: KAFKA-14414: Modify approach to calculate size of request header

[GitBox <gi...@apache.org>]

showuon commented on code in PR #12917:
URL: https://github.com/apache/kafka/pull/12917#discussion_r1035539675


##########
clients/src/main/java/org/apache/kafka/common/requests/RequestHeader.java:
##########
@@ -117,23 +117,23 @@ public static RequestHeader parse(ByteBuffer buffer) {
         try {
             // We derive the header version from the request api version, so we read that first.
             // The request api version is part of `RequestHeaderData`, so we reset the buffer position after the read.
-            int position = buffer.position();
-            int requestHeaderSize = buffer.remaining();
+            int bufferStartPositionForHeader = buffer.position();
             apiKey = buffer.getShort();
             short apiVersion = buffer.getShort();
             short headerVersion = ApiKeys.forId(apiKey).requestHeaderVersion(apiVersion);
-            buffer.position(position);
-            RequestHeaderData headerData = new RequestHeaderData(new ByteBufferAccessor(buffer), headerVersion);
+            buffer.position(bufferStartPositionForHeader);
+            final RequestHeaderData headerData = new RequestHeaderData(new ByteBufferAccessor(buffer), headerVersion);
             // Due to a quirk in the protocol, client ID is marked as nullable.
             // However, we treat a null client ID as equivalent to an empty client ID.
             if (headerData.clientId() == null) {
                 headerData.setClientId("");
             }
             final RequestHeader header = new RequestHeader(headerData, headerVersion);
-            // Size of a buffer required to serialize the information in this header is already known and would not
-            // change since the RequestHeader object is immutable. Instead of computing it again whenever
-            // RequestHeader#size() is called, we choose to cache the size value when available.
-            header.size = requestHeaderSize;
+            // Size of header is calculated by the shift in the position of buffer's start position during parsing.
+            // Prior to parsing, the buffer's start position points to header data and after the parsing operation
+            // the buffer's start position points to api message. For more information on how the buffer is
+            // constructed, see RequestUtils#serialize()
+            header.size = Math.max(buffer.position() - bufferStartPositionForHeader, 0);

Review Comment:
   Nice improvement! Thanks.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] mimaison merged pull request #12917: KAFKA-14414: Modify approach to calculate size of request header

[GitBox <gi...@apache.org>]

mimaison merged PR #12917:
URL: https://github.com/apache/kafka/pull/12917


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org