[improve][client]PIP-230 Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other by congbobo184 · Pull Request #18981 · apache/pulsar

congbobo184 · 2022-12-19T08:44:33Z

Motivation

#18957

Modifications

#18957

Verifying this change

Add the different MessageId compareTo another MessageId test

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

Documentation

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

Matching PR in forked repository

PR in forked repository:
congbobo184#13

…eIdImpl compare with each other

github-actions · 2022-12-19T08:45:15Z

@congbobo184 Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

mattisonchao · 2022-12-19T13:06:59Z

    public int compareTo(@Nonnull MessageId o) {
        if (o instanceof MessageIdImpl) {
+            if (o instanceof BatchMessageIdImpl) {
+                throw new UnsupportedOperationException(this.getClass().getName()


maybe we could allow comparison when MessageIdImpl' and BatchMessageIdImpl' do not have the same ledgerId and entryId?

This may cause confusion for users, direct forbid may be more effective and easier to understand

But comparing (1,1,0) to (1,2) is expected, right? only comparing (1,1,0) to 1,1 is not expected.

We should forbit comparing the two types. This is not the correct behavior for users. In what scenario will they compare BatchMessageId and MessageId? Now that have decided to throw an exception, why not forbid it outright? if allow some, forbid some, the user may use the wrong situation, which may exceed his expectations and judgments. @codelipenghui /cc

liangyepianzhou · 2022-12-20T02:15:06Z

    public int compareTo(@Nonnull MessageId o) {
        if (o instanceof MessageIdImpl) {
+            if (!(o instanceof BatchMessageIdImpl)) {
+                throw new UnsupportedOperationException(this.getClass().getName()


If we use the way of throwing exceptions then illegalArgumentException may be better here when the BatchMessageID and MessageID have the same ledger ID and entry ID.

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageIdImpl.java

Line 83 in 3177345

throw new UnsupportedOperationException("Unknown MessageId type: " + o.getClass().getName());

this throw UnsupportedOperationException , We should throw the same exception, otherwise, the user needs to catch both exceptions. MessageIdImpl and BatchMessageIdImpl can compare not illegal arguments, It's just that we don't support it

eolivelli · 2022-12-24T12:13:36Z

+            long ledgerId2, long entryId2, int partitionIndex2, int batchIndex2
+    ) {
+        return ComparisonChain.start()
+                .compare(ledgerId1, ledgerId2)


This code will be exd uted in hot paths probably.
Is it better to use raw java code? We could save method calls and allocations

goog point! I think we can use raw java code

github-actions · 2023-01-26T02:00:36Z

The pr had no activity for 30 days, mark with Stale label.

poorbarcode · 2023-04-10T07:50:19Z

Since we will start the RC version of 3.0.0 on 2023-04-11, I will change the label/milestone of PR who have not been merged.

The PR of type feature is deferred to 3.1.0
The PR of type fix is deferred to 3.0.1
So drag this PR to 3.1.0

github-actions · 2023-05-11T01:54:42Z

The pr had no activity for 30 days, mark with Stale label.

[improve][pip-230] Throw exception when MessageIdImpl and BatchMessag…

0a84a22

…eIdImpl compare with each other

congbobo184 added type/bug The PR fixed a bug or issue reported a bug area/client doc-not-needed Your PR changes do not impact docs labels Dec 19, 2022

congbobo184 added this to the 2.12.0 milestone Dec 19, 2022

congbobo184 requested review from BewareMyPower, codelipenghui, coderzc, eolivelli, gaoran10, lhotari, liangyepianzhou, mattisonchao and nicoloboschi December 19, 2022 08:44

congbobo184 self-assigned this Dec 19, 2022

github-actions Bot added doc-label-missing and removed doc-not-needed Your PR changes do not impact docs labels Dec 19, 2022

github-actions Bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Dec 19, 2022

congbobo184 changed the title ~~[improve][pip-230] Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other~~ [PIP-230][improve] Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other Dec 19, 2022

mattisonchao reviewed Dec 19, 2022

View reviewed changes

congbobo184 removed the type/bug The PR fixed a bug or issue reported a bug label Dec 19, 2022

congbobo184 changed the title ~~[PIP-230][improve] Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other~~ [feat][PIP-230] Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other Dec 19, 2022

congbobo184 changed the title ~~[feat][PIP-230] Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other~~ [feat][client] Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other Dec 19, 2022

congbobo184 changed the title ~~[feat][client] Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other~~ [feat][pip-230] Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other Dec 19, 2022

congbobo184 changed the title ~~[feat][pip-230] Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other~~ [feat][client][pip-230] Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other Dec 19, 2022

liangyepianzhou reviewed Dec 20, 2022

View reviewed changes

congbobo184 changed the title ~~[feat][client][pip-230] Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other~~ [improve][client]PIP-230 Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other Dec 20, 2022

fix the code style

927e4d2

congbobo184 added 5 commits December 21, 2022 21:17

fix some comment

90adc8e

delete some test and add some tset

f1529de

add earliest and latest compare

23bdcd1

add the ledgerId and entryId is special value compareTo

127bf9f

fix some test

3a7b44e

congbobo184 marked this pull request as draft December 24, 2022 08:07

eolivelli reviewed Dec 24, 2022

View reviewed changes

github-actions Bot added the Stale label Jan 26, 2023

poorbarcode modified the milestones: 3.0.0, 3.1.0 Apr 10, 2023

github-actions Bot removed the Stale label Apr 11, 2023

github-actions Bot added the Stale label May 11, 2023

Technoboy- modified the milestones: 3.1.0, 3.2.0 Jul 31, 2023

Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023

coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024

lhotari modified the milestones: 4.0.0, 4.1.0 Oct 14, 2024

coderzc modified the milestones: 4.1.0, 4.2.0 Sep 1, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve][client]PIP-230 Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other#18981

[improve][client]PIP-230 Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other#18981
congbobo184 wants to merge 7 commits into
apache:masterfrom
congbobo184:congbo/pip-230

congbobo184 commented Dec 19, 2022 •

edited

Loading

Uh oh!

github-actions Bot commented Dec 19, 2022

Uh oh!

mattisonchao Dec 19, 2022

Uh oh!

congbobo184 Dec 19, 2022

Uh oh!

codelipenghui Dec 20, 2022

Uh oh!

congbobo184 Dec 20, 2022 •

edited

Loading

Uh oh!

liangyepianzhou Dec 20, 2022 •

edited

Loading

Uh oh!

congbobo184 Dec 20, 2022

Uh oh!

eolivelli Dec 24, 2022

Uh oh!

congbobo184 Dec 26, 2022

Uh oh!

github-actions Bot commented Jan 26, 2023

Uh oh!

poorbarcode commented Apr 10, 2023

Uh oh!

github-actions Bot commented May 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Conversation

congbobo184 commented Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Matching PR in forked repository

Uh oh!

github-actions Bot commented Dec 19, 2022

Uh oh!

mattisonchao Dec 19, 2022

Choose a reason for hiding this comment

Uh oh!

congbobo184 Dec 19, 2022

Choose a reason for hiding this comment

Uh oh!

codelipenghui Dec 20, 2022

Choose a reason for hiding this comment

Uh oh!

congbobo184 Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liangyepianzhou Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

congbobo184 Dec 20, 2022

Choose a reason for hiding this comment

Uh oh!

eolivelli Dec 24, 2022

Choose a reason for hiding this comment

Uh oh!

congbobo184 Dec 26, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jan 26, 2023

Uh oh!

poorbarcode commented Apr 10, 2023

Uh oh!

github-actions Bot commented May 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

congbobo184 commented Dec 19, 2022 •

edited

Loading

congbobo184 Dec 20, 2022 •

edited

Loading

liangyepianzhou Dec 20, 2022 •

edited

Loading