chia7712 commented on code in PR #23665:
URL: https://github.com/apache/kafka/pull/23665#discussion_r4312172166
##########
clients/src/main/resources/common/message/RequestHeader.json:
##########
@@ -40,6 +40,17 @@
// ApiVersionsRequest, even if it is from a newer version.
// Since the client is sending the ApiVersionsRequest in order to discover
what
// versions are supported, the client does not know the best version to
use.
+ //
+ // Currently, we can't add new non-tagged fields to the request header for
ApiVersionsRequest.
Review Comment:
This constraint is about the whole header rather than `ClientId`. I’d move
it up to the version history comment above `validVersions`, where the next
header version will be added.
```
// Version 3 introduces ClientInstanceId. (KIP-1313)
//
// Compatibility constraints:
// - New header fields must be tagged until every broker version that
clients are required to
// support (see KIP-896) includes the fix from KAFKA-21160 (first
shipped in 4.5.0).
// - The first four fields (RequestApiKey, RequestApiVersion,
CorrelationId, ClientId) must keep
// their order and encoding across all versions.
// - Version 1 must not be removed. Brokers use it to parse headers with
an unsupported API version.
// See RequestHeader#parse for details.
"validVersions": "1-3",
```
##########
clients/src/main/java/org/apache/kafka/common/requests/RequestHeader.java:
##########
@@ -137,11 +137,27 @@ public ResponseHeader toResponseHeader() {
return new ResponseHeader(data.correlationId(),
apiKey().responseHeaderVersion(apiVersion()));
}
+ /**
+ * Parses a request header from the given buffer. For a supported request
API version, the buffer is positioned
+ * at the start of the request body on return.
+ * <p>
+ * If the request API version is not supported by this node, the
corresponding header version may also be unknown.
+ * In this case, the header is parsed using version 1, which contains only
the fields common to all header versions:
+ * API key, API version, correlation ID, and client ID. Any remaining
header bytes, including tagged fields,
+ * are left unread. The returned header has {@link
#isApiVersionSupported()} set to false, and the caller is
+ * expected to check it before parsing the rest of the bytes in the
request, since the buffer is not positioned
+ * at the start of the request body.
+ *
+ * @param buffer the buffer containing the request header
+ * @return the parsed request header
+ * @throws InvalidRequestException if the api key is unknown or the header
cannot be parsed
+ */
public static RequestHeader parse(ByteBuffer buffer) {
short apiKeyId = -1;
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.
+ // If the api version is unsupported, the header is parsed with
header version 1
Review Comment:
Could we move this comment down next to headerVersion?
##########
clients/src/main/java/org/apache/kafka/common/requests/RequestHeader.java:
##########
@@ -152,7 +168,7 @@ public static RequestHeader parse(ByteBuffer buffer) {
if (!apiKey.hasValidVersion())
throw new InvalidRequestException("Unsupported api with key "
+ apiKeyId + " (" + apiKey.name + ") and version " + apiVersion);
- short headerVersion = apiKey.requestHeaderVersion(apiVersion);
+ short headerVersion = apiKey.isVersionSupported(apiVersion) ?
apiKey.requestHeaderVersion(apiVersion) : (short) 1;
Review Comment:
Should we hardcode the parsing of the four common fields instead of relying
on the generated code? What we need is “read only the first four fields”, and
header v1 only happens to match that today.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
