You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/12/16 09:23:10 UTC
[GitHub] [pulsar] congbobo184 opened a new issue, #18957: PIP-230: Modify MessageIdImpl and BatchMessageIdImpl compareTo(MessageId o) method
congbobo184 opened a new issue, #18957:
URL: https://github.com/apache/pulsar/issues/18957
### Motivation
now when `BatchMessageIdImpl` and `MessageIdImpl` with the same`LedgerId` and `EntryId`, one of it compared with the other, the`BatchMessageIdImpl` will always be greater than MessageIdImpl.
compare logic see :
https://github.com/apache/pulsar/blob/a35670d83b0b3f2fb63d11e4fb222d7f24099c9a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageIdImpl.java#L29
https://github.com/apache/pulsar/blob/a35670d83b0b3f2fb63d11e4fb222d7f24099c9a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageIdImpl.java#L74-L75
https://github.com/apache/pulsar/blob/a35670d83b0b3f2fb63d11e4fb222d7f24099c9a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/MessageIdImpl.java#L206-L208
`MessageIdImpl` add this logic: https://github.com/apache/pulsar/pull/6621
`BatchMessageIdImpl` add this logic: https://github.com/apache/pulsar/pull/1285
MessageID is mainly used for seek, rest cursor, Individual ack, and cumulative ack.
now cumulative ack independently fixed this issue: https://github.com/apache/pulsar/blob/a35670d83b0b3f2fb63d11e4fb222d7f24099c9a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java#L665-L681
**Disadvantages of the current compareTo() implementation:**
1. Developers can easily ignore this problem when writing code using MessageId.compareTo().
2. Many users may use this method incorrectly, resulting in seek, ack, or resetCursor use incorrect MessageId, which has the risk of losing messages, because they will use a larger messageId to seek, ack etc.
3. It makes it difficult for users to use compareTo() correctly. Users will abandon using compareTo().
### Goal
1. Fix the problem that users mistakenly use MessageID compareTo() to cause message loss.
2. Make it easier for developers to use compareTo() to reduce bugs
3. Make users can better use MessageId compareTo(), not implement it by urself
Although it's a breaking change, It fixes problems caused by misuse by users and developers. Modifying it is much better than keeping the existing way
### API Changes
_No response_
### Implementation
change current `MessageIdImpl` and `BatchMessageIdImpl` compareTo() logic as the same as https://github.com/apache/pulsar/blob/a35670d83b0b3f2fb63d11e4fb222d7f24099c9a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java#L665-L681
### Alternatives
keep the current logic
### Anything else?
_No response_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on issue #18957: PIP-230: Modify MessageIdImpl and BatchMessageIdImpl compareTo(MessageId o) method
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on issue #18957:
URL: https://github.com/apache/pulsar/issues/18957#issuecomment-1357202319
@liangyepianzhou Yes we should avoid comparing them, the new implementation will throw an exception
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] PIP-230: Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat closed issue #18957: PIP-230: Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other
URL: https://github.com/apache/pulsar/issues/18957
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on issue #18957: PIP-230: Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on issue #18957:
URL: https://github.com/apache/pulsar/issues/18957#issuecomment-1396345925
The issue had no activity for 30 days, mark with Stale label.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] liangyepianzhou commented on issue #18957: PIP-230: Modify MessageIdImpl and BatchMessageIdImpl compareTo(MessageId o) method
Posted by GitBox <gi...@apache.org>.
liangyepianzhou commented on issue #18957:
URL: https://github.com/apache/pulsar/issues/18957#issuecomment-1356759691
> now when BatchMessageIdImpl and MessageIdImpl with the sameLedgerId and EntryId, one of it compared with the other, theBatchMessageIdImpl will always be greater than MessageIdImpl.
Sorry, I can not understand this case.
Why do there exist MessageID and BatchMessageID with the same ledgerID, entryID, and partitionIndex?
> change current `MessageIdImpl` and `BatchMessageIdImpl` compareTo() logic as the same as https://github.com/apache/pulsar/blob/a35670d83b0b3f2fb63d11e4fb222d7f24099c9a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java#L665-L681
And why it is right that the MessageID is always bigger than the BatchMessageID?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] PIP-230: Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on issue #18957:
URL: https://github.com/apache/pulsar/issues/18957#issuecomment-6377558118
Thanks @congbobo184 for the proposal. The PIP-230 vote (https://lists.apache.org/thread/1tx1fz3h72m6pp1q6vzxs0fdw7yq0k1x) received no votes, and #18981 was closed. Message ID comparison has since been reworked by PIP-229 (#19414): `compareTo` is now a default method of the `MessageIdAdv` interface, and `MessageIdImpl` and `BatchMessageIdImpl` no longer have their own implementations. I'm closing this as not planned. Anyone is welcome to restart it as a pip/ document following pip/README.md.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org