junrao commented on code in PR #23845:
URL: https://github.com/apache/kafka/pull/23845#discussion_r4327493913
##########
clients/src/main/resources/common/message/GetTelemetrySubscriptionsRequest.json:
##########
@@ -27,7 +27,7 @@
"1+": "3"
},
"fields": [
- { "name": "ClientInstanceId", "type": "uuid", "versions": "0",
+ { "name": "ClientInstanceId", "type": "uuid", "versions": "0",
"ignorable": true,
Review Comment:
The change itself seems safe. However, it is confusing to change the
definition of an existing field since a developer implementing the protocol
won't know which definition to follow. We have removed non-ignorable fields
before like replicaId in FetchRequest below. The way this is handled is to
customize the setting of the removed field based on the version in the builder,
same as what you did to GetTelemetrySubscriptions in trunk. So, I think it's
better to keep this convention and not change the existing field to ignorable.
```
if (version < 15) {
fetchRequestData.setReplicaId(replicaId); // old field,
versions 0-14
} else {
fetchRequestData.setReplicaState(new ReplicaState() // new
location from v15
.setReplicaId(replicaId)
.setReplicaEpoch(replicaEpoch));
}
```
--
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]
