Skip to content

fix: fix equal bug in some files#10574

Open
enkilee wants to merge 3 commits into
apache:developfrom
enkilee:fix-eual-bug
Open

fix: fix equal bug in some files#10574
enkilee wants to merge 3 commits into
apache:developfrom
enkilee:fix-eual-bug

Conversation

@enkilee

@enkilee enkilee commented Jul 2, 2026

Copy link
Copy Markdown

Which Issue(s) This PR Fixes

  • Fixes #issue_id

Brief Description

compareTo 与 Object.equals 共存.
在 TreeSet/TreeMap 中,compareTo 返回 0 的两个对象会被视为"相等"但 equals() 返回 false,违反 Comparable 契约,可能导致集合中元素丢失或行为不一致。
修复:重写 equals/hashCode 与 compareTo 一致。

How Did You Test This Change?

enkilee added 2 commits July 2, 2026 15:08
fix equal bug in some files.
equals/hashCode 基于当前字段值。若对象在集合中时字段被修改,可能导致定位失败——这是这些类本就存在的可变性问题,本次修复仅保证契约一致性,未改变可变性。

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This PR adds equals() and hashCode() implementations to 7 classes that implement Comparable but were missing proper equality methods.

Overall Assessment: ✅ Generally good, with minor suggestions

Changes Reviewed:

  • TraceContext.java - Uses timeStamp for equality
  • GrpcClientChannel.java - Uses clientId for equality
  • RemoteChannel.java - Uses id for equality
  • PopCheckPoint.java - Uses startOffset for equality
  • FileSegment.java - Uses baseOffset for equality
  • GroupConsumeInfo - Uses count + diffTotal for equality
  • TagCountBean - Uses tag for equality

Observations:

  1. Consistency with compareTo(): Most implementations align equality fields with compareTo(), which is good practice. However, note that TraceContext uses only timeStamp while compareTo() also uses timeStamp — this is consistent.

  2. Null handling: GrpcClientChannel and TagCountBean properly handle null values in equals() and hashCode(). Good.

  3. Potential issue in GroupConsumeInfo: The compareTo() uses diffTotal - diffTotal (subtraction), but equals() uses field comparison. This is acceptable, but ensure the class is not used in sorted collections where compareTo() returning 0 should imply equals() returning true.

  4. Missing Objects.equals(): Some implementations use manual null checks instead of Objects.equals(). Consider using:

    return Objects.equals(this.tag, that.tag);

Suggestion: Consider adding unit tests to verify the equals()/hashCode() contracts (reflexive, symmetric, transitive, consistent).


Review by @RockteMQ-AI

fix by rocketmq ai suggestion
@enkilee enkilee requested a review from RockteMQ-AI July 15, 2026 02:38

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by github-manager-bot (Re-review after new commit 72b8f81)

Summary

New commit pushed addressing previous review suggestions. Two equals() implementations changed and unit tests added for GroupConsumeInfo and TagCountBean.

Changes in This Commit

File Change Assessment
GroupConsumeInfo equals() now uses group instead of count+diffTotal ✅ Semantically correct — two entries with the same group name should be equal
TagCountBean equals() now uses count.get() instead of tag ✅ Consistent with compareTo() which also orders by count
GroupConsumeInfoTest (new) Tests for equals contract (reflexive, symmetric, transitive) and hashCode ✅ Good coverage
TagCountBeanTest (new) Tests for equals contract and hashCode ✅ Good coverage

Remaining Observations

  1. GroupConsumeInfocompareTo vs equals field mismatch: compareTo() uses diffTotal while equals() uses group. This means two objects with the same group but different diffTotal would be equals() == true but compareTo() != 0. This is technically a Comparable contract violation, but it's a pre-existing design issue in the class (the original compareTo was already questionable). Not a blocker for this PR.

  2. TagCountBean — descending order: compareTo() returns o.count - this.count (descending), while equals() uses count. This is actually consistent (same field, just different ordering direction), so no issue here.

Verdict

Approved — The new commit properly addresses the previous suggestions. Adding unit tests is a nice improvement. The remaining compareTo/equals field mismatch in GroupConsumeInfo is a pre-existing design concern, not introduced by this PR.


Automated review by github-manager-bot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants