cmccabe commented on PR #11969:
URL: https://github.com/apache/kafka/pull/11969#issuecomment-1119953102
I made two changes just now:
1. Fixed it so TenantPartitionAssignor ignores fenced brokers, and added a
unit test for this (as we discussed offline)
2. Removed
ClusterMetadataTest#testNodeReplicaCountsForExistingTenantTopicWithInvalidAssignment
and replaced it with
ClusterMetadataTest#testNodeReplicaCountsForUnregisteredBrokers. Basically, the
assumptions of the old test were invalid (it is actually possible to have
partitions placed on brokers that are no longer in the cluster).
Looking at the code, I see that it already took this into account when
computing the nodeMetadata:
```
String tenantPrefix = tenant + TenantContext.DELIMITER;
for (Iterator<String> iterator = cluster.topicNames();
iterator.hasNext();) {
String topic = iterator.next();
boolean isTenantTopic = topic.startsWith(tenantPrefix);
for (List<Integer> replicas : cluster.replicasForTopicName(topic)) {
for (int i = 0; i < replicas.size(); i++) {
Integer replica = replicas.get(i);
NodeMetadata nodeMetadata = nodeMetadatas.computeIfAbsent(replica,
node -> new NodeMetadata());
nodeMetadata.incrementReplicas(i == 0, isTenantTopic);
}
}
}
```
So the code was already handling this case smoothly, which is what we want.
But the very artificial unit test (injecting a null into the replicas array,
which wouldn't happen in a real Cluster object) was confusing the issue.
Hopefully this clears up the issue.
--
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]
