chia7712 commented on code in PR #18497:
URL: https://github.com/apache/kafka/pull/18497#discussion_r1922376442
##########
generator/src/main/java/org/apache/kafka/message/ApiMessageTypeGenerator.java:
##########
@@ -355,28 +362,9 @@ private void generateHeaderVersion(String type) {
buffer.decrementIndent();
continue;
}
- if (type.equals("request") && apiKey == 7) {
- buffer.printf("// Version 0 of ControlledShutdownRequest has a
non-standard request header%n");
- buffer.printf("// which does not include clientId. Version 1
of ControlledShutdownRequest%n");
- buffer.printf("// and later use the standard request
header.%n");
- buffer.printf("if (_version == 0) {%n");
- buffer.incrementIndent();
- buffer.printf("return (short) 0;%n");
- buffer.decrementIndent();
- buffer.printf("}%n");
- }
- ApiData data = entry.getValue();
- MessageSpec spec;
- if (type.equals("request")) {
- spec = data.requestSpec;
- } else if (type.equals("response")) {
- spec = data.responseSpec;
- } else {
- throw new RuntimeException("Invalid type " + type + " for
generateHeaderVersion");
- }
- if (spec == null) {
- throw new RuntimeException("failed to find " + type + " for
API key " + apiKey);
- }
+ MessageSpec spec = messageSpec(type, apiKey, entry.getValue());
+ if (!spec.hasValidVersion())
+ continue;
Review Comment:
It seems we need to add `buffer.decrementIndent()` to avoid generating
following code.
```java
case 4: // LeaderAndIsr
case 5: // StopReplica
case 6: // UpdateMetadata
case 7: // ControlledShutdown
case 8: // OffsetCommit
```
##########
generator/src/main/java/org/apache/kafka/message/ApiMessageTypeGenerator.java:
##########
@@ -355,28 +362,9 @@ private void generateHeaderVersion(String type) {
buffer.decrementIndent();
continue;
}
- if (type.equals("request") && apiKey == 7) {
- buffer.printf("// Version 0 of ControlledShutdownRequest has a
non-standard request header%n");
- buffer.printf("// which does not include clientId. Version 1
of ControlledShutdownRequest%n");
- buffer.printf("// and later use the standard request
header.%n");
- buffer.printf("if (_version == 0) {%n");
- buffer.incrementIndent();
- buffer.printf("return (short) 0;%n");
- buffer.decrementIndent();
- buffer.printf("}%n");
- }
- ApiData data = entry.getValue();
- MessageSpec spec;
- if (type.equals("request")) {
- spec = data.requestSpec;
- } else if (type.equals("response")) {
- spec = data.responseSpec;
- } else {
- throw new RuntimeException("Invalid type " + type + " for
generateHeaderVersion");
- }
- if (spec == null) {
- throw new RuntimeException("failed to find " + type + " for
API key " + apiKey);
- }
+ MessageSpec spec = messageSpec(type, apiKey, entry.getValue());
+ if (!spec.hasValidVersion())
+ continue;
Review Comment:
Or maybe we should skip those api kips to throw
`UnsupportedVersionException` directly?
##########
clients/src/main/resources/common/message/LeaderAndIsrRequest.json:
##########
@@ -16,84 +16,6 @@
{
"apiKey": 4,
"type": "request",
- "listeners": ["zkBroker"],
"name": "LeaderAndIsrRequest",
- // Version 1 adds IsNew.
- //
- // Version 2 adds broker epoch and reorganizes the partitions by topic.
- //
- // Version 3 adds AddingReplicas and RemovingReplicas.
- //
- // Version 4 is the first flexible version.
- //
- // Version 5 adds Topic ID and Type to the TopicStates, as described in
KIP-516.
- //
- // Version 6 adds LeaderRecoveryState as described in KIP-704.
- //
- // Version 7 adds KRaft Controller ID field as part of KIP-866
- "validVersions": "0-7",
- "flexibleVersions": "4+",
- "fields": [
- { "name": "ControllerId", "type": "int32", "versions": "0+", "entityType":
"brokerId",
- "about": "The current controller ID." },
- { "name": "isKRaftController", "type": "bool", "versions": "7+",
"default": "false",
- "about": "If KRaft controller id is used during migration. See KIP-866."
},
- { "name": "ControllerEpoch", "type": "int32", "versions": "0+",
- "about": "The current controller epoch." },
- { "name": "BrokerEpoch", "type": "int64", "versions": "2+", "ignorable":
true, "default": "-1",
- "about": "The current broker epoch." },
- { "name": "Type", "type": "int8", "versions": "5+",
- "about": "The type that indicates whether all topics are included in the
request."},
- { "name": "UngroupedPartitionStates", "type":
"[]LeaderAndIsrPartitionState", "versions": "0-1",
- "about": "The state of each partition, in a v0 or v1 message." },
- // In v0 or v1 requests, each partition is listed alongside its topic name.
- // In v2+ requests, partitions are organized by topic, so that each topic
name
- // only needs to be listed once.
- { "name": "TopicStates", "type": "[]LeaderAndIsrTopicState", "versions":
"2+",
- "about": "Each topic.", "fields": [
- { "name": "TopicName", "type": "string", "versions": "2+", "entityType":
"topicName",
- "about": "The topic name." },
- { "name": "TopicId", "type": "uuid", "versions": "5+", "ignorable": true,
- "about": "The unique topic ID." },
- { "name": "PartitionStates", "type": "[]LeaderAndIsrPartitionState",
"versions": "2+",
- "about": "The state of each partition." }
- ]},
- { "name": "LiveLeaders", "type": "[]LeaderAndIsrLiveLeader", "versions":
"0+",
- "about": "The current live leaders.", "fields": [
- { "name": "BrokerId", "type": "int32", "versions": "0+", "entityType":
"brokerId",
- "about": "The leader's broker ID." },
- { "name": "HostName", "type": "string", "versions": "0+",
- "about": "The leader's hostname." },
- { "name": "Port", "type": "int32", "versions": "0+",
- "about": "The leader's port." }
- ]}
- ],
- "commonStructs": [
- { "name": "LeaderAndIsrPartitionState", "versions": "0+", "fields": [
- { "name": "TopicName", "type": "string", "versions": "0-1",
"entityType": "topicName", "ignorable": true,
- "about": "The topic name. This is only present in v0 or v1." },
- { "name": "PartitionIndex", "type": "int32", "versions": "0+",
- "about": "The partition index." },
- { "name": "ControllerEpoch", "type": "int32", "versions": "0+",
- "about": "The controller epoch." },
- { "name": "Leader", "type": "int32", "versions": "0+", "entityType":
"brokerId",
- "about": "The broker ID of the leader." },
- { "name": "LeaderEpoch", "type": "int32", "versions": "0+",
- "about": "The leader epoch." },
- { "name": "Isr", "type": "[]int32", "versions": "0+", "entityType":
"brokerId",
- "about": "The in-sync replica IDs." },
- { "name": "PartitionEpoch", "type": "int32", "versions": "0+",
- "about": "The current epoch for the partition. The epoch is a
monotonically increasing value which is incremented after every partition
change. (Since the LeaderAndIsr request is only used by the legacy controller,
this corresponds to the zkVersion)." },
- { "name": "Replicas", "type": "[]int32", "versions": "0+", "entityType":
"brokerId",
- "about": "The replica IDs." },
- { "name": "AddingReplicas", "type": "[]int32", "versions": "3+",
"ignorable": true, "entityType": "brokerId",
- "about": "The replica IDs that we are adding this partition to, or
null if no replicas are being added." },
- { "name": "RemovingReplicas", "type": "[]int32", "versions": "3+",
"ignorable": true, "entityType": "brokerId",
- "about": "The replica IDs that we are removing this partition from, or
null if no replicas are being removed." },
- { "name": "IsNew", "type": "bool", "versions": "1+", "default": "false",
"ignorable": true,
- "about": "Whether the replica should have existed on the broker or
not." },
- { "name": "LeaderRecoveryState", "type": "int8", "versions": "6+",
"default": "0",
- "about": "1 if the partition is recovering from an unclean leader
election; 0 otherwise." }
- ]}
- ]
+ "validVersions": "none"
Review Comment:
Maybe we can add "This request was removed in Apache Kafka 4.0." to this
file?
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
