KAFKA-14702: Extend server side assignor to support rack aware replica placement by rreddy-22 · Pull Request #13998 · apache/kafka

rreddy-22 · 2023-07-12T01:08:41Z

We want to make the assignors rack aware and in order to obtain and provide the rack Information the following interface modifications were made:-

TopicDescriber - Interface passed to the assignor to obtain topic and partition metadata
AssignmentTopicDescriber - Implementation of the interface above
Add partitionRack field to the ConsumerGroupPartitionMetadataValue.json
TopicMetadata has a new attribute Map<Integer, Set> partitionRackInfo

… TopicIdToPartition data structure

…ccording to PR comments

…ava doc for getter methods

…ForMember, addressed PR comments, changed Map.Entry to Map.forEach etc.

…dy-22/KAFKA-14514 # Conflicts: # group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/AssignmentMemberSpec.java # group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/GroupAssignment.java # group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/MemberAssignment.java

…t topic test.

…tests

…515-Optimized

…ss to handle topic and cluster images and added an AbstractPartitionAssignor

…on to the assignor, modified tests to incorporate changes

dajac

@rreddy-22 Thanks for the PR. I made a pass on the non-test files and left some comments/questions.

dajac · 2023-07-20T09:30:15Z

+ * implemented and ready to be released.
+ */
+@InterfaceStability.Unstable
+public interface AssignmentTopicDescriber {


nit: I wonder if SubscribedTopicDescriber would be better based on the javadoc.

Yeah I named it this way cause I was just wondering if it'd be more uniform with assignmentSpec but I'll change it cause I agree

dajac · 2023-07-20T09:30:37Z

+    Set<Uuid> subscribedTopicIds();
+
+    /**
+     * Number of partitions for the given topicId.


nit: topicIds -> topic id.

It says topicId singular already, did we want a space between topic and Id

dajac · 2023-07-20T09:49:56Z

@@ -29,7 +29,27 @@
      { "name": "TopicName", "versions": "0+", "type": "string",
        "about": "The topic name." },
      { "name": "NumPartitions", "versions": "0+", "type": "int32",


Do we still need this field? I suppose that it depends on whether you plan to store partition metadata for partitions without racks or not.

I think we shouldn't store the racks if they aren't available, I just changed it to remove numPartitions but I'll change it back now

…e-Changes' into rreddy-22/Rack-Assignor-Interface-Changes

…anges

dajac

@rreddy-22 Thanks for the update. I left some comments. Note that the build is read. Could you also look into this?

dajac · 2023-07-24T16:02:24Z

  <allow pkg="org.apache.kafka.common.utils" />
  <allow pkg="org.apache.kafka.common.errors" exact-match="true" />
  <allow pkg="org.apache.kafka.common.memory" />
+  <allow pkg="org.apache.kafka.coordinator.group.common"/>


I suppose that we can remove this, isn't it?

yep correct we don't need it now that we don't have any classes in it

dajac · 2023-07-24T16:07:54Z

      { "name": "NumPartitions", "versions": "0+", "type": "int32",
-        "about": "The number of partitions of the topic." }
+        "about": "The number of partitions of the topic." },
+      { "name": "PartitionRacks",


nit: Partitions instead of PartitionRacks?

I changed it from partitionRackInfo to partitionRacks based on the previous comment

named it partitionMetadata as well

dajac · 2023-07-24T16:46:05Z

-            topics.put(topicMetadata.id(), new AssignmentTopicMetadata(topicMetadata.numPartitions()))
+            topicMetadataMap.put(
+                topicMetadata.id(),
+                new PartitionMetadata(topicMetadata.partitionRacks())


I am confused by the usage of PartitionMetadata only here. I think that this should be used in subscription metadata as well or not at all. It does not make sense to only use it here, right? Our goal is to avoid having to copy anything.

Let me create a Uuid -> PartitionMetadata map in TopicMetadata itself and then pass it directly

dajac · 2023-07-24T16:53:47Z

+     * @return Number of partitions associated with the topic.
+     */
+    public int numPartitions() {
+        return partitionsWithRacks.size();


My understanding is that the map won't have anything for partitions without racks so this must be wrong, isn't it?

yes, must've forgotten to change this file after reversing the changes to remove numPartitions, sorry

…anges

…m:rreddy-22/kafka-rreddy into rreddy-22/Rack-Assignor-Interface-Changes

…picMetadata, PR comments

dajac · 2023-07-26T06:40:55Z

Replaced by #14099.

rreddy-22 and others added 30 commits March 23, 2023 13:02

Server Side Sticky Range Assignor full implementation

e8d3607

removed reduce partitions case

3df3460

removed reduce partitions case

7410529

Addressed PR comments

3b6e65e

Addressed PR comments

4b321aa

Merge branch 'apache:trunk' into rreddy-22/KAFKA-14514

b4041a7

minor

7352cbb

Made subscribed topics a collection, removed * import exception

57b94ca

Interface changes for assignment map and subscribed topics, added new…

5065109

… TopicIdToPartition data structure

Added toString method and hash

100a04e

Made all attributes private and added getter methods, changed names a…

73c7fdc

…ccording to PR comments

Removed topicIdToPartition class, changed attribute names and added j…

68d53b3

…ava doc for getter methods

Server Side Sticky Range Assignor full implementation

48a9822

removed reduce partitions case

dcb8198

removed reduce partitions case

9f3a423

Addressed PR comments

a925b02

Addressed PR comments

7d16269

minor

7369db3

Made subscribed topics a collection, removed * import exception

918d262

Addressed some PR comments

288fa1f

Removed putList, putSet, changed generic pair to remainingAssignments…

a1bdb58

…ForMember, addressed PR comments, changed Map.Entry to Map.forEach etc.

Interface changes incorporated, added stickiness test and non-existen…

3d92645

…t topic test.

Merge branch 'trunk' into rreddy-22/KAFKA-14514

309cc61

First draft optimised uniform assignor code

7fd7794

Fixed checkstyle and added test file

dffad67

import changes

ef81486

Separated uniform and general builder in different files, added more …

0cd618a

…tests

Added stickiness test

d33bbfa

Renamed topicIdToPartition class

2d50c27

rreddy-22 added 8 commits May 31, 2023 16:21

test changes and code changes

931bcf7

Refactored code to reduce format conversion time

e9ebc4f

Abstract Partition Assignor

9815d6d

Merge remote-tracking branch 'upstream/trunk' into rreddy-22/KAFKA-14…

ce540f9

…515-Optimized

Added rack interface changes, added a new TopicAndClusterMetadata cla…

478488e

…ss to handle topic and cluster images and added an AbstractPartitionAssignor

Removed extra lines

c50f3bd

Small edits

f68471a

New interfaces and classes added to facilitate passing rack informati…

579bb94

…on to the assignor, modified tests to incorporate changes

dajac changed the title ~~KIP-848: Rack awareness interface changes for assignors~~ KAFKA-14702: Extend server side assignor to support rack aware replica placement Jul 20, 2023

dajac added the KIP-848 The Next Generation of the Consumer Rebalance Protocol label Jul 20, 2023

dajac reviewed Jul 20, 2023

View reviewed changes

rreddy-22 and others added 9 commits July 21, 2023 03:09

Changes based on PR comments

4511683

Merge branch 'trunk' into rreddy-22/Rack-Assignor-Interface-Changes

faafed2

minor changes

22f2df0

Merge remote-tracking branch 'origin/rreddy-22/Rack-Assignor-Interfac…

6650ea5

…e-Changes' into rreddy-22/Rack-Assignor-Interface-Changes

Moved SubscribedTopicMetadata to the consumer package

061dac7

Merge branch 'apache:trunk' into rreddy-22/Rack-Assignor-Interface-Ch…

57b9a62

…anges

minor

a30a534

minor

6fe72df

reverted grammar changes

c0b464c

dajac reviewed Jul 24, 2023

View reviewed changes

rreddy-22 and others added 6 commits July 24, 2023 10:04

reverted reviewers.py changes

a39e76f

minor

2d40125

Merge branch 'apache:trunk' into rreddy-22/Rack-Assignor-Interface-Ch…

81f1ccd

…anges

Merge branch 'rreddy-22/Rack-Assignor-Interface-Changes' of github.co…

1c955b5

…m:rreddy-22/kafka-rreddy into rreddy-22/Rack-Assignor-Interface-Changes

Minor

5a93532

Deleted PartitionMetadata and added TopicMetadata to the SubscribedTo…

71f8488

…picMetadata, PR comments

rreddy-22 mentioned this pull request Jul 25, 2023

KAFKA-14702: Extend server side assignor to support rack aware replica placement #14099

Merged

dajac closed this Jul 26, 2023

Uh oh!

Conversation

rreddy-22 commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dajac 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!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dajac commented Jul 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rreddy-22 commented Jul 12, 2023 •

edited

Loading