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
[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
[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