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 the base class 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
