dajac commented on code in PR #12181:
URL: https://github.com/apache/kafka/pull/12181#discussion_r893138063
##########
core/src/main/scala/kafka/server/AlterPartitionManager.scala:
##########
@@ -124,7 +126,9 @@ class DefaultAlterPartitionManager(
val metadataVersionSupplier: () => MetadataVersion
) extends AlterPartitionManager with Logging with KafkaMetricsGroup {
- // Used to allow only one pending ISR update per partition (visible for
testing)
+ // Used to allow only one pending ISR update per partition (visible for
testing).
+ // Note that we key items by TopicPartition despite using TopicIdPartition
while
+ // submitting it. We do this because we don't always have a topic id to rely
on.
Review Comment:
I realize that my comment is not clear. The main issue is when we upgrade a
cluster from an IBP < 2.8 to a newer IBP >= 2.8. During this transition, the ZK
controller assigns topics id to partitions. From an alter partition manager
perspective, it means that it might already have an update inflight for
partition (noid, foo, 1) while the partition gets its topic id assigned. As a
result, the partition will start sending updates with (id, foo, 1). If we don't
use `TopicPartition` here, they won't collide and we might end up with the two
updates in the Map for the same partition.
Jason's point is also valid. This is especially true when the request cannot
use the version which supports topic ids. When we construct the request, we
would end up sending two topics with the same name.
Overall, I think that it is preferable to keep `TopicPartition` as a key for
the time being. We will be able to get rid of this when ZK is removed.
Let me update the comment to make that clearer.
--
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]
