junrao commented on code in PR #23665:
URL: https://github.com/apache/kafka/pull/23665#discussion_r4330765154
##########
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()));
Review Comment:
@AndrewJSchofield : Good point. Claude suggested the following way to
tighten this up.
```
RequestContext is the only production caller of toResponseHeader() (:159,
:173). Compute both the body version and the header version from the same
supported version:
public Send buildResponseSend(AbstractResponse body) {
short version = apiVersion();
return body.toSend(responseHeader(version), version);
}
public ByteBuffer buildResponseEnvelopePayload(AbstractResponse body) {
short version = apiVersion();
return body.serializeWithHeader(responseHeader(version), version);
}
/**
* The response header follows the version the response is written at, which
for an unsupported
* ApiVersionsRequest is v0 rather than the version of the request.
*/
private ResponseHeader responseHeader(short responseVersion) {
return new ResponseHeader(header.correlationId(),
header.apiKey().responseHeaderVersion(responseVersion));
}
```
As a related issue, currently, KafkaApis uses 4 different ways to obtain the
api version. They all agree except for unsupport ApiVersion.
```
Accessor | Returns | Why
-- | -- | --
request.header.apiVersion | N (6) | data.requestApiVersion(), the version in
the request header
request.context.apiVersion() | 0 | isUnsupportedApiVersionsRequest() is
true, so it returns 0, the version the response is written at
(RequestContext.java:180-185)
request.context.requestVersion() | N (6) | header.apiVersion(), returned as
an int (RequestContext.java:217-219)
request.body[ApiVersionsRequest].version | 0 | parseRequest builds new
ApiVersionsRequest(new ApiVersionsRequestData(), (short) 0,
header.apiVersion()) (RequestContext.java:131), so the body is a v0 request and
its body isn't parsed
```
It would be useful to document what each api means and when to use which.
We probably can change request.context.apiVersion() to sth like
request.context.effectiveApiVersion() and document it that this is the version
parsed by the server. KafkaApis should typically use this or
request.body[ApiVersionsRequest].version to get api version.
Document that request.context.requestVersion() is the way for getting the
original Api Version sent by the client. This should only be used in the
following in KafkaApis where the original version is needed.
`if (!apiVersionManager.isApiEnabled(request.header.apiKey,
request.header.apiVersion))
`
--
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]
