I still feel better to change compareTo directly.
1. Although using PulsarApiMessageId.campare() can reduce the
probability of developers using errors, it cannot be completely
avoided.
2. While a direct change would change the default behavior, I consider
it a bug, not a breaking change. We can explain it in the new version
release blog. Maybe some users use it, but they didn’t find the
problem, and we changed it correctly . I don't think any user will be
able to use the current compareTo() correctly. Because the current
implementation is unexpected. When the user finds out that this
problem exists, he will not use this method.
Thanks,
Bo
Yunze Xu <y...@streamnative.io.invalid> 于2022年12月8日周四 20:43写道：
>
> Actually I'm refactoring the MessageId related code [1], whose current
> implementations are very messy from my perspective. My solution to
> this issue is adding two compare methods, one of them is the "wrong"
> implementation and used in `MessageId#compareTo` to avoid the breaking
> change. See the `legacyCompare` and `compare` methods.
>
> ```java
> // The legacy compare method, which treats the non-batched message id
> as preceding the batched message id.
> // However, this behavior is wrong because a non-batched message id
> represents an entry, while a batched message
> // represents a single message in the entry, which should precedes the
> message id.
> // Keep this implementation just for backward compatibility when users
> compare two message ids.
> static int legacyCompare(MessageIdDataInterface lhs,
> MessageIdDataInterface rhs) { /* ... */ }
>
> static int compare(MessageIdDataInterface lhs, MessageIdDataInterface
> rhs) { /* ... */ }
> ```
>
> [1] https://github.com/BewareMyPower/pulsar/pull/11/files
>
> Thanks,
> Yunze
>
> On Thu, Dec 8, 2022 at 7:22 PM 丛搏 <bog...@apache.org> wrote:
> >
> > Hi, Yunze:
> > If we don't change this behavior, we should pay special attention when
> > coding `pulsar-client`, because it is a point that is easy to
> > overlook. its impact may be more serious than "wrong " behavior
> > produced by the user using the current compareTo() method manually. I
> > don’t think this is a breaking change. On the contrary, it is a bug
> > that needs to be fixed. Because we cannot guarantee that everyone can
> > find the problem of compareTo() in time when writing code or reviewing
> > pr. The current implementation is Very anti-human.
> >
> > Thanks,
> > bo
> >
> > Yunze Xu <y...@streamnative.io.invalid> 于2022年12月8日周四 18:02写道：
> > >
> > > Actually, from the user side, this comparison would never happen.
> > > Users could never receive two MessageId objects with the same ledger
> > > id, entry id while the batch index fields are different. This
> > > comparison could only exist in the `pulsar-client` implementation.
> > >
> > > If users touch the case, the MessageId object must be created
> > > manually, which is a hack. The "wrong" behavior might be used. So my
> > > perspective is that we should not change this behavior.
> > >
> > > Thanks,
> > > Yunze
> > >
> > > On Thu, Dec 8, 2022 at 5:36 PM 丛搏 <bog...@apache.org> wrote:
> > > >
> > > > Hi, all:
> > > >
> > > > does anyone have any suggestions?
> > > >
> > > > Thanks,
> > > > bo
> > > >
> > > > 丛搏 <congbobo...@gmail.com> 于2022年11月21日周一 18:57写道：
> > > > >
> > > > > Hello, Pulsar community:
> > > > >
> > > > > now when `BatchMessageIdImpl` and `MessageIdImpl` with the same
> > > > > `ledgerId` and `EntryId`, one of it compare with the other, the
> > > > > `BatchMessageIdImpl` will always be greater than MessageIdImpl.
> > > > > see :
> > > > > https://github.com/apache/pulsar/blob/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageIdImpl.java#L71-L74
> > > > >
> > > > > https://github.com/apache/pulsar/blob/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/MessageIdImpl.java#L219-L228
> > > > >
> > > > > but when we use it, we may think `MessageIdImpl` is bigger than
> > > > > `BatchMessageIdImpl` with the same `ledgerId` and `EntryId`. It causes
> > > > > a lot of bugs. I think we need to change this `compareTo()` method,
> > > > > although it is a public API, I think it is not a breaking change, it
> > > > > is a bug that needs to be fixed.
> > > > > eg. : https://github.com/apache/pulsar/pull/18486, need to add the
> > > > > separate logic for compareTo().
> > > > >
> > > > > Please leave your thoughts, thanks.
> > > > >
> > > > > Thanks,
> > > > > bo
