KAFKA-17578: Remove partitionRacks from TopicMetadata #17233

FrankYang0529 · 2024-09-19T12:37:38Z

The ModernGroup#subscribedTopicMetadata takes too much memory due to partitionRacks. This is not being used at the moment as the consumer protocol does not support rack aware assignments.

A heap dump from a group with 500 members, 2K subscribed topic partitions shows 654,400 bytes used for partitionRacks. The rest of the ConsumerGroup object holds 822,860 bytes.

Committer Checklist (excluded from commit message)

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

dajac

@FrankYang0529 Thanks for working on this. I left a few high level comments to start with.

dajac · 2024-09-19T16:04:45Z

.../src/main/java/org/apache/kafka/coordinator/group/api/assignor/SubscribedTopicDescriber.java

-     * @return The set of racks corresponding to the replicas of the topic's partition.
-     *         If the topic Id does not exist, an empty set is returned.
-     */
-    Set<String> racksForPartition(Uuid topicId, int partition);


We cannot remove this from the public interface. Let’s return an empty set for now.

dajac · 2024-09-19T16:06:04Z

group-coordinator/src/main/resources/common/message/ConsumerGroupPartitionMetadataValue.json

@@ -27,14 +27,7 @@
      { "name": "TopicName", "versions": "0+", "type": "string",
        "about": "The topic name." },
      { "name": "NumPartitions", "versions": "0+", "type": "int32",
-        "about": "The number of partitions of the topic." },


Same. Unfortunately, we cannot remove fields because we released it. We must keep it. Let’s put a comment explaining that we don’t use it.

dajac · 2024-09-19T22:50:22Z

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/modern/TopicMetadata.java

        return new TopicMetadata(
            record.topicId(),
            record.topicName(),
-            record.numPartitions(),
-            partitionRacks);
+            record.numPartitions());
    }

    public static TopicMetadata fromRecord(
            ShareGroupPartitionMetadataValue.TopicMetadata record


nit: Not related to your changes but the indentation is incorrect here.

dajac · 2024-09-19T22:50:32Z

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/modern/TopicMetadata.java

                record.topicId(),
                record.topicName(),
-                record.numPartitions(),
-                partitionRacks);
+                record.numPartitions());


nit: ditto.

dajac · 2024-09-19T22:51:12Z

group-coordinator/src/test/java/org/apache/kafka/coordinator/group/Assertions.java

@@ -242,27 +242,6 @@ private static void assertApiMessageAndVersionEquals(
                assertEquals(expectedTopicMetadata.topicId(), actualTopicMetadata.topicId());
                assertEquals(expectedTopicMetadata.topicName(), actualTopicMetadata.topicName());
                assertEquals(expectedTopicMetadata.numPartitions(), actualTopicMetadata.numPartitions());
-


I suppose that we can keep this code if we keep the fields in the record.

FrankYang0529 · 2024-09-20T16:07:38Z

Hi @dajac, thanks for review and suggestion. I addressed all comments. However, both GroupMetadataManagerTest#testGroupEpochBumpWhenNewStaticMemberJoins and GroupMetadataManagerTest#testNewJoiningMemberTriggersNewTargetAssignment are failed cases. I will try to fix them this weekend.

FrankYang0529 · 2024-09-21T06:43:14Z

Hi @dajac, I fixed both testGroupEpochBumpWhenNewStaticMemberJoins and testNewJoiningMemberTriggersNewTargetAssignment. Originally, different partitionRacks triggers subscription metadata update which generates a new consumer group subscription metadata record. Since we remove the field, we have to remove the related record in test cases.

dajac · 2024-09-21T13:21:11Z

@FrankYang0529 Thanks for the update. I am travelling today so I will review it on Monday.

dajac · 2024-09-23T16:00:28Z

@FrankYang0529 There are a few conflicts. Could you please address them?

dajac

@FrankYang0529 Thanks for the update. I left a few nits for consideration.

dajac · 2024-09-24T12:25:04Z

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/modern/ModernGroup.java

                newSubscriptionMetadata.put(topicName, new TopicMetadata(
                    topicImage.id(),
                    topicImage.name(),
-                    topicImage.partitions().size(),
-                    partitionRacks)
+                    topicImage.partitions().size())


nit: Could you please put the last closing parenthesis on the next line?

newSubscriptionMetadata.put(topicName, new TopicMetadata( topicImage.id(), topicImage.name(), topicImage.partitions().size() ));

dajac · 2024-09-24T12:25:46Z

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/modern/TopicMetadata.java

        return new TopicMetadata(
            record.topicId(),
            record.topicName(),
-            record.numPartitions(),
-            partitionRacks);
+            record.numPartitions());


nit: Ditto about the closing parenthesis.

dajac · 2024-09-24T12:25:52Z

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/modern/TopicMetadata.java

-                partitionRacks);
+            record.topicId(),
+            record.topicName(),
+            record.numPartitions());


nit: ditto.

Signed-off-by: PoAn Yang <[email protected]>

FrankYang0529 · 2024-09-24T12:37:24Z

Hi @dajac, thanks for the review. Updated it.

dajac

LGTM, thanks.

dajac · 2024-09-25T07:50:09Z

@FrankYang0529 Thanks for the patch. I just merged it. We need to work on another strategy to trigger rebalances based on rack changes. I have a few ideas. If you are interested, you could pick it up.

FrankYang0529 · 2024-09-25T08:13:53Z

Hi @dajac, I'm glad to handle it. Feel free to assign to me. Thank you 😄

dajac · 2024-10-09T07:27:36Z

@FrankYang0529 I filed https://issues.apache.org/jira/browse/KAFKA-17747.

FrankYang0529 · 2024-10-09T07:28:39Z

@FrankYang0529 I filed https://issues.apache.org/jira/browse/KAFKA-17747.

Thank you. May I try it?

FrankYang0529 marked this pull request as draft September 19, 2024 12:37

dajac reviewed Sep 19, 2024

View reviewed changes

dajac added the KIP-848 The Next Generation of the Consumer Rebalance Protocol label Sep 19, 2024

dajac reviewed Sep 19, 2024

View reviewed changes

FrankYang0529 force-pushed the KAFKA-17578 branch from 60e0793 to 0bb615e Compare September 20, 2024 15:29

github-actions bot added the performance label Sep 20, 2024

FrankYang0529 force-pushed the KAFKA-17578 branch from 0bb615e to 354b577 Compare September 21, 2024 06:37

FrankYang0529 marked this pull request as ready for review September 21, 2024 06:43

FrankYang0529 force-pushed the KAFKA-17578 branch from 354b577 to 9a7a620 Compare September 24, 2024 09:16

dajac reviewed Sep 24, 2024

View reviewed changes

KAFKA-17578: Remove partitionRacks from TopicMetadata

1485824

Signed-off-by: PoAn Yang <[email protected]>

FrankYang0529 force-pushed the KAFKA-17578 branch from 9a7a620 to 1485824 Compare September 24, 2024 12:37

dajac approved these changes Sep 25, 2024

View reviewed changes

dajac merged commit bb97d63 into apache:trunk Sep 25, 2024
9 of 10 checks passed

FrankYang0529 deleted the KAFKA-17578 branch September 25, 2024 07:49

FrankYang0529 mentioned this pull request Oct 10, 2024

KAFKA-17747: Trigger rebalance on rack topology changes #17444

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-17578: Remove partitionRacks from TopicMetadata #17233

KAFKA-17578: Remove partitionRacks from TopicMetadata #17233

FrankYang0529 commented Sep 19, 2024

dajac left a comment

dajac Sep 19, 2024

dajac Sep 19, 2024

dajac Sep 19, 2024

dajac Sep 19, 2024

dajac Sep 19, 2024

FrankYang0529 commented Sep 20, 2024

FrankYang0529 commented Sep 21, 2024

dajac commented Sep 21, 2024

dajac commented Sep 23, 2024

dajac left a comment

dajac Sep 24, 2024

dajac Sep 24, 2024

dajac Sep 24, 2024

FrankYang0529 commented Sep 24, 2024

dajac left a comment

dajac commented Sep 25, 2024

FrankYang0529 commented Sep 25, 2024

dajac commented Oct 9, 2024

FrankYang0529 commented Oct 9, 2024

KAFKA-17578: Remove partitionRacks from TopicMetadata #17233

KAFKA-17578: Remove partitionRacks from TopicMetadata #17233

Conversation

FrankYang0529 commented Sep 19, 2024

Committer Checklist (excluded from commit message)

dajac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankYang0529 commented Sep 20, 2024

FrankYang0529 commented Sep 21, 2024

dajac commented Sep 21, 2024

dajac commented Sep 23, 2024

dajac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankYang0529 commented Sep 24, 2024

dajac left a comment

Choose a reason for hiding this comment

dajac commented Sep 25, 2024

FrankYang0529 commented Sep 25, 2024

dajac commented Oct 9, 2024

FrankYang0529 commented Oct 9, 2024