dajac commented on code in PR #13524:
URL: https://github.com/apache/kafka/pull/13524#discussion_r1163998872
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/AssignmentMemberSpec.java:
##########
@@ -29,37 +31,53 @@ public class AssignmentMemberSpec {
/**
* The instance ID if provided.
*/
- final Optional<String> instanceId;
+ private final Optional<String> instanceId;
/**
* The rack ID if provided.
*/
- final Optional<String> rackId;
+ private final Optional<String> rackId;
/**
- * The topics that the member is subscribed to.
+ * The topicIds of topics that the member is subscribed to.
*/
- final Collection<String> subscribedTopics;
+ private final Collection<Uuid> subscribedTopics;
Review Comment:
nit: `subscribedTopicIds`
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/AssignmentMemberSpec.java:
##########
@@ -29,37 +31,53 @@ public class AssignmentMemberSpec {
/**
* The instance ID if provided.
*/
- final Optional<String> instanceId;
+ private final Optional<String> instanceId;
/**
* The rack ID if provided.
*/
- final Optional<String> rackId;
+ private final Optional<String> rackId;
/**
- * The topics that the member is subscribed to.
+ * The topicIds of topics that the member is subscribed to.
*/
- final Collection<String> subscribedTopics;
+ private final Collection<Uuid> subscribedTopics;
/**
- * The current target partitions of the member.
+ * Partitions assigned for this member keyed by topicId
*/
- final Collection<TopicPartition> targetPartitions;
+ private final Map<Uuid, Set<Integer>> assignedTopicIdPartitions;
Review Comment:
nit: Sorry to repeat myself but how about `assignedPartitions`?
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/AssignmentMemberSpec.java:
##########
@@ -29,37 +31,53 @@ public class AssignmentMemberSpec {
/**
* The instance ID if provided.
*/
- final Optional<String> instanceId;
+ private final Optional<String> instanceId;
/**
* The rack ID if provided.
*/
- final Optional<String> rackId;
+ private final Optional<String> rackId;
/**
- * The topics that the member is subscribed to.
+ * The topicIds of topics that the member is subscribed to.
*/
- final Collection<String> subscribedTopics;
+ private final Collection<Uuid> subscribedTopics;
/**
- * The current target partitions of the member.
+ * Partitions assigned for this member keyed by topicId
Review Comment:
nit: Add missing `.` at the end.
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/common/TopicIdToPartition.java:
##########
@@ -0,0 +1,76 @@
+/*
Review Comment:
What do we do with this file?
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/AssignmentTopicMetadata.java:
##########
@@ -25,12 +25,12 @@ public class AssignmentTopicMetadata {
/**
* The topic name.
*/
- final String topicName;
+ private final String topicName;
Review Comment:
We should probably remove the `topicName` as we only use `topicId`.
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/MemberAssignment.java:
##########
@@ -44,16 +47,16 @@ public boolean equals(Object o) {
MemberAssignment that = (MemberAssignment) o;
- return targetPartitions.equals(that.targetPartitions);
+ return
assignedTopicIdPartitions.equals(that.assignedTopicIdPartitions);
}
@Override
public int hashCode() {
- return targetPartitions.hashCode();
+ return assignedTopicIdPartitions.hashCode();
}
@Override
public String toString() {
- return "MemberAssignment(targetPartitions=" + targetPartitions + ')';
+ return "MemberAssignment (Assignment per topic Id = " +
assignedTopicIdPartitions + ')';
Review Comment:
nit: `"MemberAssignment(assignedTopicIdPartitions=" +
assignedTopicIdPartitions + ')';`
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/AssignmentSpec.java:
##########
@@ -28,12 +28,12 @@ public class AssignmentSpec {
/**
* The members keyed by member id.
*/
- final Map<String, AssignmentMemberSpec> members;
+ private final Map<String, AssignmentMemberSpec> members;
/**
* The topics' metadata keyed by topic id
*/
- final Map<Uuid, AssignmentTopicMetadata> topics;
+ private final Map<Uuid, AssignmentTopicMetadata> topics;
Review Comment:
Did you mean to add the partitions with their rack ids to
`AssignmentTopicMetadata`? What's your plan?
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/MemberAssignment.java:
##########
@@ -16,25 +16,28 @@
*/
package org.apache.kafka.coordinator.group.assignor;
-import org.apache.kafka.common.TopicPartition;
+import org.apache.kafka.common.Uuid;
-import java.util.Collection;
+import java.util.Map;
import java.util.Objects;
+import java.util.Set;
/**
* The partition assignment for a consumer group member.
*/
public class MemberAssignment {
/**
- * The target partitions assigned to this member.
+ * The target partitions assigned to this member keyed by topicId.
*/
- final Collection<TopicPartition> targetPartitions;
+ private final Map<Uuid, Set<Integer>> assignedTopicIdPartitions;
Review Comment:
nit: `targetPartitions` is actually good here or `assignedPartitions` again.
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/AssignmentMemberSpec.java:
##########
@@ -29,37 +31,53 @@ public class AssignmentMemberSpec {
/**
* The instance ID if provided.
*/
- final Optional<String> instanceId;
+ private final Optional<String> instanceId;
/**
* The rack ID if provided.
*/
- final Optional<String> rackId;
+ private final Optional<String> rackId;
/**
- * The topics that the member is subscribed to.
+ * The topicIds of topics that the member is subscribed to.
*/
- final Collection<String> subscribedTopics;
+ private final Collection<Uuid> subscribedTopics;
/**
- * The current target partitions of the member.
+ * Partitions assigned for this member keyed by topicId
*/
- final Collection<TopicPartition> targetPartitions;
+ private final Map<Uuid, Set<Integer>> assignedTopicIdPartitions;
+
+ public Optional<String> instanceId() {
Review Comment:
nit: Let's add javadoc to all those accessors. It can by as simple as:
```
/**
* @ return The instance id as an Optional.
*/
```
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
