Re: [PR] KAFKA-16626: Lazily convert subscribed topic names to topic ids [kafka]
dajac merged PR #15970: URL: https://github.com/apache/kafka/pull/15970 -- 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
Re: [PR] KAFKA-16626: Lazily convert subscribed topic names to topic ids [kafka]
dajac commented on PR #15970: URL: https://github.com/apache/kafka/pull/15970#issuecomment-2128833459 The failed tests are related to https://github.com/apache/kafka/pull/15972. Merging to trunk. -- 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
Re: [PR] KAFKA-16626: Lazily convert subscribed topic names to topic ids [kafka]
dajac commented on code in PR #15970:
URL: https://github.com/apache/kafka/pull/15970#discussion_r1609560175
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/assignor/OptimizedUniformAssignmentBuilderTest.java:
##
@@ -133,13 +135,13 @@ public void
testFirstAssignmentTwoMembersTwoTopicsNoMemberRacks() {
members.put(memberA, new AssignmentMemberSpec(
Optional.empty(),
Optional.empty(),
-Arrays.asList(topic1Uuid, topic3Uuid),
+ mkSet(topic1Uuid, topic3Uuid),
Review Comment:
nit: Indentation is off.
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/assignor/OptimizedUniformAssignmentBuilderTest.java:
##
@@ -337,7 +337,7 @@ public void
testReassignmentWhenPartitionsAreAddedForTwoMembersTwoTopics() {
members.put(memberA, new AssignmentMemberSpec(
Optional.empty(),
Optional.empty(),
-Arrays.asList(topic1Uuid, topic2Uuid),
+ mkSet(topic1Uuid, topic2Uuid),
Review Comment:
ditto.
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/assignor/OptimizedUniformAssignmentBuilderTest.java:
##
@@ -350,7 +350,7 @@ public void
testReassignmentWhenPartitionsAreAddedForTwoMembersTwoTopics() {
members.put(memberB, new AssignmentMemberSpec(
Optional.empty(),
Optional.empty(),
-Arrays.asList(topic1Uuid, topic2Uuid),
+ mkSet(topic1Uuid, topic2Uuid),
Review Comment:
ditto.
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/assignor/OptimizedUniformAssignmentBuilderTest.java:
##
@@ -409,15 +409,15 @@ public void
testReassignmentWhenOneMemberAddedAfterInitialAssignmentWithTwoMembe
members.put(memberB, new AssignmentMemberSpec(
Optional.empty(),
Optional.empty(),
-Arrays.asList(topic1Uuid, topic2Uuid),
+ mkSet(topic1Uuid, topic2Uuid),
Review Comment:
ditto.
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/assignor/OptimizedUniformAssignmentBuilderTest.java:
##
@@ -398,7 +398,7 @@ public void
testReassignmentWhenOneMemberAddedAfterInitialAssignmentWithTwoMembe
members.put(memberA, new AssignmentMemberSpec(
Optional.empty(),
Optional.empty(),
-Arrays.asList(topic1Uuid, topic2Uuid),
+ mkSet(topic1Uuid, topic2Uuid),
Review Comment:
ditto.
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/assignor/OptimizedUniformAssignmentBuilderTest.java:
##
@@ -286,7 +286,7 @@ public void
testReassignmentForTwoMembersTwoTopicsGivenUnbalancedPrevAssignment(
members.put(memberB, new AssignmentMemberSpec(
Optional.empty(),
Optional.empty(),
-Arrays.asList(topic1Uuid, topic2Uuid),
+ mkSet(topic1Uuid, topic2Uuid),
Review Comment:
ditto.
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/assignor/OptimizedUniformAssignmentBuilderTest.java:
##
@@ -409,15 +409,15 @@ public void
testReassignmentWhenOneMemberAddedAfterInitialAssignmentWithTwoMembe
members.put(memberB, new AssignmentMemberSpec(
Optional.empty(),
Optional.empty(),
-Arrays.asList(topic1Uuid, topic2Uuid),
+ mkSet(topic1Uuid, topic2Uuid),
currentAssignmentForB
));
// Add a new member to trigger a re-assignment.
members.put(memberC, new AssignmentMemberSpec(
Optional.empty(),
Optional.empty(),
-Arrays.asList(topic1Uuid, topic2Uuid),
+ mkSet(topic1Uuid, topic2Uuid),
Review Comment:
ditto.
--
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
Re: [PR] KAFKA-16626: Lazily convert subscribed topic names to topic ids [kafka]
jeffkbkim commented on PR #15970: URL: https://github.com/apache/kafka/pull/15970#issuecomment-2123510093 @dajac thanks for the review. I have addressed your comments -- 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
Re: [PR] KAFKA-16626: Lazily convert subscribed topic names to topic ids [kafka]
dajac commented on code in PR #15970:
URL: https://github.com/apache/kafka/pull/15970#discussion_r1608815939
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/assignor/OptimizedUniformAssignmentBuilderTest.java:
##
@@ -53,6 +55,13 @@ public class OptimizedUniformAssignmentBuilderTest {
private final String memberB = "B";
private final String memberC = "C";
+private final TopicsImage topicsImage = new MetadataImageBuilder()
Review Comment:
Do we still need this one?
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/consumer/TopicIds.java:
##
@@ -0,0 +1,185 @@
+/*
+ * 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.consumer;
+
+import org.apache.kafka.common.Uuid;
+import org.apache.kafka.image.TopicImage;
+import org.apache.kafka.image.TopicsImage;
+
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.NoSuchElementException;
+import java.util.Objects;
+import java.util.Set;
+
+/**
+ * TopicIds is initialized with topic names (String) but exposes a Set of
topic ids (Uuid) to the
+ * user and performs the conversion lazily with TopicsImage.
+ */
+public class TopicIds implements Set {
+private final Set topicNames;
+private final TopicsImage image;
+
+public TopicIds(
+Set topicNames,
+TopicsImage image
+) {
+this.topicNames = Objects.requireNonNull(topicNames);
+this.image = Objects.requireNonNull(image);
+}
+
+@Override
+public int size() {
+return topicNames.size();
+}
+
+@Override
+public boolean isEmpty() {
+return topicNames.isEmpty();
+}
+
+@Override
+public boolean contains(Object o) {
+if (o instanceof Uuid) {
+Uuid topicId = (Uuid) o;
+TopicImage topicImage = image.getTopic(topicId);
+if (topicImage == null) return false;
+return topicNames.contains(topicImage.name());
+}
+return false;
+}
+
+private static class TopicIdIterator implements Iterator {
+final Iterator iterator;
+final TopicsImage image;
+private Uuid next = null;
+
+private TopicIdIterator(
+Iterator iterator,
+TopicsImage image
+) {
+this.iterator = Objects.requireNonNull(iterator);
+this.image = Objects.requireNonNull(image);
+}
+
+@Override
+public boolean hasNext() {
+if (next != null) return true;
+Uuid result = null;
+do {
+if (!iterator.hasNext()) {
+return false;
+}
+String next = iterator.next();
+TopicImage topicImage = image.getTopic(next);
+if (topicImage != null) {
+result = topicImage.id();
+}
+} while (result == null);
+next = result;
+return true;
+}
+
+@Override
+public Uuid next() {
+if (!hasNext()) throw new NoSuchElementException();
+Uuid result = next;
+next = null;
+return result;
+}
+}
+
+@Override
+public Iterator iterator() {
+return new TopicIdIterator(topicNames.iterator(), image);
+}
+
+@Override
+public Object[] toArray() {
+throw new UnsupportedOperationException();
+}
+
+@Override
+public T[] toArray(T[] a) {
+throw new UnsupportedOperationException();
+}
+
+@Override
+public boolean add(Uuid o) {
+throw new UnsupportedOperationException();
+}
+
+@Override
+public boolean remove(Object o) {
+throw new UnsupportedOperationException();
+}
+
+@Override
+public boolean addAll(Collection c) {
+throw new UnsupportedOperationException();
+}
+
+@Override
+public void clear() {
+throw new UnsupportedOperationException();
+}
+
+@Override
+public boolean removeAll(Collection c) {
+throw new UnsupportedOperationException();
+}
+
+@Override
+public boolean retainAll(Col
Re: [PR] KAFKA-16626: Lazily convert subscribed topic names to topic ids [kafka]
jeffkbkim commented on PR #15970: URL: https://github.com/apache/kafka/pull/15970#issuecomment-2123121864 @dajac thanks for the review. I have addressed your comments -- 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
Re: [PR] KAFKA-16626: Lazily convert subscribed topic names to topic ids [kafka]
jeffkbkim commented on code in PR #15970:
URL: https://github.com/apache/kafka/pull/15970#discussion_r1608704063
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/assignor/GeneralUniformAssignmentBuilderTest.java:
##
@@ -103,13 +114,13 @@ public void testTwoMembersSubscribedToNonexistentTopics()
{
members.put(memberA, new AssignmentMemberSpec(
Optional.empty(),
Optional.empty(),
-Collections.singletonList(topic3Uuid),
+new TopicIds(Collections.singleton(topic3Name), topicsImage),
Review Comment:
I removed all TopicIds initializations in the assignor tests.
--
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
Re: [PR] KAFKA-16626: Lazily convert subscribed topic names to topic ids [kafka]
dajac commented on code in PR #15970:
URL: https://github.com/apache/kafka/pull/15970#discussion_r1608552211
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/assignor/GeneralUniformAssignmentBuilderTest.java:
##
@@ -45,12 +48,20 @@ public class GeneralUniformAssignmentBuilderTest {
private final Uuid topic4Uuid = Uuid.fromString("T4-Tw9fVTLCz5HbPp6YQsa");
private final String topic1Name = "topic1";
private final String topic2Name = "topic2";
-private final String topic3Name = "topic3";
+private final String topic3Name = "topic3";
Review Comment:
nit: Indentation is off.
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/assignor/OptimizedUniformAssignmentBuilderTest.java:
##
@@ -133,13 +144,13 @@ public void
testFirstAssignmentTwoMembersTwoTopicsNoMemberRacks() {
members.put(memberA, new AssignmentMemberSpec(
Optional.empty(),
Optional.empty(),
-Arrays.asList(topic1Uuid, topic3Uuid),
+ new TopicIds(mkSet(topic1Name, topic3Name), topicsImage),
Review Comment:
Same question.
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/consumer/TopicIds.java:
##
@@ -0,0 +1,185 @@
+/*
+ * 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.consumer;
+
+import org.apache.kafka.common.Uuid;
+import org.apache.kafka.image.TopicImage;
+import org.apache.kafka.image.TopicsImage;
+
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.NoSuchElementException;
+import java.util.Objects;
+import java.util.Set;
+
+/**
+ * TopicIds is initialized with topic names (String) but exposes a Set of
topic ids (Uuid) to the
+ * user and performs the conversion lazily with TopicsImage.
+ */
+public class TopicIds implements Set {
Review Comment:
Should we add unit tests for this class?
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/assignor/GeneralUniformAssignmentBuilderTest.java:
##
@@ -103,13 +114,13 @@ public void testTwoMembersSubscribedToNonexistentTopics()
{
members.put(memberA, new AssignmentMemberSpec(
Optional.empty(),
Optional.empty(),
-Collections.singletonList(topic3Uuid),
+new TopicIds(Collections.singleton(topic3Name), topicsImage),
Review Comment:
Why do we need this change? It looks like the previous version would just
work, no?
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/assignor/RangeAssignorTest.java:
##
@@ -99,7 +108,7 @@ public void testOneConsumerSubscribedToNonExistentTopic() {
new AssignmentMemberSpec(
Optional.empty(),
Optional.empty(),
-Collections.singletonList(topic2Uuid),
+new TopicIds(Collections.singleton(topic2Name), topicsImage),
Review Comment:
Same question.
##
jmh-benchmarks/src/main/java/org/apache/kafka/jmh/assignor/ServerSideAssignorBenchmark.java:
##
@@ -119,7 +123,13 @@ public enum AssignmentType {
private SubscribedTopicDescriber subscribedTopicDescriber;
-private final List allTopicIds = new ArrayList<>();
+private final List allTopicNames = new ArrayList<>();
+
+private ImmutableMap topicsById = ImmutableMap.empty();
+
+private ImmutableMap topicsByName =
ImmutableMap.empty();
+
+private TopicsImage topicsImage = TopicsImage.EMPTY;
Review Comment:
Hum... I guess that we cannot. In this case, it may be better to build the
image the right way. We usually build it like we do in the MetadataImageBuilder.
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/consumer/TopicIds.java:
##
@@ -0,0 +1,185 @@
+/*
+ * 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 Li
Re: [PR] KAFKA-16626: Lazily convert subscribed topic names to topic ids [kafka]
jeffkbkim commented on PR #15970: URL: https://github.com/apache/kafka/pull/15970#issuecomment-2122882077 @dajac thanks for the review and the suggestions, i was a bit annoyed with the code to create those topics images. I have addressed the comments -- 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
Re: [PR] KAFKA-16626: Lazily convert subscribed topic names to topic ids [kafka]
jeffkbkim commented on code in PR #15970:
URL: https://github.com/apache/kafka/pull/15970#discussion_r1608522106
##
jmh-benchmarks/src/main/java/org/apache/kafka/jmh/assignor/ServerSideAssignorBenchmark.java:
##
@@ -119,7 +123,13 @@ public enum AssignmentType {
private SubscribedTopicDescriber subscribedTopicDescriber;
-private final List allTopicIds = new ArrayList<>();
+private final List allTopicNames = new ArrayList<>();
+
+private ImmutableMap topicsById = ImmutableMap.empty();
+
+private ImmutableMap topicsByName =
ImmutableMap.empty();
+
+private TopicsImage topicsImage = TopicsImage.EMPTY;
Review Comment:
I can't seem to use MetadataImageBuilder in jmh module as it is in the test
directory. Do you know whether we can go around that?
--
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
Re: [PR] KAFKA-16626: Lazily convert subscribed topic names to topic ids [kafka]
dajac commented on code in PR #15970:
URL: https://github.com/apache/kafka/pull/15970#discussion_r1607805094
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/AssignmentMemberSpec.java:
##
@@ -65,7 +64,7 @@ public Optional rackId() {
/**
* @return Collection of subscribed topic Ids.
Review Comment:
nit: Should we change `Collection` to `Set`?
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/assignor/OptimizedUniformAssignmentBuilderTest.java:
##
@@ -72,7 +101,7 @@ public void testOneMemberNoTopicSubscription() {
new AssignmentMemberSpec(
Optional.empty(),
Optional.empty(),
-Collections.emptyList(),
+TopicIds.EMPTY,
Review Comment:
ditto.
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/TopicIds.java:
##
@@ -0,0 +1,176 @@
+/*
+ * 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.assignor;
+
+import org.apache.kafka.common.Uuid;
+import org.apache.kafka.image.TopicImage;
+import org.apache.kafka.image.TopicsImage;
+
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Iterator;
+import java.util.NoSuchElementException;
+import java.util.Objects;
+import java.util.Set;
+
+/**
+ * TopicIds is initialized with topic names (String) but exposes a Set of
topic ids (Uuid) to the
+ * user and performs the conversion lazily with TopicsImage.
+ */
+public class TopicIds implements Set {
+
+public static final TopicIds EMPTY = new TopicIds(Collections.emptySet(),
TopicsImage.EMPTY);
+private final Set topicNames;
+private final TopicsImage image;
+
+public TopicIds(
+Set topicNames,
+TopicsImage image
+) {
+this.topicNames = topicNames;
+this.image = image;
Review Comment:
nit: Should we require them to be non-null?
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/assignor/RangeAssignorTest.java:
##
@@ -69,7 +98,7 @@ public void testOneConsumerNoTopic() {
new AssignmentMemberSpec(
Optional.empty(),
Optional.empty(),
-Collections.emptyList(),
+TopicIds.EMPTY,
Review Comment:
ditto.
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/assignor/GeneralUniformAssignmentBuilderTest.java:
##
@@ -69,13 +103,13 @@ public void testTwoMembersNoTopicSubscription() {
members.put(memberA, new AssignmentMemberSpec(
Optional.empty(),
Optional.empty(),
-Collections.emptyList(),
+TopicIds.EMPTY,
Review Comment:
Should we revert all the changes in this file? I don't think that there are
necessary as the assignor requires a Set.
##
jmh-benchmarks/src/main/java/org/apache/kafka/jmh/assignor/TargetAssignmentBuilderBenchmark.java:
##
@@ -123,13 +130,14 @@ private Map
generateMockMembers() {
private Map generateMockSubscriptionMetadata() {
Map subscriptionMetadata = new HashMap<>();
+ImmutableMap topicsById = ImmutableMap.empty();
+ImmutableMap topicsByName = ImmutableMap.empty();
Review Comment:
ditto.
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/TopicIds.java:
##
@@ -0,0 +1,176 @@
+/*
+ * 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
Re: [PR] KAFKA-16626: Lazily convert subscribed topic names to topic ids [kafka]
jeffkbkim commented on PR #15970: URL: https://github.com/apache/kafka/pull/15970#issuecomment-2118184548 RangeAssignor baseline ``` Benchmark (memberCount) (partitionsToMemberRatio) (topicCount) Mode CntScoreError Units TargetAssignmentBuilderBenchmark.build1 10 100 avgt5 235.398 ± 15.092 ms/op ``` RangeAssignor PR ``` Benchmark (memberCount) (partitionsToMemberRatio) (topicCount) Mode CntScoreError Units TargetAssignmentBuilderBenchmark.build1 10 100 avgt5 161.683 ± 22.739 ms/op ``` -- 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
Re: [PR] KAFKA-16626: Lazily convert subscribed topic names to topic ids [kafka]
jeffkbkim commented on code in PR #15970:
URL: https://github.com/apache/kafka/pull/15970#discussion_r1602316991
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/assignor/AssignorTestUtils.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.assignor;
+
+import org.apache.kafka.common.DirectoryId;
+import org.apache.kafka.common.Uuid;
+import org.apache.kafka.image.TopicImage;
+import org.apache.kafka.image.TopicsImage;
+import org.apache.kafka.metadata.LeaderRecoveryState;
+import org.apache.kafka.metadata.PartitionRegistration;
+import org.apache.kafka.server.immutable.ImmutableMap;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+public class AssignorTestUtils {
Review Comment:
I mainly created this class to copy the methods in
https://github.com/apache/kafka/blob/trunk/metadata/src/test/java/org/apache/kafka/image/TopicsImageTest.java#L70-L93
I was unable to access the methods in TopicsImageTest while playing around
with import-control-group-coordinator.xml. We currently allow
`org.apache.kafka.image` package so I'm not sure what is preventing it. Open to
suggestions
--
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
Re: [PR] KAFKA-16626: Lazily convert subscribed topic names to topic ids [kafka]
jeffkbkim commented on PR #15970: URL: https://github.com/apache/kafka/pull/15970#issuecomment-2113427888 TargetAssignmentBuilderBenchmark results (Baseline vs KAFKA-16626). The runs with high consumer group member count is noticeably better now. Baseline ``` Benchmark (memberCount) (partitionsToMemberRatio) (topicCount) Mode CntScoreError Units TargetAssignmentBuilderBenchmark.build 100 510 avgt50.181 ± 0.025 ms/op TargetAssignmentBuilderBenchmark.build 100 5 100 avgt50.483 ± 0.042 ms/op TargetAssignmentBuilderBenchmark.build 100 5 1000 avgt53.693 ± 0.074 ms/op TargetAssignmentBuilderBenchmark.build 100 1010 avgt50.287 ± 0.010 ms/op TargetAssignmentBuilderBenchmark.build 100 10 100 avgt50.634 ± 0.034 ms/op TargetAssignmentBuilderBenchmark.build 100 10 1000 avgt54.573 ± 0.127 ms/op TargetAssignmentBuilderBenchmark.build 100 5010 avgt51.016 ± 0.063 ms/op TargetAssignmentBuilderBenchmark.build 100 50 100 avgt51.960 ± 0.106 ms/op TargetAssignmentBuilderBenchmark.build 100 50 1000 avgt56.669 ± 0.257 ms/op TargetAssignmentBuilderBenchmark.build 500 510 avgt50.849 ± 0.083 ms/op TargetAssignmentBuilderBenchmark.build 500 5 100 avgt52.209 ± 0.139 ms/op TargetAssignmentBuilderBenchmark.build 500 5 1000 avgt5 22.515 ± 1.182 ms/op TargetAssignmentBuilderBenchmark.build 500 1010 avgt51.519 ± 0.181 ms/op TargetAssignmentBuilderBenchmark.build 500 10 100 avgt53.038 ± 0.291 ms/op TargetAssignmentBuilderBenchmark.build 500 10 1000 avgt5 23.761 ± 0.734 ms/op TargetAssignmentBuilderBenchmark.build 500 5010 avgt56.651 ± 0.785 ms/op TargetAssignmentBuilderBenchmark.build 500 50 100 avgt5 16.168 ± 0.939 ms/op TargetAssignmentBuilderBenchmark.build 500 50 1000 avgt5 39.315 ± 1.811 ms/op TargetAssignmentBuilderBenchmark.build 1000 510 avgt51.782 ± 0.193 ms/op TargetAssignmentBuilderBenchmark.build 1000 5 100 avgt54.886 ± 0.669 ms/op TargetAssignmentBuilderBenchmark.build 1000 5 1000 avgt5 37.605 ± 0.674 ms/op TargetAssignmentBuilderBenchmark.build 1000 1010 avgt53.982 ± 0.534 ms/op TargetAssignmentBuilderBenchmark.build 1000 10 100 avgt57.444 ± 0.411 ms/op TargetAssignmentBuilderBenchmark.build 1000 10 1000 avgt5 46.231 ± 1.616 ms/op TargetAssignmentBuilderBenchmark.build 1000 5010 avgt5 15.327 ± 1.394 ms/op TargetAssignmentBuilderBenchmark.build 1000 50 100 avgt5 29.014 ± 0.890 ms/op TargetAssignmentBuilderBenchmark.build 1000 50 1000 avgt5 76.194 ± 0.952 ms/op TargetAssignmentBuilderBenchmark.build 5000 510 avgt5 18.086 ± 2.724 ms/op TargetAssignmentBuilderBenchmark.build 5000 5 100 avgt5 31.667 ± 1.372 ms/op TargetAssignmentBuilderBenchmark.build 5000 5 1000 avgt5 208.516 ± 7.774 ms/op TargetAssignmentBuilderBenchmark.build 5000 1010 avgt5 25.929 ± 1.926 ms/op TargetAssignmentBuilderBenchmark.build 5000 10 100 avgt5 43.845 ± 1.089 ms/op TargetAssignmentBuilderBenchmark.build 5000
[PR] KAFKA-16626: Lazily convert subscribed topic names to topic ids [kafka]
jeffkbkim opened a new pull request, #15970: URL: https://github.com/apache/kafka/pull/15970 This patch aims to remove the data structure that stores the conversion from topic names to topic ids which was taking time similar to the actual assignment computation. Instead, we reuse the already existing ConsumerGroupMember.subscribedTopicNames() and do the conversion to topic ids when the iterator is requested. I have provided the results from trunk vs. this PR in the comments ### Committer Checklist (excluded from commit message) - [ ] Verify design and implementation - [ ] Verify test coverage and CI build status - [ ] Verify documentation (including upgrade notes) -- 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
