[GitHub] [kafka] rreddy-22 commented on a diff in pull request #13524: MINOR: Refine `PartitionAssignor` server-side interface
rreddy-22 commented on code in PR #13524:
URL: https://github.com/apache/kafka/pull/13524#discussion_r1165892001
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/MemberAssignment.java:
##
@@ -54,6 +60,6 @@ public int hashCode() {
@Override
public String toString() {
-return "MemberAssignment(targetPartitions=" + targetPartitions + ')';
+return "MemberAssignment (Target partitions = " + targetPartitions +
')';
Review Comment:
fixed thanks
--
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
[GitHub] [kafka] rreddy-22 commented on a diff in pull request #13524: MINOR: Refine `PartitionAssignor` server-side interface
rreddy-22 commented on code in PR #13524:
URL: https://github.com/apache/kafka/pull/13524#discussion_r1165681079
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/common/TopicIdToPartition.java:
##
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.coordinator.group.common;
+
+import org.apache.kafka.common.Uuid;
+
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+public class TopicIdToPartition {
+private final Uuid topicId;
+private final Integer partition;
+private final Optional> rackIds;
+
+public TopicIdToPartition(Uuid topicId, Integer topicPartition,
Optional> rackIds) {
+this.topicId = Objects.requireNonNull(topicId, "topicId cannot be
null");
+this.partition = Objects.requireNonNull(topicPartition,
"topicPartition cannot be null");
+this.rackIds = Objects.requireNonNull(rackIds, "rackId cannot be
null");
+}
+
+/**
+ * @return Universally unique id representing this topic partition.
+ */
+public Uuid topicId() {
+return topicId;
+}
+
+/**
+ * @return the partition number.
+ */
+public int partition() {
+return partition;
+}
+
+@Override
+public boolean equals(Object o) {
+if (this == o) {
+return true;
+}
+if (o == null || getClass() != o.getClass()) {
+return false;
+}
+TopicIdToPartition that = (TopicIdToPartition) o;
+return topicId.equals(that.topicId) &&
+partition.equals(that.partition);
+}
+
+@Override
+public int hashCode() {
+final int prime = 31;
Review Comment:
that sounds good! We'll name it that when we add the file! Thanks!
--
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
[GitHub] [kafka] rreddy-22 commented on a diff in pull request #13524: MINOR: Refine `PartitionAssignor` server-side interface
rreddy-22 commented on code in PR #13524:
URL: https://github.com/apache/kafka/pull/13524#discussion_r1165679441
##
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:
okay
--
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
[GitHub] [kafka] rreddy-22 commented on a diff in pull request #13524: MINOR: Refine `PartitionAssignor` server-side interface
rreddy-22 commented on code in PR #13524:
URL: https://github.com/apache/kafka/pull/13524#discussion_r1165678602
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/AssignmentMemberSpec.java:
##
@@ -37,29 +39,30 @@ public class AssignmentMemberSpec {
final Optional rackId;
/**
- * The topics that the member is subscribed to.
+ * The topicIds of topics that the member is subscribed to.
Review Comment:
yep changed it
--
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
[GitHub] [kafka] rreddy-22 commented on a diff in pull request #13524: MINOR: Refine `PartitionAssignor` server-side interface
rreddy-22 commented on code in PR #13524:
URL: https://github.com/apache/kafka/pull/13524#discussion_r1164542031
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/common/TopicIdToPartition.java:
##
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.coordinator.group.common;
+
+import org.apache.kafka.common.Uuid;
+
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+public class TopicIdToPartition {
+private final Uuid topicId;
+private final Integer partition;
+private final Optional> rackIds;
+
+public TopicIdToPartition(Uuid topicId, Integer topicPartition,
Optional> rackIds) {
+this.topicId = Objects.requireNonNull(topicId, "topicId cannot be
null");
+this.partition = Objects.requireNonNull(topicPartition,
"topicPartition cannot be null");
+this.rackIds = Objects.requireNonNull(rackIds, "rackId cannot be
null");
+}
+
+/**
+ * @return Universally unique id representing this topic partition.
+ */
+public Uuid topicId() {
+return topicId;
+}
+
+/**
+ * @return the partition number.
+ */
+public int partition() {
+return partition;
+}
+
+@Override
+public boolean equals(Object o) {
+if (this == o) {
+return true;
+}
+if (o == null || getClass() != o.getClass()) {
+return false;
+}
+TopicIdToPartition that = (TopicIdToPartition) o;
+return topicId.equals(that.topicId) &&
+partition.equals(that.partition);
+}
+
+@Override
+public int hashCode() {
+final int prime = 31;
Review Comment:
Hey, yeah we wanted a data structure that only had a topicId to partition
number mapping for each partition. The existing topicIdPartition class has
topicNames as well. I didn't know how else to name it uniquely xD
--
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
[GitHub] [kafka] rreddy-22 commented on a diff in pull request #13524: MINOR: Refine `PartitionAssignor` server-side interface
rreddy-22 commented on code in PR #13524: URL: https://github.com/apache/kafka/pull/13524#discussion_r1164538097 ## group-coordinator/src/main/java/org/apache/kafka/coordinator/group/common/TopicIdToPartition.java: ## @@ -0,0 +1,76 @@ +/* Review Comment: we can remove it here if its not the right place for it and then couple it with the assignor PRs where its used -- 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
[GitHub] [kafka] rreddy-22 commented on a diff in pull request #13524: MINOR: Refine `PartitionAssignor` server-side interface
rreddy-22 commented on code in PR #13524:
URL: https://github.com/apache/kafka/pull/13524#discussion_r1164536324
##
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:
I think there's no harm in keeping it just in case we want to know what
topic name is associated with what topicId wdyt?
--
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
[GitHub] [kafka] rreddy-22 commented on a diff in pull request #13524: MINOR: Refine `PartitionAssignor` server-side interface
rreddy-22 commented on code in PR #13524:
URL: https://github.com/apache/kafka/pull/13524#discussion_r1164535218
##
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 members;
+private final Map members;
/**
* The topics' metadata keyed by topic id
*/
-final Map topics;
+private final Map topics;
Review Comment:
I didn't think about the full implementation yet and where the rackIds will
be passed, figured we could edit it when it came to implementing the algorithm.
The topicIdToPartition data structure could be populated using this so maybe we
can add a field for a map of partition to its rackIds
--
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
[GitHub] [kafka] rreddy-22 commented on a diff in pull request #13524: MINOR: Refine `PartitionAssignor` server-side interface
rreddy-22 commented on code in PR #13524:
URL: https://github.com/apache/kafka/pull/13524#discussion_r1164530365
##
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 targetPartitions;
+private final Map> assignedTopicIdPartitions;
Review Comment:
sounds good
--
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
[GitHub] [kafka] rreddy-22 commented on a diff in pull request #13524: MINOR: Refine `PartitionAssignor` server-side interface
rreddy-22 commented on code in PR #13524:
URL: https://github.com/apache/kafka/pull/13524#discussion_r1164499488
##
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 instanceId;
+private final Optional instanceId;
/**
* The rack ID if provided.
*/
-final Optional rackId;
+private final Optional rackId;
/**
- * The topics that the member is subscribed to.
+ * The topicIds of topics that the member is subscribed to.
*/
-final Collection subscribedTopics;
+private final Collection subscribedTopics;
/**
- * The current target partitions of the member.
+ * Partitions assigned for this member keyed by topicId
*/
-final Collection targetPartitions;
+private final Map> assignedTopicIdPartitions;
Review Comment:
I thought it would be more clear if it was topicIdpartitions since we're
keying by topicId? Should we change it?
--
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
[GitHub] [kafka] rreddy-22 commented on a diff in pull request #13524: MINOR: Refine `PartitionAssignor` server-side interface
rreddy-22 commented on code in PR #13524:
URL: https://github.com/apache/kafka/pull/13524#discussion_r1163356477
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/AssignmentMemberSpec.java:
##
@@ -89,7 +92,7 @@ public String toString() {
return "AssignmentMemberSpec(instanceId=" + instanceId +
", rackId=" + rackId +
", subscribedTopics=" + subscribedTopics +
-", targetPartitions=" + targetPartitions +
+", targetPartitions=" + currentAssignmentTopicIdPartitions +
Review Comment:
done mb
--
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
[GitHub] [kafka] rreddy-22 commented on a diff in pull request #13524: MINOR: Refine `PartitionAssignor` server-side interface
rreddy-22 commented on code in PR #13524:
URL: https://github.com/apache/kafka/pull/13524#discussion_r1163355154
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/AssignmentMemberSpec.java:
##
@@ -37,29 +39,30 @@ public class AssignmentMemberSpec {
final Optional rackId;
/**
- * The topics that the member is subscribed to.
+ * The topicIds of topics that the member is subscribed to.
*/
-final Collection subscribedTopics;
+final Collection subscribedTopics;
/**
- * The current target partitions of the member.
+ * Partitions assigned for this member grouped by topicId
*/
-final Collection targetPartitions;
+final Map> currentAssignmentTopicIdPartitions;
Review Comment:
makes sense, I was also thinking of changing it, lemme change it here
--
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
[GitHub] [kafka] rreddy-22 commented on a diff in pull request #13524: MINOR: Refine `PartitionAssignor` server-side interface
rreddy-22 commented on code in PR #13524:
URL: https://github.com/apache/kafka/pull/13524#discussion_r1163354734
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/AssignmentMemberSpec.java:
##
@@ -37,29 +39,30 @@ public class AssignmentMemberSpec {
final Optional rackId;
/**
- * The topics that the member is subscribed to.
+ * The topicIds of topics that the member is subscribed to.
Review Comment:
consumers usually subscribe using topic names so I assumed topics would mean
topic name, to make sure we know its the topic Id generated by the coordinator
I said topicIds of the topics that the member is subscribed to
--
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
