soarez commented on code in PR #14290:
URL: https://github.com/apache/kafka/pull/14290#discussion_r1371390386
##########
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:
It's a definitely a good shout – some of the assumptions have changed since
then. I agree it's attractive to add a new orthogonal dir UUIDs field, that'll
be an easier change to handle. And I'm also uncomfortable with not taking any
action to deprecate the existing `Replicas` field.
My concerns are:
* It will be possible to construct a record that is semantically invalid,
with a different number of entries in `record.replicas` and `record.logDirs`.
* We'll need extra care and it might be a bit awkward to keep the order of
both lists in sync, any failures there will lead to issues that might be hard
to detect or diagnose, as there won't be any hard link between both values.
* This problem stems mainly from our recent decision to postpone figuring
out how to burn field tags. My understanding is that keeping the Replicas field
unchanged is a temporary measure until then. Whereas in the long term every
replica should always have log dir. In other words, I don't think we'd consider
this if `Replicas` didn't already exist, so it feels like we're sacrificing the
end design due to the existing limitation on tagged fields.
##########
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:
It's a definitely a good shout – some of the assumptions have changed since
then. I agree it's attractive to add a new orthogonal dir UUIDs field, that'll
be an easier change to handle. And I'm also uncomfortable with not taking any
action to deprecate the existing `Replicas` field.
My concerns are:
* It will be possible to construct a record that is semantically invalid,
with a different number of entries in `record.replicas` and `record.logDirs`.
* We'll need extra care and it might be a bit awkward to keep the order of
both lists in sync, any failures there will lead to issues that might be hard
to detect or diagnose, as there won't be any hard link between both values.
* This problem stems mainly from our recent decision to postpone figuring
out how to burn field tags. My understanding is that keeping the Replicas field
unchanged is a temporary measure until then. Whereas in the long term every
replica should always have log dir. In other words, I don't think we'd consider
this if `Replicas` didn't already exist, so it feels like we're sacrificing the
end design due to the existing limitation on tagged fields.
--
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
