You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pulsar.apache.org by 丛搏 <bo...@apache.org> on 2022/12/08 09:35:07 UTC
Re: [DISCUSS] Modify MessageIdImpl and BatchMessageIdImpl compareTo(MessageId o) method
Hi, all:
does anyone have any suggestions?
Thanks,
bo
丛搏 <co...@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
Re: [DISCUSS] Modify MessageIdImpl and BatchMessageIdImpl compareTo(MessageId o) method
Posted by Yunze Xu <yz...@streamnative.io.INVALID>.
In short, this change is okay for me. But since it's a breaking
change, the Pulsar PMCs should be responsible to make the idea pass.
Thanks,
Yunze
On Fri, Dec 16, 2022 at 11:53 AM Yunze Xu <yz...@streamnative.io> wrote:
>
> PIP is required when you want to make a breaking change. [1] What you
> think is wrong might have been applied in existing applications, just
> like the comments here [2]
>
> The only controversial point is that should we treat it as a breaking
> change? From [3]
>
> > A change in one part of a software system that potentially causes other components to fail
>
> The `MessageId#compareTo` is a public interface in
> `pulsar-client-api`, if someone has adopted the current "wrong" logic,
> "fixing it" could cause the application to fail. I don't want to make
> breaking changes in my proposals and PRs, if you insist on "fixing the
> bug" (making the breaking change), you have to open a PIP to get PMC
> votes.
>
> [1] https://github.com/apache/pulsar/blob/master/wiki/proposals/PIP.md#what-is-the-goal-of-a-pip
> [2] https://github.com/apache/pulsar/pull/10601#issuecomment-1318397159
> [3] https://en.wiktionary.org/wiki/breaking_change#:~:text=breaking%20change%20(plural%20breaking%20changes,to%20new%20in%20import%20lib.
>
> Thanks,
> Yunze
>
> On Fri, Dec 16, 2022 at 11:32 AM 丛搏 <bo...@apache.org> wrote:
> >
> > It doesn’t matter if PIP is open. can directly describe it clearly in
> > https://lists.apache.org/thread/mbrpjsgrgwrlkdpvkk738jxnlk7rf4qk and
> > modify it. But either newly opened PIP or in
> > https://lists.apache.org/thread/mbrpjsgrgwrlkdpvkk738jxnlk7rf4q, my
> > point is that we need to fix it, not compatible with it.
> >
> > Thanks,
> > Bo
> >
> > Yunze Xu <yz...@streamnative.io.invalid> 于2022年12月16日周五 08:21写道：
> > >
> > > Let's see another example that is considered as a "bug", not a breaking change.
> > >
> > > https://lists.apache.org/thread/88t1xxf68j092k09srdwyzj1tk4ml5n9
> > >
> > > > I think that this is fixing a bug, if the topic does not exist we should return "not found".
> > >
> > > The "bug" and "breaking change" are not in contrast. What a user
> > > thinks is a bug might be a breaking change. Would you like to open a
> > > PIP for that? From my perspective, both modifying or not is okay.
> > >
> > > Thanks,
> > > Yunze
> > >
> > > On Wed, Dec 14, 2022 at 11:27 PM 丛搏 <bo...@apache.org> wrote:
> > > >
> > > > 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 <yz...@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 丛搏 <bo...@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 <yz...@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 丛搏 <bo...@apache.org> wrote:
> > > > > > > >
> > > > > > > > Hi, all:
> > > > > > > >
> > > > > > > > does anyone have any suggestions?
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > bo
> > > > > > > >
> > > > > > > > 丛搏 <co...@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
Re: [DISCUSS] Modify MessageIdImpl and BatchMessageIdImpl compareTo(MessageId o) method
Posted by Yunze Xu <yz...@streamnative.io.INVALID>.
PIP is required when you want to make a breaking change. [1] What you
think is wrong might have been applied in existing applications, just
like the comments here [2]
The only controversial point is that should we treat it as a breaking
change? From [3]
> A change in one part of a software system that potentially causes other components to fail
The `MessageId#compareTo` is a public interface in
`pulsar-client-api`, if someone has adopted the current "wrong" logic,
"fixing it" could cause the application to fail. I don't want to make
breaking changes in my proposals and PRs, if you insist on "fixing the
bug" (making the breaking change), you have to open a PIP to get PMC
votes.
[1] https://github.com/apache/pulsar/blob/master/wiki/proposals/PIP.md#what-is-the-goal-of-a-pip
[2] https://github.com/apache/pulsar/pull/10601#issuecomment-1318397159
[3] https://en.wiktionary.org/wiki/breaking_change#:~:text=breaking%20change%20(plural%20breaking%20changes,to%20new%20in%20import%20lib.
Thanks,
Yunze
On Fri, Dec 16, 2022 at 11:32 AM 丛搏 <bo...@apache.org> wrote:
>
> It doesn’t matter if PIP is open. can directly describe it clearly in
> https://lists.apache.org/thread/mbrpjsgrgwrlkdpvkk738jxnlk7rf4qk and
> modify it. But either newly opened PIP or in
> https://lists.apache.org/thread/mbrpjsgrgwrlkdpvkk738jxnlk7rf4q, my
> point is that we need to fix it, not compatible with it.
>
> Thanks,
> Bo
>
> Yunze Xu <yz...@streamnative.io.invalid> 于2022年12月16日周五 08:21写道：
> >
> > Let's see another example that is considered as a "bug", not a breaking change.
> >
> > https://lists.apache.org/thread/88t1xxf68j092k09srdwyzj1tk4ml5n9
> >
> > > I think that this is fixing a bug, if the topic does not exist we should return "not found".
> >
> > The "bug" and "breaking change" are not in contrast. What a user
> > thinks is a bug might be a breaking change. Would you like to open a
> > PIP for that? From my perspective, both modifying or not is okay.
> >
> > Thanks,
> > Yunze
> >
> > On Wed, Dec 14, 2022 at 11:27 PM 丛搏 <bo...@apache.org> wrote:
> > >
> > > 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 <yz...@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 丛搏 <bo...@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 <yz...@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 丛搏 <bo...@apache.org> wrote:
> > > > > > >
> > > > > > > Hi, all:
> > > > > > >
> > > > > > > does anyone have any suggestions?
> > > > > > >
> > > > > > > Thanks,
> > > > > > > bo
> > > > > > >
> > > > > > > 丛搏 <co...@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
Re: [DISCUSS] Modify MessageIdImpl and BatchMessageIdImpl compareTo(MessageId o) method
Posted by 丛搏 <bo...@apache.org>.
It doesn’t matter if PIP is open. can directly describe it clearly in
https://lists.apache.org/thread/mbrpjsgrgwrlkdpvkk738jxnlk7rf4qk and
modify it. But either newly opened PIP or in
https://lists.apache.org/thread/mbrpjsgrgwrlkdpvkk738jxnlk7rf4q, my
point is that we need to fix it, not compatible with it.
Thanks,
Bo
Yunze Xu <yz...@streamnative.io.invalid> 于2022年12月16日周五 08:21写道：
>
> Let's see another example that is considered as a "bug", not a breaking change.
>
> https://lists.apache.org/thread/88t1xxf68j092k09srdwyzj1tk4ml5n9
>
> > I think that this is fixing a bug, if the topic does not exist we should return "not found".
>
> The "bug" and "breaking change" are not in contrast. What a user
> thinks is a bug might be a breaking change. Would you like to open a
> PIP for that? From my perspective, both modifying or not is okay.
>
> Thanks,
> Yunze
>
> On Wed, Dec 14, 2022 at 11:27 PM 丛搏 <bo...@apache.org> wrote:
> >
> > 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 <yz...@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 丛搏 <bo...@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 <yz...@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 丛搏 <bo...@apache.org> wrote:
> > > > > >
> > > > > > Hi, all:
> > > > > >
> > > > > > does anyone have any suggestions?
> > > > > >
> > > > > > Thanks,
> > > > > > bo
> > > > > >
> > > > > > 丛搏 <co...@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
Re: [DISCUSS] Modify MessageIdImpl and BatchMessageIdImpl compareTo(MessageId o) method
Posted by Yunze Xu <yz...@streamnative.io.INVALID>.
Let's see another example that is considered as a "bug", not a breaking change.
https://lists.apache.org/thread/88t1xxf68j092k09srdwyzj1tk4ml5n9
> I think that this is fixing a bug, if the topic does not exist we should return "not found".
The "bug" and "breaking change" are not in contrast. What a user
thinks is a bug might be a breaking change. Would you like to open a
PIP for that? From my perspective, both modifying or not is okay.
Thanks,
Yunze
On Wed, Dec 14, 2022 at 11:27 PM 丛搏 <bo...@apache.org> wrote:
>
> 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 <yz...@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 丛搏 <bo...@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 <yz...@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 丛搏 <bo...@apache.org> wrote:
> > > > >
> > > > > Hi, all:
> > > > >
> > > > > does anyone have any suggestions?
> > > > >
> > > > > Thanks,
> > > > > bo
> > > > >
> > > > > 丛搏 <co...@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
Re: [DISCUSS] Modify MessageIdImpl and BatchMessageIdImpl compareTo(MessageId o) method
Posted by 丛搏 <bo...@apache.org>.
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 <yz...@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 丛搏 <bo...@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 <yz...@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 丛搏 <bo...@apache.org> wrote:
> > > >
> > > > Hi, all:
> > > >
> > > > does anyone have any suggestions?
> > > >
> > > > Thanks,
> > > > bo
> > > >
> > > > 丛搏 <co...@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
Re: [DISCUSS] Modify MessageIdImpl and BatchMessageIdImpl compareTo(MessageId o) method
Posted by Yunze Xu <yz...@streamnative.io.INVALID>.
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 丛搏 <bo...@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 <yz...@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 丛搏 <bo...@apache.org> wrote:
> > >
> > > Hi, all:
> > >
> > > does anyone have any suggestions?
> > >
> > > Thanks,
> > > bo
> > >
> > > 丛搏 <co...@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
Re: [DISCUSS] Modify MessageIdImpl and BatchMessageIdImpl compareTo(MessageId o) method
Posted by 丛搏 <bo...@apache.org>.
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 <yz...@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 丛搏 <bo...@apache.org> wrote:
> >
> > Hi, all:
> >
> > does anyone have any suggestions?
> >
> > Thanks,
> > bo
> >
> > 丛搏 <co...@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
Re: [DISCUSS] Modify MessageIdImpl and BatchMessageIdImpl compareTo(MessageId o) method
Posted by Yunze Xu <yz...@streamnative.io.INVALID>.
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 丛搏 <bo...@apache.org> wrote:
>
> Hi, all:
>
> does anyone have any suggestions?
>
> Thanks,
> bo
>
> 丛搏 <co...@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