KAFKA-6727 fix broken Config hashCode() and equals() #4796
Conversation
Current implementation stores reference to `entries` wrapped in `Collections.unmodifiableCollection`, which breaks `hashCode` and `equals`.
|
LGTM |
|
Good catch. This tells me that we have to stop using |
| @@ -36,14 +37,14 @@ | |||
| * Create a configuration instance with the provided entries. | |||
| */ | |||
| public Config(Collection<ConfigEntry> entries) { | |||
| this.entries = Collections.unmodifiableCollection(entries); | |||
| this.entries = Objects.requireNonNull(entries); | |||
There was a problem hiding this comment.
I wonder if we should copy to a list so that we are guaranteed that the collection won't change behind our backs and so that comparison between two Config objects will be valid even if they are constructed using different collection types.
There was a problem hiding this comment.
On second thought, considering the usage of get below, maybe we should create a Map instead so that we can more easily access an entry by name.
There was a problem hiding this comment.
I thought about a defensive copy - that would be my standard pattern here. But wanted to minimise my changes.
Give me a mo... I'll switch...
|
Yer, BTW, I'd normally test hashCode & equals using Google's EqualsTester. Is there something already like this included in AK? If not, would adding this as a test dependency be an option? @ijuma @hachikuji @omkreddy code updated inline with review comments. |
|
@big-andy-coates I'd argue that what we have now is still not good. :) After all, people can still call |
|
LGTM! |
|
retest this please |
|
@big-andy-coates Note the checkstyle failure. The patch LGTM otherwise. |
The docs for this class do not make any guarantees about the equality properties of the return value from |
|
retest this please (as the builds have been deleted) |
|
@big-andy-coates The class doesn't document List<ConfigEntry> entries = ...
return new Config(entries).entries().equals(entries);Don't you? |
|
@ijuma true, but is anyone likely to do this? Would you prefer if it took a defensive copy? |
|
@ijuma @hachikuji @omkreddy @guozhangwang changes made - hopefully last review. If you're happy, then can someone please merge. Thanks all for your time reviewing :D |
There was a problem hiding this comment.
@big-andy-coates Thanks. I synced with Ismael offline and had one minor suggestion. If it seems reasonable, we can go ahead and merge.
| } | ||
|
|
||
| /** | ||
| * Configuration entries for a resource. | ||
| */ | ||
| public Collection<ConfigEntry> entries() { | ||
| return entries; | ||
| return new ArrayList<>(entries.values()); |
There was a problem hiding this comment.
Discussed offline with @ijuma. This actually isn't quite enough to fix the example he brought up earlier in all cases. For example, it still doesn't work if you construct Config with a Set for the entries. Given that we can't guarantee equality in this way generally, I'd probably suggest we just revert to using Collections.unmodifiableCollection like you had before.
There was a problem hiding this comment.
Yep, makes sense. Done.
|
@hachikuji this should be good to merge now. |
|
LGTM. Will merge as soon as the builds complete. |
Reviewers: Manikumar Reddy O <manikumar.reddy@gmail.com>, Guozhang Wang <wangguoz@gmail.com>, Ismael Juma <ismael@juma.me.uk>, Jason Gustafson <jason@confluent.io>
Reviewers: Manikumar Reddy O <manikumar.reddy@gmail.com>, Guozhang Wang <wangguoz@gmail.com>, Ismael Juma <ismael@juma.me.uk>, Jason Gustafson <jason@confluent.io>
Fix for KAFKA-6727.
Current implementation stores reference to
entrieswrapped inCollections.unmodifiableCollection, which breakshashCodeandequals.From Java docs
Contribution is my own original work and I license the work to the project under the project's open source license.