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 <y...@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 丛搏 <bog...@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 <y...@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 丛搏 <bog...@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 <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
