Skip to content

[improve][client] Implement equals by compareTo to avoid logic unaligned - #18843

Merged
tisonkun merged 8 commits into
apache:masterfrom
tisonkun:improve-equals-impl
Dec 13, 2022
Merged

[improve][client] Implement equals by compareTo to avoid logic unaligned#18843
tisonkun merged 8 commits into
apache:masterfrom
tisonkun:improve-equals-impl

Conversation

@tisonkun

@tisonkun tisonkun commented Dec 9, 2022

Copy link
Copy Markdown
Member

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@labuladong

Copy link
Copy Markdown
Contributor

Nice catch! Maybe we can add a test in MessageIdTest.java to cover this change?

@tisonkun

Copy link
Copy Markdown
Member Author

@labuladong this doesn't change any existing logic and easy to be proven. If you have good ideas on tests, you can send me a patch.

@labuladong

Copy link
Copy Markdown
Contributor

this doesn't change any existing logic and easy to be proven

I don't think so, the original implementation is a little complex... I suggest just constructing different types of messageID and comparing them, the tests pass would prove the correctness easily.

@tisonkun

Copy link
Copy Markdown
Member Author

@labuladong I get your point. Updating...

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@codecov-commenter

codecov-commenter commented Dec 10, 2022

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 47.77%. Comparing base (3180a4a) to head (b780203).
⚠️ Report is 3142 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18843      +/-   ##
============================================
+ Coverage     46.17%   47.77%   +1.59%     
- Complexity    10359    10636     +277     
============================================
  Files           703      703              
  Lines         68845    68832      -13     
  Branches       7382     7379       -3     
============================================
+ Hits          31788    32883    +1095     
+ Misses        33448    32242    -1206     
- Partials       3609     3707      +98     
Flag Coverage Δ
unittests 47.77% <100.00%> (+1.59%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
.../apache/pulsar/client/impl/BatchMessageIdImpl.java 67.50% <100.00%> (-4.73%) ⬇️
...a/org/apache/pulsar/client/impl/MessageIdImpl.java 80.85% <100.00%> (+0.29%) ⬆️
.../apache/pulsar/client/impl/MultiMessageIdImpl.java 80.55% <100.00%> (+7.58%) ⬆️

... and 94 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@labuladong labuladong 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.

LGTM

} else if (o instanceof TopicMessageIdImpl) {
return equals(((TopicMessageIdImpl) o).getInnerMessageId());
if (o instanceof MessageId) {
return compareTo((MessageId) o) == 0;

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.

This will throw an exception if o is an instance of MessageId but not MessageIdImpl nor TopicMessageIdImpl, like MultiMessageIdImpl.
Before this PR, it just return false.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated. I change the impl in MultiMessageIdImpl also since it previous throws exception on type mistach in equals which seems unnecessary.

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
tisonkun requested a review from Jason918 December 13, 2022 05:11

@Jason918 Jason918 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.

LGTM

@tisonkun

Copy link
Copy Markdown
Member Author

Merging...

@tisonkun
tisonkun merged commit 0a2d927 into apache:master Dec 13, 2022
@tisonkun
tisonkun deleted the improve-equals-impl branch December 13, 2022 07:56
Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 26, 2022
Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 29, 2022
lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/client doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants