rondagostino commented on code in PR #14290:
URL: https://github.com/apache/kafka/pull/14290#discussion_r1367993770
##########
metadata/src/main/resources/common/metadata/PartitionRecord.json:
##########
@@ -47,6 +47,13 @@
"about": "The eligible leader replicas of this partition." },
{ "name": "LastKnownELR", "type": "[]int32", "default": "null",
"entityType": "brokerId",
"versions": "1+", "nullableVersions": "1+", "taggedVersions": "1+",
"tag": 2,
- "about": "The last known eligible leader replicas of this partition." }
+ "about": "The last known eligible leader replicas of this partition." },
+ { "name": "Assignment", "type": "[]ReplicaAssignment", "versions": "2+",
+ "about": "The replica assignment for this partition, sorted by preferred
order.", "fields": [
+ { "name": "Broker", "type": "int32", "versions": "2+", "entityType":
"brokerId",
+ "about": "The broker ID hosting the replica." },
+ { "name": "Directory", "type": "uuid", "versions": "2+",
+ "about": "The log directory hosting the replica" }
+ ]}
Review Comment:
The `Assignment` field added to `PartitionChangeRecord` is added as a tagged
field, yet the `Assignment` field added here in `PartitionRecord` is not. I
think they should both be tagged fields?
##########
server-common/src/main/java/org/apache/kafka/server/common/MetadataVersion.java:
##########
@@ -344,7 +348,9 @@ public boolean isControllerRegistrationSupported() {
}
public short partitionChangeRecordVersion() {
- if (isElrSupported()) {
+ if (isDirectoryAssignmentSupported()) {
+ return (short) 2;
+ } else if (isElrSupported()) {
Review Comment:
We are going to need to do the same thing in `partitionRecordVersion()` and
`registerBrokerRecordVersion()`. There is no
`brokerRegistrationChangeRecordVersion()`, and it appears we don't need that
yet since we explicitly send either version 0 or version 1.
##########
metadata/src/main/resources/common/metadata/PartitionRecord.json:
##########
@@ -17,9 +17,9 @@
"apiKey": 3,
"type": "metadata",
"name": "PartitionRecord",
- "validVersions": "0-1",
- // Version 1 implements Eligiable Leader Replicas and LastKnownELR as
described in KIP-966.
- //
+ // Version 1 replaces Replicas with Assignment
+ // Version 2 implements Eligiable Leader Replicas and LastKnownELR as
described in KIP-966.
+ "validVersions": "0-2",
Review Comment:
These comments are backwards: Version 1 is ELR, Version 2 JBOD.
Also, we are adding Assignment, but we are not actually using it yet, so I
would be careful about stating "replaces Replicas with Assignment". I would
consider stating `Version 2 adds Assignment as a future replacement for
Replicas once enabled via MetadaatVersion.isDirectoryAssignmentSupported()`
--
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
