AndrewJSchofield commented on code in PR #17656:
URL: https://github.com/apache/kafka/pull/17656#discussion_r1825801573
##########
share-coordinator/src/main/java/org/apache/kafka/coordinator/share/ShareCoordinatorService.java:
##########
@@ -539,7 +539,7 @@ public void onNewMetadataImage(MetadataImage newImage,
MetadataDelta delta) {
}
private TopicPartition topicPartitionFor(SharePartitionKey key) {
- return new TopicPartition(Topic.SHARE_GROUP_STATE_TOPIC_NAME,
partitionFor(key.toString()));
+ return new TopicPartition(Topic.SHARE_GROUP_STATE_TOPIC_NAME,
partitionFor(key.asCoordinatorKey()));
Review Comment:
@smjn I don't quite agree. I understand that the FindCoordinator RPC
includes a string `group-id:topic-id:partition`. We could transform that into a
`SharePartitionKey` on receipt, and then `ShareCoordinator.partitionFor` could
take a `SharePartitionKey` as its parameter.
One other thing jumped out at me when I was debugging this.
`SharePartitionKey` is a combination of a group ID and a `TopicIdPartition`.
The topic name in the `TopicIdPartition` was null because it was constructed
when no topic name was available. While a partition can be identified by just a
topic ID and a partition, it does strike me that we need to be super-careful
that we don't have a topic name sometimes, and no topic name at others, and
introduce a bug due to this difference. One thing we could do for instance is
ensure that the `TopicIdPartition` in a `SharePartitionKey` always has a null
topic name. Maybe it doesn't make sense to use `TopicIdPartition.toString()` as
part of the `SharePartitionKey.toString()` because the topic name is always
null. Just wondering whether there's an easy win here in terms of defensive
coding.
--
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]
