rondagostino commented on code in PR #14290:
URL: https://github.com/apache/kafka/pull/14290#discussion_r1368928056
##########
metadata/src/main/resources/common/metadata/BrokerRegistrationChangeRecord.json:
##########
@@ -13,11 +13,12 @@
// See the License for the specific language governing permissions and
// limitations under the License.
+// Version 2 adds LogDirs
Review Comment:
Same here, can we add the missing comment `Version 1 adds
InControlledShutdown`?
##########
metadata/src/main/java/org/apache/kafka/controller/PartitionChangeBuilder.java:
##########
@@ -54,6 +54,7 @@ public static boolean
changeRecordIsNoOp(PartitionChangeRecord record) {
if (record.removingReplicas() != null) return false;
if (record.addingReplicas() != null) return false;
if (record.leaderRecoveryState() != LeaderRecoveryState.NO_CHANGE)
return false;
+ if (record.assignment() != null) return false;
Review Comment:
The `org.apache.kafka.metadata.PartitionRegistration` class has a `merge()`
method that will need some adjusting as well.
##########
metadata/src/main/resources/common/metadata/RegisterBrokerRecord.json:
##########
@@ -13,11 +13,12 @@
// See the License for the specific language governing permissions and
// limitations under the License.
+// Version 3 adds LogDirs
Review Comment:
Can we do a little cleanup by adding missing comments?
```
Version 1 adds InControlledShutdown
Version 2 adds IsMigratingZkBroker
```
##########
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:
Fair. I'm wondering, though, if maybe we want to abandon this `Assignment`
of type `[]ReplicaAssignment` in favor of just an array of UUIDs. @cmccabe had
originally expressed a preference for that in the VOTE thread:
```
I would prefer a new (tagged) array for replica UUIDs,
rather than creating the ReplicaAssignment array.
```
I had disagreed, and you had concurred with me, but that was based on an
assumption that no longer holds since we now always specify the log directory
UUID regardless of whether multiple log directories are in use or not:
```
Colin had mentioned that in PartitionRecord he would prefer a new
(tagged) array for replica UUIDs, rather than creating the
ReplicaAssignment array. While adding to an RPC is arguably less
intrusive than replacing, I am inclined to disagree with Colin's
suggestion for the following reason. We have agreed to not specify
the log dir uuid in the case where a replica only has one registered
log dir. If we were to add a new tagged array, it would become
necessary to specify all replica log dir uuids for all replicas in the
PartitionRecord if any one such replica had more than one log dir
configured. By creating the ReplicaAssignment array we can just
specify the uuid -- or not -- for each replica based on whether that
replica itself has multiple registered log dirs or not.
...
It is a bit of a pain to replace the field, but I agree that is
the best approach for the same reason you pointed out.
```
The ELR fields were just added as tagged fields on both records.
I also feel a bit uncomfortable with the `Replicas` field existing when it
would not be used. Granted that it isn't used very much -- just 1 place -- but
still, I worry about forcing any future uses to check to see if there are
directory assignments and grab the replicas from there if they exist, otherwise
grab them from the old location.
I thought perhaps we had discussed this in our Zoom meeting last week, but I
could be wrong.
WDYT?
--
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
