AndrewJSchofield commented on code in PR #23651:
URL: https://github.com/apache/kafka/pull/23651#discussion_r4163681018
##########
clients/src/main/java/org/apache/kafka/common/requests/GetTelemetrySubscriptionsRequest.java:
##########
@@ -40,6 +41,11 @@ public Builder(GetTelemetrySubscriptionsRequestData data,
boolean enableUnstable
@Override
public GetTelemetrySubscriptionsRequest build(short version) {
+ // From v1 the client instance ID is carried by the request header
(KIP-1313) and the body
+ // field only exists in v0, so a non-zero value must not reach the
writer.
+ if (version >= 1 &&
!Uuid.ZERO_UUID.equals(data.clientInstanceId())) {
Review Comment:
Hmm. So, the data did contain a non-zero value and we were going to send a
v1 request, wouldn't that mean there was a bug somewhere? I wonder why this
extra piece of code was necessary.
##########
clients/src/main/resources/common/message/GetTelemetrySubscriptionsRequest.json:
##########
@@ -18,14 +18,17 @@
"type": "request",
"listeners": ["broker"],
"name": "GetTelemetrySubscriptionsRequest",
- "validVersions": "0",
+ // Version 1 removes ClientInstanceId and uses the v3 request header
(KIP-1313).
+ "latestVersionUnstable": true,
+ "validVersions": "0-1",
"flexibleVersions": "0+",
"headerVersions": {
- "0+": "2"
+ "0": "2",
+ "1+": "3"
},
"fields": [
{
- "name": "ClientInstanceId", "type": "uuid", "versions": "0+",
Review Comment:
nit: These 4 request/response JSON schema are not formatted consistently
with all the other schemas. Could you bring them in line please?
##########
clients/src/main/java/org/apache/kafka/common/requests/PushTelemetryRequest.java:
##########
@@ -45,6 +46,11 @@ public Builder(PushTelemetryRequestData data, boolean
enableUnstableLastVersion)
@Override
public PushTelemetryRequest build(short version) {
+ // From v1 the client instance ID is carried by the request header
(KIP-1313) and the body
+ // field only exists in v0, so a non-zero value must not reach the
writer.
+ if (version >= 1 &&
!Uuid.ZERO_UUID.equals(data.clientInstanceId())) {
Review Comment:
And that same as the last comment. I wonder why this is required.
--
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]
