cmccabe commented on code in PR #13280: URL: https://github.com/apache/kafka/pull/13280#discussion_r1162128357
########## 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<Uuid, TopicImage> entry : topicsById.entrySet()) { + entry.getValue().write(writer, options); Review Comment: The `java.util.Collection` returned by `java.util.Map#values` is pretty basic. I can't find any comment about how its equals method is supposed to work. For `HashMap#values`, it seems to just be doing reference equality. (In contrast, `Map#keySet` returns an actual Set which is compared how you would expect.) @rondagostino, like you, I wrote a test program with `java.util.HashMap<StringString>` and `java.util.HashSet<String>` and got these very similar results ``` foo = {a->a, b->b) bar = {a, b} foo.values().equals(foo.values()) = true new HashSet<>(foo.values()).equals(bar) = true foo.values().equals(bar) = false bar.equals(foo.values()) = false foo.keySet().equals(bar) = true bar.equals(foo.keySet()) = true ``` > 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: What version of the pcollections source code were you looking at? I downloaded the source from https://github.com/hrldcpr/pcollections and wasn't able to find any comment or deprecated indicator for `HashPMap#values()`. In fact, it looks like it simply inherits the `AbstractMap#values` implementation without any changes. I suspect that this will actually implement reference equality, since this implementation saves the Collection object it creates in a `transient` field (ew) But even leaving that aside, I can't find any API guarantees about what the equals method of the collection returned by `Map#values` is supposed to do. It's possible that this is just undefined. At any rate the existing behavior of the java.util.Map subclasses is certainly useless here (it will not be what anyone expects) `Collection#equals` says you can do whatever you want for `equals`, but you should "exercise care". Zooming out a bit, the big picture is that interfaces like List or Set define a reasonable behavior for equals, whereas Collection (which is a parent interface for both) is just like :shrug: -- 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