You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/09/08 20:36:31 UTC
[GitHub] [kafka] cmccabe opened a new pull request #11311: KAFKA-13280: Avoid O(N) behavior in KRaftMetadataCache#topicNamesToIds
cmccabe opened a new pull request #11311:
URL: https://github.com/apache/kafka/pull/11311
Avoid O(N) behavior in KRaftMetadataCache#topicNamesToIds and
KRaftMetadataCache#topicIdsToNames by returning a map subclass that
exposes the TopicsImage data structures without copying them.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #11311: KAFKA-13280: Avoid O(N) behavior in KRaftMetadataCache#topicNamesToIds
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #11311:
URL: https://github.com/apache/kafka/pull/11311#discussion_r711026024
##########
File path: metadata/src/main/java/org/apache/kafka/image/TopicsImage.java
##########
@@ -92,6 +98,148 @@ public int hashCode() {
return Objects.hash(topicsById, topicsByName);
}
+ /**
+ * Expose a view of this TopicsImage as a map from topic names to IDs.
+ *
+ * Like TopicsImage itself, this map is immutable.
+ */
+ public Map<String, Uuid> topicNameToIdView() {
+ return new TopicNameToIdMap();
+ }
+
+ class TopicNameToIdMap extends AbstractMap<String, Uuid> {
+ private final TopicNameToIdMapEntrySet set = new TopicNameToIdMapEntrySet();
+
+ @Override
+ public boolean containsKey(Object key) {
+ return topicsByName.containsKey(key);
+ }
+
+ @Override
+ public Uuid get(Object key) {
+ TopicImage image = topicsByName.get(key);
+ if (image == null) return null;
+ return image.id();
+ }
+
+ @Override
+ public Set<Entry<String, Uuid>> entrySet() {
+ return set;
+ }
+ }
+
+ class TopicNameToIdMapEntrySet extends AbstractSet<Entry<String, Uuid>> {
+ @Override
+ public Iterator<Entry<String, Uuid>> iterator() {
+ return new TopicNameToIdMapEntrySetIterator(topicsByName.entrySet().iterator());
+ }
+
+ @SuppressWarnings("rawtypes")
+ @Override
+ public boolean contains(Object o) {
+ if (!(o instanceof Entry)) return false;
+ Entry other = (Entry) o;
+ TopicImage image = topicsByName.get(other.getKey());
+ if (image == null) return false;
+ return image.id().equals(other.getValue());
+ }
+
+ @Override
+ public int size() {
+ return topicsByName.size();
+ }
+ }
+
+ static class TopicNameToIdMapEntrySetIterator implements Iterator<Entry<String, Uuid>> {
+ private final Iterator<Entry<String, TopicImage>> iterator;
+
+ TopicNameToIdMapEntrySetIterator(Iterator<Entry<String, TopicImage>> iterator) {
+ this.iterator = iterator;
+ }
+
+ @Override
+ public boolean hasNext() {
+ return this.iterator.hasNext();
+ }
+
+ @Override
+ public Entry<String, Uuid> next() {
+ Entry<String, TopicImage> entry = iterator.next();
+ return new SimpleImmutableEntry<>(entry.getKey(), entry.getValue().id());
Review comment:
Does this mean that repeated iterations are more expensive since they require allocating these entries?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma removed a comment on pull request #11311: KAFKA-13280: Avoid O(N) behavior in KRaftMetadataCache#topicNamesToIds
Posted by GitBox <gi...@apache.org>.
ijuma removed a comment on pull request #11311:
URL: https://github.com/apache/kafka/pull/11311#issuecomment-921768127
Why do we need to use these classes from Java?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #11311: KAFKA-13280: Avoid O(N) behavior in KRaftMetadataCache#topicNamesToIds
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #11311:
URL: https://github.com/apache/kafka/pull/11311#discussion_r712456110
##########
File path: metadata/src/main/java/org/apache/kafka/image/TopicsImage.java
##########
@@ -92,6 +98,148 @@ public int hashCode() {
return Objects.hash(topicsById, topicsByName);
}
+ /**
+ * Expose a view of this TopicsImage as a map from topic names to IDs.
+ *
+ * Like TopicsImage itself, this map is immutable.
+ */
+ public Map<String, Uuid> topicNameToIdView() {
+ return new TopicNameToIdMap();
+ }
+
+ class TopicNameToIdMap extends AbstractMap<String, Uuid> {
+ private final TopicNameToIdMapEntrySet set = new TopicNameToIdMapEntrySet();
+
+ @Override
+ public boolean containsKey(Object key) {
+ return topicsByName.containsKey(key);
+ }
+
+ @Override
+ public Uuid get(Object key) {
+ TopicImage image = topicsByName.get(key);
+ if (image == null) return null;
+ return image.id();
+ }
+
+ @Override
+ public Set<Entry<String, Uuid>> entrySet() {
+ return set;
+ }
+ }
+
+ class TopicNameToIdMapEntrySet extends AbstractSet<Entry<String, Uuid>> {
+ @Override
+ public Iterator<Entry<String, Uuid>> iterator() {
+ return new TopicNameToIdMapEntrySetIterator(topicsByName.entrySet().iterator());
+ }
+
+ @SuppressWarnings("rawtypes")
+ @Override
+ public boolean contains(Object o) {
+ if (!(o instanceof Entry)) return false;
+ Entry other = (Entry) o;
+ TopicImage image = topicsByName.get(other.getKey());
+ if (image == null) return false;
+ return image.id().equals(other.getValue());
+ }
+
+ @Override
+ public int size() {
+ return topicsByName.size();
+ }
+ }
+
+ static class TopicNameToIdMapEntrySetIterator implements Iterator<Entry<String, Uuid>> {
+ private final Iterator<Entry<String, TopicImage>> iterator;
+
+ TopicNameToIdMapEntrySetIterator(Iterator<Entry<String, TopicImage>> iterator) {
+ this.iterator = iterator;
+ }
+
+ @Override
+ public boolean hasNext() {
+ return this.iterator.hasNext();
+ }
+
+ @Override
+ public Entry<String, Uuid> next() {
+ Entry<String, TopicImage> entry = iterator.next();
+ return new SimpleImmutableEntry<>(entry.getKey(), entry.getValue().id());
Review comment:
Hmm... my understanding is that allocating a short-lived temporary object is typically almost free, unless the number of objects you allocate overwhelms the eden space in the GC. And people are not supposed to be doing O(num_topics) iterations anyway (if they are, that is a bug)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #11311: KAFKA-13280: Avoid O(N) behavior in KRaftMetadataCache#topicNamesToIds
Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #11311:
URL: https://github.com/apache/kafka/pull/11311#issuecomment-925086878
I filed https://issues.apache.org/jira/browse/KAFKA-13318 to create JMH benchmarks for this and other cache operations. For now, I think we should commit this to fix the O(N) behavior regression...
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe merged pull request #11311: KAFKA-13280: Avoid O(N) behavior in KRaftMetadataCache#topicNamesToIds
Posted by GitBox <gi...@apache.org>.
cmccabe merged pull request #11311:
URL: https://github.com/apache/kafka/pull/11311
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #11311: KAFKA-13280: Avoid O(N) behavior in KRaftMetadataCache#topicNamesToIds
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #11311:
URL: https://github.com/apache/kafka/pull/11311#discussion_r712521701
##########
File path: metadata/src/main/java/org/apache/kafka/image/TopicsImage.java
##########
@@ -92,6 +98,148 @@ public int hashCode() {
return Objects.hash(topicsById, topicsByName);
}
+ /**
+ * Expose a view of this TopicsImage as a map from topic names to IDs.
+ *
+ * Like TopicsImage itself, this map is immutable.
+ */
+ public Map<String, Uuid> topicNameToIdView() {
+ return new TopicNameToIdMap();
+ }
+
+ class TopicNameToIdMap extends AbstractMap<String, Uuid> {
+ private final TopicNameToIdMapEntrySet set = new TopicNameToIdMapEntrySet();
+
+ @Override
+ public boolean containsKey(Object key) {
+ return topicsByName.containsKey(key);
+ }
+
+ @Override
+ public Uuid get(Object key) {
+ TopicImage image = topicsByName.get(key);
+ if (image == null) return null;
+ return image.id();
+ }
+
+ @Override
+ public Set<Entry<String, Uuid>> entrySet() {
+ return set;
+ }
+ }
+
+ class TopicNameToIdMapEntrySet extends AbstractSet<Entry<String, Uuid>> {
+ @Override
+ public Iterator<Entry<String, Uuid>> iterator() {
+ return new TopicNameToIdMapEntrySetIterator(topicsByName.entrySet().iterator());
+ }
+
+ @SuppressWarnings("rawtypes")
+ @Override
+ public boolean contains(Object o) {
+ if (!(o instanceof Entry)) return false;
+ Entry other = (Entry) o;
+ TopicImage image = topicsByName.get(other.getKey());
+ if (image == null) return false;
+ return image.id().equals(other.getValue());
+ }
+
+ @Override
+ public int size() {
+ return topicsByName.size();
+ }
+ }
+
+ static class TopicNameToIdMapEntrySetIterator implements Iterator<Entry<String, Uuid>> {
+ private final Iterator<Entry<String, TopicImage>> iterator;
+
+ TopicNameToIdMapEntrySetIterator(Iterator<Entry<String, TopicImage>> iterator) {
+ this.iterator = iterator;
+ }
+
+ @Override
+ public boolean hasNext() {
+ return this.iterator.hasNext();
+ }
+
+ @Override
+ public Entry<String, Uuid> next() {
+ Entry<String, TopicImage> entry = iterator.next();
+ return new SimpleImmutableEntry<>(entry.getKey(), entry.getValue().id());
+ }
+ }
+
+ /**
+ * Expose a view of this TopicsImage as a map from IDs to names.
+ *
+ * Like TopicsImage itself, this map is immutable.
+ */
+ public Map<Uuid, String> topicIdToNameView() {
+ return new TopicIdToNameMap();
+ }
+
+ class TopicIdToNameMap extends AbstractMap<Uuid, String> {
+ private final TopicIdToNameMapEntrySet set = new TopicIdToNameMapEntrySet();
+
+ @Override
+ public boolean containsKey(Object key) {
+ return topicsById.containsKey(key);
+ }
+
+ @Override
+ public String get(Object key) {
+ TopicImage image = topicsById.get(key);
+ if (image == null) return null;
+ return image.name();
+ }
+
+ @Override
+ public Set<Entry<Uuid, String>> entrySet() {
+ return set;
+ }
+ }
+
+ class TopicIdToNameMapEntrySet extends AbstractSet<Entry<Uuid, String>> {
Review comment:
That's basically `map` right?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #11311: KAFKA-13280: Avoid O(N) behavior in KRaftMetadataCache#topicNamesToIds
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #11311:
URL: https://github.com/apache/kafka/pull/11311#discussion_r712456110
##########
File path: metadata/src/main/java/org/apache/kafka/image/TopicsImage.java
##########
@@ -92,6 +98,148 @@ public int hashCode() {
return Objects.hash(topicsById, topicsByName);
}
+ /**
+ * Expose a view of this TopicsImage as a map from topic names to IDs.
+ *
+ * Like TopicsImage itself, this map is immutable.
+ */
+ public Map<String, Uuid> topicNameToIdView() {
+ return new TopicNameToIdMap();
+ }
+
+ class TopicNameToIdMap extends AbstractMap<String, Uuid> {
+ private final TopicNameToIdMapEntrySet set = new TopicNameToIdMapEntrySet();
+
+ @Override
+ public boolean containsKey(Object key) {
+ return topicsByName.containsKey(key);
+ }
+
+ @Override
+ public Uuid get(Object key) {
+ TopicImage image = topicsByName.get(key);
+ if (image == null) return null;
+ return image.id();
+ }
+
+ @Override
+ public Set<Entry<String, Uuid>> entrySet() {
+ return set;
+ }
+ }
+
+ class TopicNameToIdMapEntrySet extends AbstractSet<Entry<String, Uuid>> {
+ @Override
+ public Iterator<Entry<String, Uuid>> iterator() {
+ return new TopicNameToIdMapEntrySetIterator(topicsByName.entrySet().iterator());
+ }
+
+ @SuppressWarnings("rawtypes")
+ @Override
+ public boolean contains(Object o) {
+ if (!(o instanceof Entry)) return false;
+ Entry other = (Entry) o;
+ TopicImage image = topicsByName.get(other.getKey());
+ if (image == null) return false;
+ return image.id().equals(other.getValue());
+ }
+
+ @Override
+ public int size() {
+ return topicsByName.size();
+ }
+ }
+
+ static class TopicNameToIdMapEntrySetIterator implements Iterator<Entry<String, Uuid>> {
+ private final Iterator<Entry<String, TopicImage>> iterator;
+
+ TopicNameToIdMapEntrySetIterator(Iterator<Entry<String, TopicImage>> iterator) {
+ this.iterator = iterator;
+ }
+
+ @Override
+ public boolean hasNext() {
+ return this.iterator.hasNext();
+ }
+
+ @Override
+ public Entry<String, Uuid> next() {
+ Entry<String, TopicImage> entry = iterator.next();
+ return new SimpleImmutableEntry<>(entry.getKey(), entry.getValue().id());
Review comment:
Allocating a short-lived temporary object is typically almost free, unless the number of objects you allocate overwhelms the eden space in the GC. And people are not supposed to be doing O(num_topics) iterations anyway (if they are, that is a bug)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #11311: KAFKA-13280: Avoid O(N) behavior in KRaftMetadataCache#topicNamesToIds
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #11311:
URL: https://github.com/apache/kafka/pull/11311#discussion_r713235847
##########
File path: metadata/src/main/java/org/apache/kafka/image/TopicsImage.java
##########
@@ -92,6 +98,148 @@ public int hashCode() {
return Objects.hash(topicsById, topicsByName);
}
+ /**
+ * Expose a view of this TopicsImage as a map from topic names to IDs.
+ *
+ * Like TopicsImage itself, this map is immutable.
+ */
+ public Map<String, Uuid> topicNameToIdView() {
+ return new TopicNameToIdMap();
+ }
+
+ class TopicNameToIdMap extends AbstractMap<String, Uuid> {
+ private final TopicNameToIdMapEntrySet set = new TopicNameToIdMapEntrySet();
+
+ @Override
+ public boolean containsKey(Object key) {
+ return topicsByName.containsKey(key);
+ }
+
+ @Override
+ public Uuid get(Object key) {
+ TopicImage image = topicsByName.get(key);
+ if (image == null) return null;
+ return image.id();
+ }
+
+ @Override
+ public Set<Entry<String, Uuid>> entrySet() {
+ return set;
+ }
+ }
+
+ class TopicNameToIdMapEntrySet extends AbstractSet<Entry<String, Uuid>> {
+ @Override
+ public Iterator<Entry<String, Uuid>> iterator() {
+ return new TopicNameToIdMapEntrySetIterator(topicsByName.entrySet().iterator());
+ }
+
+ @SuppressWarnings("rawtypes")
+ @Override
+ public boolean contains(Object o) {
+ if (!(o instanceof Entry)) return false;
+ Entry other = (Entry) o;
+ TopicImage image = topicsByName.get(other.getKey());
+ if (image == null) return false;
+ return image.id().equals(other.getValue());
+ }
+
+ @Override
+ public int size() {
+ return topicsByName.size();
+ }
+ }
+
+ static class TopicNameToIdMapEntrySetIterator implements Iterator<Entry<String, Uuid>> {
+ private final Iterator<Entry<String, TopicImage>> iterator;
+
+ TopicNameToIdMapEntrySetIterator(Iterator<Entry<String, TopicImage>> iterator) {
+ this.iterator = iterator;
+ }
+
+ @Override
+ public boolean hasNext() {
+ return this.iterator.hasNext();
+ }
+
+ @Override
+ public Entry<String, Uuid> next() {
+ Entry<String, TopicImage> entry = iterator.next();
+ return new SimpleImmutableEntry<>(entry.getKey(), entry.getValue().id());
Review comment:
The existing code literally copies the entire map every time. Copying the entire map is going to be more expensive than copying just the Entry elements (although again, this is something we should not be doing, so not worth optimizing)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on pull request #11311: KAFKA-13280: Avoid O(N) behavior in KRaftMetadataCache#topicNamesToIds
Posted by GitBox <gi...@apache.org>.
hachikuji commented on pull request #11311:
URL: https://github.com/apache/kafka/pull/11311#issuecomment-922028929
@cmccabe Note also that there are checkstyle errors.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #11311: KAFKA-13280: Avoid O(N) behavior in KRaftMetadataCache#topicNamesToIds
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #11311:
URL: https://github.com/apache/kafka/pull/11311#discussion_r711275100
##########
File path: metadata/src/test/java/org/apache/kafka/image/TopicsImageTest.java
##########
@@ -375,4 +385,38 @@ private void testToImageAndBack(TopicsImage image) throws Throwable {
TopicsImage nextImage = delta.apply();
assertEquals(image, nextImage);
}
+
+ @Test
+ public void testTopicNameToIdView() {
+ Map<String, Uuid> map = IMAGE1.topicNameToIdView();
+ assertTrue(map.containsKey("foo"));
+ assertEquals(FOO_UUID, map.get("foo"));
+ assertTrue(map.containsKey("bar"));
+ assertEquals(BAR_UUID, map.get("bar"));
+ assertFalse(map.containsKey("baz"));
+ assertEquals(null, map.get("baz"));
+ HashSet<Uuid> uuids = new HashSet<>();
+ map.values().iterator().forEachRemaining(u -> uuids.add(u));
+ HashSet<Uuid> expectedUuids = new HashSet<>(Arrays.asList(
+ Uuid.fromString("ThIaNwRnSM2Nt9Mx1v0RvA"),
+ Uuid.fromString("f62ptyETTjet8SL5ZeREiw")));
+ assertEquals(expectedUuids, uuids);
+ assertThrows(UnsupportedOperationException.class, () -> map.remove("foo"));
Review comment:
Perhaps a similar assertion for `map.put`? Ditto below.
##########
File path: metadata/src/main/java/org/apache/kafka/image/TopicsImage.java
##########
@@ -92,6 +98,148 @@ public int hashCode() {
return Objects.hash(topicsById, topicsByName);
}
+ /**
+ * Expose a view of this TopicsImage as a map from topic names to IDs.
+ *
+ * Like TopicsImage itself, this map is immutable.
+ */
+ public Map<String, Uuid> topicNameToIdView() {
+ return new TopicNameToIdMap();
+ }
+
+ class TopicNameToIdMap extends AbstractMap<String, Uuid> {
+ private final TopicNameToIdMapEntrySet set = new TopicNameToIdMapEntrySet();
+
+ @Override
+ public boolean containsKey(Object key) {
+ return topicsByName.containsKey(key);
+ }
+
+ @Override
+ public Uuid get(Object key) {
+ TopicImage image = topicsByName.get(key);
+ if (image == null) return null;
+ return image.id();
+ }
+
+ @Override
+ public Set<Entry<String, Uuid>> entrySet() {
+ return set;
+ }
+ }
+
+ class TopicNameToIdMapEntrySet extends AbstractSet<Entry<String, Uuid>> {
+ @Override
+ public Iterator<Entry<String, Uuid>> iterator() {
+ return new TopicNameToIdMapEntrySetIterator(topicsByName.entrySet().iterator());
+ }
+
+ @SuppressWarnings("rawtypes")
+ @Override
+ public boolean contains(Object o) {
+ if (!(o instanceof Entry)) return false;
+ Entry other = (Entry) o;
+ TopicImage image = topicsByName.get(other.getKey());
+ if (image == null) return false;
+ return image.id().equals(other.getValue());
+ }
+
+ @Override
+ public int size() {
+ return topicsByName.size();
+ }
+ }
+
+ static class TopicNameToIdMapEntrySetIterator implements Iterator<Entry<String, Uuid>> {
+ private final Iterator<Entry<String, TopicImage>> iterator;
+
+ TopicNameToIdMapEntrySetIterator(Iterator<Entry<String, TopicImage>> iterator) {
+ this.iterator = iterator;
+ }
+
+ @Override
+ public boolean hasNext() {
+ return this.iterator.hasNext();
+ }
+
+ @Override
+ public Entry<String, Uuid> next() {
+ Entry<String, TopicImage> entry = iterator.next();
+ return new SimpleImmutableEntry<>(entry.getKey(), entry.getValue().id());
+ }
+ }
+
+ /**
+ * Expose a view of this TopicsImage as a map from IDs to names.
+ *
+ * Like TopicsImage itself, this map is immutable.
+ */
+ public Map<Uuid, String> topicIdToNameView() {
+ return new TopicIdToNameMap();
+ }
+
+ class TopicIdToNameMap extends AbstractMap<Uuid, String> {
+ private final TopicIdToNameMapEntrySet set = new TopicIdToNameMapEntrySet();
+
+ @Override
+ public boolean containsKey(Object key) {
+ return topicsById.containsKey(key);
+ }
+
+ @Override
+ public String get(Object key) {
+ TopicImage image = topicsById.get(key);
+ if (image == null) return null;
+ return image.name();
+ }
+
+ @Override
+ public Set<Entry<Uuid, String>> entrySet() {
+ return set;
+ }
+ }
+
+ class TopicIdToNameMapEntrySet extends AbstractSet<Entry<Uuid, String>> {
Review comment:
There is probably a generic pattern that you could factor out here. Basically you are providing a translation from `Map<K, V1>` to `Map<K, V2>` in both of these cases. So you could construct a generic `MapView` or something like that with a `Function<V1, V2>` and save some of the duplication.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #11311: KAFKA-13280: Avoid O(N) behavior in KRaftMetadataCache#topicNamesToIds
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #11311:
URL: https://github.com/apache/kafka/pull/11311#discussion_r712520803
##########
File path: metadata/src/main/java/org/apache/kafka/image/TopicsImage.java
##########
@@ -92,6 +98,148 @@ public int hashCode() {
return Objects.hash(topicsById, topicsByName);
}
+ /**
+ * Expose a view of this TopicsImage as a map from topic names to IDs.
+ *
+ * Like TopicsImage itself, this map is immutable.
+ */
+ public Map<String, Uuid> topicNameToIdView() {
+ return new TopicNameToIdMap();
+ }
+
+ class TopicNameToIdMap extends AbstractMap<String, Uuid> {
+ private final TopicNameToIdMapEntrySet set = new TopicNameToIdMapEntrySet();
+
+ @Override
+ public boolean containsKey(Object key) {
+ return topicsByName.containsKey(key);
+ }
+
+ @Override
+ public Uuid get(Object key) {
+ TopicImage image = topicsByName.get(key);
+ if (image == null) return null;
+ return image.id();
+ }
+
+ @Override
+ public Set<Entry<String, Uuid>> entrySet() {
+ return set;
+ }
+ }
+
+ class TopicNameToIdMapEntrySet extends AbstractSet<Entry<String, Uuid>> {
+ @Override
+ public Iterator<Entry<String, Uuid>> iterator() {
+ return new TopicNameToIdMapEntrySetIterator(topicsByName.entrySet().iterator());
+ }
+
+ @SuppressWarnings("rawtypes")
+ @Override
+ public boolean contains(Object o) {
+ if (!(o instanceof Entry)) return false;
+ Entry other = (Entry) o;
+ TopicImage image = topicsByName.get(other.getKey());
+ if (image == null) return false;
+ return image.id().equals(other.getValue());
+ }
+
+ @Override
+ public int size() {
+ return topicsByName.size();
+ }
+ }
+
+ static class TopicNameToIdMapEntrySetIterator implements Iterator<Entry<String, Uuid>> {
+ private final Iterator<Entry<String, TopicImage>> iterator;
+
+ TopicNameToIdMapEntrySetIterator(Iterator<Entry<String, TopicImage>> iterator) {
+ this.iterator = iterator;
+ }
+
+ @Override
+ public boolean hasNext() {
+ return this.iterator.hasNext();
+ }
+
+ @Override
+ public Entry<String, Uuid> next() {
+ Entry<String, TopicImage> entry = iterator.next();
+ return new SimpleImmutableEntry<>(entry.getKey(), entry.getValue().id());
Review comment:
We don't know if the allocations are short-lived since it depends on what the caller does with them. In the best case, they `Entry` is immediately discarded and the JVM elides the allocation. But escape analysis is a bit hit and miss. If the allocation does happen, then short-lived allocations are cheaper (the cost of the collection is proportional to the live objects remaining typically).
If you say that we generally don't iterate, then it's not an issue, but how can we make it data driven? Do we have an existing benchmark where we can show reduced allocation?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe merged pull request #11311: KAFKA-13280: Avoid O(N) behavior in KRaftMetadataCache#topicNamesToIds
Posted by GitBox <gi...@apache.org>.
cmccabe merged pull request #11311:
URL: https://github.com/apache/kafka/pull/11311
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #11311: KAFKA-13280: Avoid O(N) behavior in KRaftMetadataCache#topicNamesToIds
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #11311:
URL: https://github.com/apache/kafka/pull/11311#issuecomment-921768127
Why do we need to use these classes from Java?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #11311: KAFKA-13280: Avoid O(N) behavior in KRaftMetadataCache#topicNamesToIds
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #11311:
URL: https://github.com/apache/kafka/pull/11311#discussion_r712471886
##########
File path: metadata/src/main/java/org/apache/kafka/image/TopicsImage.java
##########
@@ -92,6 +98,148 @@ public int hashCode() {
return Objects.hash(topicsById, topicsByName);
}
+ /**
+ * Expose a view of this TopicsImage as a map from topic names to IDs.
+ *
+ * Like TopicsImage itself, this map is immutable.
+ */
+ public Map<String, Uuid> topicNameToIdView() {
+ return new TopicNameToIdMap();
+ }
+
+ class TopicNameToIdMap extends AbstractMap<String, Uuid> {
+ private final TopicNameToIdMapEntrySet set = new TopicNameToIdMapEntrySet();
+
+ @Override
+ public boolean containsKey(Object key) {
+ return topicsByName.containsKey(key);
+ }
+
+ @Override
+ public Uuid get(Object key) {
+ TopicImage image = topicsByName.get(key);
+ if (image == null) return null;
+ return image.id();
+ }
+
+ @Override
+ public Set<Entry<String, Uuid>> entrySet() {
+ return set;
+ }
+ }
+
+ class TopicNameToIdMapEntrySet extends AbstractSet<Entry<String, Uuid>> {
+ @Override
+ public Iterator<Entry<String, Uuid>> iterator() {
+ return new TopicNameToIdMapEntrySetIterator(topicsByName.entrySet().iterator());
+ }
+
+ @SuppressWarnings("rawtypes")
+ @Override
+ public boolean contains(Object o) {
+ if (!(o instanceof Entry)) return false;
+ Entry other = (Entry) o;
+ TopicImage image = topicsByName.get(other.getKey());
+ if (image == null) return false;
+ return image.id().equals(other.getValue());
+ }
+
+ @Override
+ public int size() {
+ return topicsByName.size();
+ }
+ }
+
+ static class TopicNameToIdMapEntrySetIterator implements Iterator<Entry<String, Uuid>> {
+ private final Iterator<Entry<String, TopicImage>> iterator;
+
+ TopicNameToIdMapEntrySetIterator(Iterator<Entry<String, TopicImage>> iterator) {
+ this.iterator = iterator;
+ }
+
+ @Override
+ public boolean hasNext() {
+ return this.iterator.hasNext();
+ }
+
+ @Override
+ public Entry<String, Uuid> next() {
+ Entry<String, TopicImage> entry = iterator.next();
+ return new SimpleImmutableEntry<>(entry.getKey(), entry.getValue().id());
+ }
+ }
+
+ /**
+ * Expose a view of this TopicsImage as a map from IDs to names.
+ *
+ * Like TopicsImage itself, this map is immutable.
+ */
+ public Map<Uuid, String> topicIdToNameView() {
+ return new TopicIdToNameMap();
+ }
+
+ class TopicIdToNameMap extends AbstractMap<Uuid, String> {
+ private final TopicIdToNameMapEntrySet set = new TopicIdToNameMapEntrySet();
+
+ @Override
+ public boolean containsKey(Object key) {
+ return topicsById.containsKey(key);
+ }
+
+ @Override
+ public String get(Object key) {
+ TopicImage image = topicsById.get(key);
+ if (image == null) return null;
+ return image.name();
+ }
+
+ @Override
+ public Set<Entry<Uuid, String>> entrySet() {
+ return set;
+ }
+ }
+
+ class TopicIdToNameMapEntrySet extends AbstractSet<Entry<Uuid, String>> {
Review comment:
Yeah, I thought about that, but with only two cases, it didn't seem worth it. If I have to add a third case, then I'll try to make something generic, I guess.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org