KAFKA-17912: Align string representations of SharePartitionKey by AndrewJSchofield · Pull Request #17656 · apache/kafka

AndrewJSchofield · 2024-10-31T17:19:55Z

The recent test failure documented by #17649 seems to be related to the string representation of SharePartitionKey. This is a combination of group-id:topic-id:partition used by share groups to refer to a "share-partition" which is a particular share group's view of a topic-partition. The SharePartitionKey is used to find the share coordinator responsible for a share-partition.

It looks like some code was using a simple concatenation of the fields, while others was using toString which is human-readable. This mismatch seems to have caused requests to be sent to the wrong share coordinator, resulting in tests timing out.

This PR aligns all lookup-based uses of SharePartitionKey on the simple concatenation of its constituent parts.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

mumrah

Thanks for the quick fix on this @AndrewJSchofield 👍

mumrah · 2024-10-31T17:26:15Z


    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()));


@AndrewJSchofield could we move away from using String keys to avoid issues like this in the future? IOW, could we refactor partitionFor to accept a SharePartitionKey?

I created https://issues.apache.org/jira/browse/KAFKA-17914 to cover this. Unfortunately, it's a little more work than just changing this class, and I think it would be better to prioritise getting this merged.

@mumrah Changing partitionFor would be counter productive, IMO. This is because it will be invoked by the KafkaApis FIND_COORD API handler.

The find coordinator request comprises of the coordinator keys as Strings (as its a JSON defined RPC spec). The find coordinator handler will simply invoke partitionFor with the key specified in the request argument.

If we change partitionFor signature to accept the SharePartitionKey, we would not have enough information in FIND_COORD API handler to create a SharePartitionKey object and call partitionFor unless we resort to splitting and formatting string in the request and creating the required object and pass to partitionFor.

cc: @AndrewJSchofield

@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.

@AndrewJSchofield

We could transform that into a SharePartitionKey on receipt

Which would mean writing code to split and format strings - was trying to avoid it.

is a combination of a group ID and a TopicIdPartition

I did not want to do this. The initial impl of SPK clearly defined 3 fields -String groupId, Uuid topicId, int partition. In fact, the entire share-coordinator flow does not use the TopicIdPartition argument to the constructor at all. Not sure whether it is necessary for the SharePartition flow or was it done for aesthetic reasons.

the entire share-coordinator flow does not use the TopicIdPartition argument to the constructor at all

IMO, we should use TopicIdPartition class itself as used elsewhere, not beacause of aesthetic reasons but it's a logical encapsulated entity.

but it's a logical encapsulated entity.

Yes thats true but it might not be a good fit since topicName is always (or mostly) null.

Yes thats true but it might not be a good fit since topicName is always (or mostly) null.

And why is that? For the calls from SharePartition we can fill up topicName as well. We do a metadata read to verify first.

If you plan to use it then it should be fine. Otherwise, serves no purpose.

It can be argued that doing this causes additional problems, both with FIND_COORD RPC as well as well as persistence RPCs.

If we forgo the topicName from both the SharePartition and the ShareCoordinator side, we could directly invoke SPK.hashCode() method in the partitionFor method thus completely abstracting out the reliance on groupId:topicId:partitionId.

And as pointed out above we do not have access to the topic name in the above RPCs.

mumrah · 2024-10-31T17:27:26Z

@@ -539,7 +539,7 @@ public void onNewMetadataImage(MetadataImage newImage, MetadataDelta delta) {
    }

    private TopicPartition topicPartitionFor(SharePartitionKey key) {


Can we add a unit test for this method?

Is the string representation String.format("%s:%s:%d", groupId, topicId, partition) included by KIP-932? if not, maybe we should add it to the KIP and json file (FindCoordinatorRequest).

I have added it to the JSON file. I'll make sure the KIP contains this too.

Kip ref:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-932%3A+Queues+for+Kafka#KIP932:QueuesforKafka-Thesharecoordinatorandtheshare-groupstatetopic

apoorvmittal10 · 2024-10-31T17:36:24Z

Thanks @AndrewJSchofield for the quick fix.

chia7712 · 2024-10-31T17:38:51Z

@AndrewJSchofield nice find. It seems like it could pass sometimes if we're lucky, as both string representations might end up mapping to the same partition.

apoorvmittal10

Shall we also enable back the failing testShareGroups test?

chia7712

LGTM

I'll merge this to unblock the CI. Please feel free to leave more comments or open a JIRA for further improvements.

chia7712 · 2024-11-01T07:55:05Z

Shall we also enable back the failing testShareGroups test?

https://issues.apache.org/jira/browse/KAFKA-17919

…#17656) Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

KAFKA-17912: Align string representations of SharePartitionKey

08424cf

github-actions Bot added KIP-932 Queues for Kafka small Small PRs labels Oct 31, 2024

AndrewJSchofield added the ci-approved label Oct 31, 2024

AndrewJSchofield mentioned this pull request Oct 31, 2024

KAFKA-17463: Fixing test share groups test #17645

Merged

3 tasks

mumrah reviewed Oct 31, 2024

View reviewed changes

apoorvmittal10 reviewed Oct 31, 2024

View reviewed changes

Review comments

1be9666

github-actions Bot added the clients label Oct 31, 2024

chia7712 approved these changes Nov 1, 2024

View reviewed changes

chia7712 merged commit 346fdba into apache:trunk Nov 1, 2024

AndrewJSchofield deleted the KAFKA-17912 branch November 1, 2024 13:01

AndrewJSchofield mentioned this pull request Nov 1, 2024

KAFKA-17919: enable back the failing testShareGroups test #17658

Merged

3 tasks

abhishekgiri23 pushed a commit to abhishekgiri23/kafka that referenced this pull request Nov 2, 2024

KAFKA-17912 Align string representations of SharePartitionKey (apache…

b1971b5

…#17656) Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

chiacyu pushed a commit to chiacyu/kafka that referenced this pull request Nov 30, 2024

KAFKA-17912 Align string representations of SharePartitionKey (apache…

26c003a

…#17656) Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

tedyu pushed a commit to tedyu/kafka that referenced this pull request Jan 6, 2025

KAFKA-17912 Align string representations of SharePartitionKey (apache…

6163f23

…#17656) Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

		@@ -539,7 +539,7 @@ public void onNewMetadataImage(MetadataImage newImage, MetadataDelta delta) {
		}

		private TopicPartition topicPartitionFor(SharePartitionKey key) {

Conversation

AndrewJSchofield commented Oct 31, 2024

Committer Checklist (excluded from commit message)

Uh oh!

mumrah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smjn Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smjn Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smjn Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoorvmittal10 commented Oct 31, 2024

Uh oh!

chia7712 commented Oct 31, 2024

Uh oh!

apoorvmittal10 left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Nov 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

smjn Nov 1, 2024 •

edited

Loading

smjn Nov 1, 2024 •

edited

Loading

smjn Nov 1, 2024 •

edited

Loading