[GitHub] [kafka] rondagostino commented on a diff in pull request #13280: KAFKA-14735: Improve KRaft metadata image change performance at high …
rondagostino commented on code in PR #13280:
URL: https://github.com/apache/kafka/pull/13280#discussion_r1169270141
##
metadata/src/main/java/org/apache/kafka/image/TopicsImage.java:
##
@@ -76,8 +84,8 @@ public TopicImage getTopic(String name) {
}
public void write(ImageWriter writer, ImageWriterOptions options) {
-for (TopicImage topicImage : topicsById.values()) {
-topicImage.write(writer, options);
+for (Map.Entry entry : topicsById.entrySet()) {
+entry.getValue().write(writer, options);
Review Comment:
Yeah, it was in the Paguro implementation where `values()` was deprecated.
We've moved away from that, but I left the code change in that stopped using
the `values()` collection. This change doesn't harm anything.
--
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] rondagostino commented on a diff in pull request #13280: KAFKA-14735: Improve KRaft metadata image change performance at high …
rondagostino commented on code in PR #13280:
URL: https://github.com/apache/kafka/pull/13280#discussion_r1162184665
##
server-common/src/main/java/org/apache/kafka/server/immutable/ImmutableMap.java:
##
@@ -0,0 +1,47 @@
+/*
+ * 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.server.immutable;
+
+import java.util.Map;
+
+/**
+ * A persistent Hash-based Map wrapper.
+ * java.util.Map methods that mutate in-place will throw
UnsupportedOperationException
+ *
+ * @param the key type
+ * @param the value type
+ */
+public interface ImmutableMap extends Map {
+/**
+ * @return the underlying persistent map
+ */
+Object underlying();
Review Comment:
I removed 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] rondagostino commented on a diff in pull request #13280: KAFKA-14735: Improve KRaft metadata image change performance at high …
rondagostino commented on code in PR #13280:
URL: https://github.com/apache/kafka/pull/13280#discussion_r1162184505
##
server-common/src/main/java/org/apache/kafka/server/immutable/ImmutableMap.java:
##
@@ -0,0 +1,47 @@
+/*
+ * 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.server.immutable;
+
+import java.util.Map;
+
+/**
+ * A persistent Hash-based Map wrapper.
+ * java.util.Map methods that mutate in-place will throw
UnsupportedOperationException
+ *
+ * @param the key type
+ * @param the value type
+ */
+public interface ImmutableMap extends Map {
Review Comment:
I updated it. I removed the factory in favor of static methods on the
interface.
--
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] rondagostino commented on a diff in pull request #13280: KAFKA-14735: Improve KRaft metadata image change performance at high …
rondagostino commented on code in PR #13280:
URL: https://github.com/apache/kafka/pull/13280#discussion_r1161329362
##
server-common/src/main/java/org/apache/kafka/server/immutable/ImmutableMap.java:
##
@@ -0,0 +1,47 @@
+/*
+ * 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.server.immutable;
+
+import java.util.Map;
+
+/**
+ * A persistent Hash-based Map wrapper.
+ * java.util.Map methods that mutate in-place will throw
UnsupportedOperationException
+ *
+ * @param the key type
+ * @param the value type
+ */
+public interface ImmutableMap extends Map {
Review Comment:
If we add static methods here then those methods would have to hard-code
which underlying library to use. Right now we just have PCollections because
it meets a mandatory requirement for the ACL lock removal work, but it is
possible we might find either a need or a significant performance advantage to
using something else for some use case. Any static methods here might have to
have the underlying library in the name. For example, a method name here might
be `emptyMapPCollections()` (or `pCollectionsEmptyMap()`. I assumed it was
easy for clients to just explicitly invoke `PCOLLECTIONS_FACTORY.emptySet()`
(for example) -- or, more likely, they would have access to the factory already
(like `TopicsImage` does as it declares `private static final
ImmutableMapSetFactory FACTORY = ImmutableMapSetFactory.PCOLLECTIONS_FACTORY;`)
and could invoke `FACTORY.emptySet()`.
--
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] rondagostino commented on a diff in pull request #13280: KAFKA-14735: Improve KRaft metadata image change performance at high …
rondagostino commented on code in PR #13280:
URL: https://github.com/apache/kafka/pull/13280#discussion_r1161328104
##
server-common/src/main/java/org/apache/kafka/server/immutable/ImmutableMap.java:
##
@@ -0,0 +1,47 @@
+/*
+ * 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.server.immutable;
+
+import java.util.Map;
+
+/**
+ * A persistent Hash-based Map wrapper.
+ * java.util.Map methods that mutate in-place will throw
UnsupportedOperationException
+ *
+ * @param the key type
+ * @param the value type
+ */
+public interface ImmutableMap extends Map {
+/**
+ * @return the underlying persistent map
+ */
+Object underlying();
Review Comment:
> Is there any need to expose the underlying object?
I added it as an escape hatch. We could make it private for now, though --
it's easy to expose it later if necessary.
--
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] rondagostino commented on a diff in pull request #13280: KAFKA-14735: Improve KRaft metadata image change performance at high …
rondagostino commented on code in PR #13280:
URL: https://github.com/apache/kafka/pull/13280#discussion_r1114692109
##
metadata/src/main/java/org/apache/kafka/image/TopicsImage.java:
##
@@ -38,15 +40,21 @@
*/
public final class TopicsImage {
public static final TopicsImage EMPTY =
-new TopicsImage(Collections.emptyMap(), Collections.emptyMap());
+new TopicsImage(map(), map());
+
+final ImMap topicsById;
+final ImMap topicsByName;
-private final Map topicsById;
-private final Map topicsByName;
+public TopicsImage(ImMap topicsById,
+ ImMap topicsByName) {
+this.topicsById = topicsById;
+this.topicsByName = topicsByName;
+}
-public TopicsImage(Map topicsById,
- Map topicsByName) {
-this.topicsById = Collections.unmodifiableMap(topicsById);
-this.topicsByName = Collections.unmodifiableMap(topicsByName);
+public TopicsImage including(TopicImage topic) {
Review Comment:
It is used from a test class in the `core` module so it needs to be public
:-(
--
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] rondagostino commented on a diff in pull request #13280: KAFKA-14735: Improve KRaft metadata image change performance at high …
rondagostino commented on code in PR #13280:
URL: https://github.com/apache/kafka/pull/13280#discussion_r1114679640
##
metadata/src/main/java/org/apache/kafka/image/TopicsImage.java:
##
@@ -76,8 +84,8 @@ public TopicImage getTopic(String name) {
}
public void write(ImageWriter writer, ImageWriterOptions options) {
-for (TopicImage topicImage : topicsById.values()) {
-topicImage.write(writer, options);
+for (Map.Entry entry : topicsById.entrySet()) {
+entry.getValue().write(writer, options);
Review Comment:
We could, but it is marked deprecated in the library because there is no way
to provide a reasonable `.equals()` method. I actually checked, and indeed it
is true:
```
@Test
public void testMapValuesEquality() {
Map m = new HashMap<>();
m.put("a", "a");
m.put("b", "b");
assertEquals(m.keySet(), new HashSet<>(Arrays.asList("a", "b")));
assertEquals(m.keySet(), new HashSet<>(Arrays.asList("b", "a")));
// note that these all assert inequality
assertNotEquals(m.values(), new HashSet<>(Arrays.asList("a", "b")));
assertNotEquals(m.values(), Arrays.asList("a", "b"));
assertNotEquals(m.values(), new HashSet<>(Arrays.asList("b", "a")));
assertNotEquals(m.values(), Arrays.asList("b", "a"));
}
```
--
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] rondagostino commented on a diff in pull request #13280: KAFKA-14735: Improve KRaft metadata image change performance at high …
rondagostino commented on code in PR #13280:
URL: https://github.com/apache/kafka/pull/13280#discussion_r1114673066
##
metadata/src/main/java/org/apache/kafka/image/TopicsDelta.java:
##
@@ -126,29 +127,27 @@ public void handleMetadataVersionChange(MetadataVersion
newVersion) {
}
public TopicsImage apply() {
-Map newTopicsById = new
HashMap<>(image.topicsById().size());
-Map newTopicsByName = new
HashMap<>(image.topicsByName().size());
-for (Entry entry : image.topicsById().entrySet()) {
-Uuid id = entry.getKey();
-TopicImage prevTopicImage = entry.getValue();
-TopicDelta delta = changedTopics.get(id);
-if (delta == null) {
-if (!deletedTopicIds.contains(id)) {
-newTopicsById.put(id, prevTopicImage);
-newTopicsByName.put(prevTopicImage.name(), prevTopicImage);
-}
+ImMap newTopicsById = image.topicsById;
+ImMap newTopicsByName = image.topicsByName;
+// apply all the deletes
+for (Uuid topicId: deletedTopicIds) {
+// it was deleted, so we have to remove it from the maps
+TopicImage originalTopicToBeDeleted =
image.topicsById.get(topicId);
+if (originalTopicToBeDeleted == null) {
+throw new IllegalStateException("Missing topic id " + topicId);
} else {
-TopicImage newTopicImage = delta.apply();
-newTopicsById.put(id, newTopicImage);
-newTopicsByName.put(delta.name(), newTopicImage);
+newTopicsById = newTopicsById.without(topicId);
+newTopicsByName =
newTopicsByName.without(originalTopicToBeDeleted.name());
}
}
-for (Entry entry : changedTopics.entrySet()) {
-if (!newTopicsById.containsKey(entry.getKey())) {
-TopicImage newTopicImage = entry.getValue().apply();
-newTopicsById.put(newTopicImage.id(), newTopicImage);
-newTopicsByName.put(newTopicImage.name(), newTopicImage);
-}
+// apply all the updates/additions
+for (Map.Entry entry: changedTopics.entrySet()) {
+Uuid topicId = entry.getKey();
+TopicImage newTopicToBeAddedOrUpdated = entry.getValue().apply();
+// put new information into the maps
+String topicName = newTopicToBeAddedOrUpdated.name();
+newTopicsById = newTopicsById.assoc(topicId,
newTopicToBeAddedOrUpdated);
+newTopicsByName = newTopicsByName.assoc(topicName,
newTopicToBeAddedOrUpdated);
Review Comment:
Persistent data structures are immutable, so they always create and return a
new data structure.
--
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
