AndrewJSchofield commented on code in PR #23317:
URL: https://github.com/apache/kafka/pull/23317#discussion_r3967611122
##########
generator/src/main/java/org/apache/kafka/message/MessageSpec.java:
##########
@@ -109,6 +129,26 @@ public MessageSpec(@JsonProperty("name") String name,
}
}
+ /**
+ * ApiVersionsResponse must use a v0 response header at every version so
that older brokers can always
+ * parse the response header (KIP-511). Checking that the header versions
implied by flexibleVersions are
Review Comment:
I would move the second sentence from this comment. It could easily persist
beyond the anticipated follow-up work, so probably best not to include such a
comment.
##########
generator/src/main/java/org/apache/kafka/message/MessageSpec.java:
##########
@@ -64,6 +71,11 @@ public MessageSpec(@JsonProperty("name") String name,
this.flexibleVersions = Versions.NONE;
this.listeners = List.of();
this.latestVersionUnstable = false;
+ if (headerVersions != null) {
Review Comment:
It seems to me that all of the other properties are just ignored for a
message with no valid versions. You could do exactly the same here.
##########
generator/src/main/java/org/apache/kafka/message/ApiMessageTypeGenerator.java:
##########
@@ -396,6 +401,28 @@ private void generateHeaderVersion(String type) {
buffer.printf("}%n");
}
+ private void generateHeaderVersionFromMap(HeaderVersions headerVersions) {
+ List<HeaderVersions.Entry> entries = headerVersions.entries();
Review Comment:
This generates subtly different code than previously for the case where the
header versions maps describe versions which are no longer valid. You should
clamp the generated code to the valid versions I think. Here's what I mean.
before:
```
case 27: // WriteTxnMarkers
return (short) 2;
```
after:
```
case 27: // WriteTxnMarkers
if (_version >= 1) {
return (short) 2;
} else {
return (short) 1;
}
```
Versions 1-2 are valid, so even though the schema describes v0, the
generated code need not support it, and did not prior to this PR.
##########
generator/src/main/java/org/apache/kafka/message/MessageSpec.java:
##########
@@ -89,6 +101,14 @@ public MessageSpec(@JsonProperty("name") String name,
}
this.latestVersionUnstable = latestVersionUnstable;
+ if (headerVersions != null && type != MessageSpecType.REQUEST &&
type != MessageSpecType.RESPONSE) {
+ throw new RuntimeException("The `headerVersions` property is
only valid for " +
+ "messages with type `request` or `response`");
+ }
+ this.headerVersions = Optional.ofNullable(
+ HeaderVersions.parse(name, headerVersions,
this.validVersions()));
+ checkApiVersionsResponseHeaderVersion();
Review Comment:
I think it would also be worth asserting that the flexible versions and
header versions align. Essentially, for a request, flexible version implies
header version is >=2, and for a response, flexible version implies header
version >= 1.
--
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]
