KAFKA 6673 - Implemented missing override equals method#4745
KAFKA 6673 - Implemented missing override equals method#4745guozhangwang merged 6 commits intoapache:trunkfrom
Conversation
|
The fix is to implement the missing override equals() method. |
|
As I stated on the kafka jira: That implementation would just do an identity comparison. Because Stamped and Segment don't have an explicit super class, just Object. And Object's equals is this: The whole idea behind this ticket is that the equals method should act logically the same as the compareTo method, as to have only 1 kind of behavior. |
|
@KoenDG - Thanks for the review. Have tried to mimic compareTo logic for similar behavior. |
|
@KoenDG - Are we good with the changes? Anything else needed as part of this PR? |
|
@asutosh936 They're fine for me. But I just made the ticket. I can't actually merge this. |
|
Can someone review and provide feedback? |
|
@hachikuji @guozhangwang @ewencp Could someone please review and merge, if code change looks good. |
|
@bbejeck could you take a look? @asutosh936 Did you run unit tests locally and make sure they are passed? |
|
retest this please |
There was a problem hiding this comment.
Thanks for the patch @asutosh936 . There are a couple of things that need fixing here.
The test failures are from findbugs warnings that need to be addressed. If you run ./gradlew streams:clean streams:test then look at the report in streams/build/reports/findbugs/main.html you can read what needs to change.
The more general idea is that when foo.compareTo(bar) returns 0, then foo.equals(bar) should return true and and a non-zero return from compareTo means equals should return false.
| @Override | ||
| public boolean equals(Object other) { | ||
|
|
||
| if (getClass() != other.getClass()) return false; |
| if (getClass() != other.getClass()) return false; | ||
|
|
||
| long otherTimestamp = ((Stamped<?>) other).timestamp; | ||
| return timestamp > otherTimestamp; |
There was a problem hiding this comment.
This implementation of equals will return false for timestamps of the same value; maybe this could be something like return Long.compare(timestamp, otherTimestamp) == 0
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return super.hashCode(); |
There was a problem hiding this comment.
Since equality is measured by comparing timestamps something like return Objects.hash(timestamp); should be used instead.
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return super.hashCode(); |
|
|
||
| @Override | ||
| public boolean equals(Object obj) { | ||
| if (getClass() != obj.getClass()) return false; |
There was a problem hiding this comment.
need a check for null on obj here as well
| if (getClass() != obj.getClass()) return false; | ||
|
|
||
| Segment segment = (Segment) obj; | ||
| return Long.compare(id, segment.id) > 1; |
There was a problem hiding this comment.
Left this out of my previous review.
This needs to change as well as it will return false for two Segment instances with the same id.
Also, this statement will always evaluate to false as Long.compare(x, y) only returns -1, 0, or 1.
|
Thanks @bbejeck will implement Your review comments and update. |
|
@bbejeck @guozhangwang |
|
retest this please |
bbejeck
left a comment
bbejeck
left a comment
There was a problem hiding this comment.
@asutosh936 thanks for the changes. LGTM.
|
Merged to trunk, thanks @asutosh936 ! |
…-record-version * apache-github/trunk: KAFKA-6894: Improve err msg when connecting processor with global store (apache#5000) KAFKA-6893; Create processors before starting acceptor in SocketServer (apache#4999) MINOR: Fix typo in ConsumerRebalanceListener JavaDoc (apache#4996) MINOR: Remove deprecated valueTransformer.punctuate (apache#4993) MINOR: Update dynamic broker configuration doc for truststore update (apache#4954) KAFKA-6870 Concurrency conflicts in SampledStat (apache#4985) KAFKA-6361: Fix log divergence between leader and follower after fast leader fail over (apache#4882) KAFKA-6813: Remove deprecated APIs in KIP-182, Part II (apache#4976) KAFKA-6878 Switch the order of underlying.init and initInternal (apache#4988) KAFKA-6299; Fix AdminClient error handling when metadata changes (apache#4295) KAFKA-6878: NPE when querying global state store not in READY state (apache#4978) KAFKA 6673: Implemented missing override equals method (apache#4745) KAFKA-6834: Handle compaction with batches bigger than max.message.bytes (apache#4953)
Reviewers: Guozhang Wang <guozhang@confluent.io>, Bill Bejeck <bill@confluent.io>
5 participants
More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.
Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.
Committer Checklist (excluded from commit message)