[fix][client] Fix wrong results of hasMessageAvailable and readNext after seeking by timestamp #22363
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…fter seeking by timestamp ### Motivation After seeking by timestamp, `hasMessageAvailable()` and `readNext()` could return wrong values. The 1st bug was a regression introduced by apache#22201, which modifies `startMessageId` to `seekMessageId` before a seek operation is done. However, the previous behavior is also a bug but accidentally works in this case. When seeking by timestamp, the `seekMessageId` is modified to `earliest`, which should not be compared with `lastMessageIdInBroker` because the actual start position is determined by the seek timestamp, not the `earliest` message id. The 2nd bug was caused by apache#9652, when `startMessageIdInclusive()` is configured to create a reader, it could seek to the position of the latest message when `lastDequeuedMessageId` is `earliest` and `startMessageId` is `latest`. ### Modifications Add a boolean flag `hasSoughtByTimestamp` to represent whether the last seek call accepts a timestamp. If it's true, don't take `startMessageId` into comparison with `lastMessageIdInBroker`, just compare the mark-delete position and last position in the GetLastMessageId response. Add `testHasMessageAvailableAfterSeekTimestamp` to verify the change. For the `readNext` call, if the reader has sought by timestamp, don't seek to the latest position in `hasMessageAvailable`. Modify `testReadMessageWithBatchingWithMessageInclusive` to verify the fix. However, this patch does not modify the existing behavior when `seek` is not called because the inclusive reader relies on the implicit seek operation in `hasMessageAvailable`.
BewareMyPower
requested review from
lhotari,
Technoboy-,
codelipenghui,
RobertIndie,
dao-jun,
poorbarcode and
shibd
lhotari
reviewed
Mar 26, 2024
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/ReaderTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Lari Hotari <[email protected]>
dao-jun
approved these changes
Mar 27, 2024
shibd
approved these changes
Mar 27, 2024
BewareMyPower
added a commit
to BewareMyPower/pulsar-client-cpp
that referenced
this pull request
Mar 27, 2024
…y timestamp Fixes apache#420 It's a catch-up for apache/pulsar#22363
lhotari
pushed a commit
that referenced
this pull request
Mar 27, 2024
…fter seeking by timestamp (#22363) Co-authored-by: Lari Hotari <[email protected]> (cherry picked from commit 149deaa)
lhotari
pushed a commit
that referenced
this pull request
Mar 27, 2024
…fter seeking by timestamp (#22363) Co-authored-by: Lari Hotari <[email protected]> (cherry picked from commit 149deaa)
BewareMyPower
added a commit
to apache/pulsar-client-cpp
that referenced
this pull request
Mar 28, 2024
…y timestamp (#422) Fixes #420 It's a catch-up for apache/pulsar#22363
BewareMyPower
added a commit
to apache/pulsar-client-cpp
that referenced
this pull request
Mar 28, 2024
…y timestamp (#422) Fixes #420 It's a catch-up for apache/pulsar#22363 (cherry picked from commit 27d8cc0)
Technoboy-
pushed a commit
to Technoboy-/pulsar
that referenced
this pull request
Apr 1, 2024
…fter seeking by timestamp (apache#22363) Co-authored-by: Lari Hotari <[email protected]>
mukesh-ctds
pushed a commit
to datastax/pulsar
that referenced
this pull request
Apr 15, 2024
…fter seeking by timestamp (apache#22363) Co-authored-by: Lari Hotari <[email protected]> (cherry picked from commit 149deaa) (cherry picked from commit 1045f8b)
mukesh-ctds
pushed a commit
to datastax/pulsar
that referenced
this pull request
Apr 17, 2024
…fter seeking by timestamp (apache#22363) Co-authored-by: Lari Hotari <[email protected]> (cherry picked from commit 149deaa) (cherry picked from commit 1045f8b)
mukesh-ctds
pushed a commit
to datastax/pulsar
that referenced
this pull request
Apr 17, 2024
…fter seeking by timestamp (apache#22363) Co-authored-by: Lari Hotari <[email protected]> (cherry picked from commit 149deaa) (cherry picked from commit 1045f8b)
mukesh-ctds
pushed a commit
to datastax/pulsar
that referenced
this pull request
Apr 19, 2024
…fter seeking by timestamp (apache#22363) Co-authored-by: Lari Hotari <[email protected]> (cherry picked from commit 149deaa) (cherry picked from commit 1045f8b)
srinath-ctds
pushed a commit
to datastax/pulsar
that referenced
this pull request
Apr 23, 2024
…fter seeking by timestamp (apache#22363) Co-authored-by: Lari Hotari <[email protected]> (cherry picked from commit 149deaa) (cherry picked from commit 1045f8b)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
After seeking by timestamp,
hasMessageAvailable()andreadNext()could return wrong values.The 1st bug was a regression introduced by
#22201, which modifies
startMessageIdtoseekMessageIdbefore a seek operation is done. However, the previous behavior is also a bug but accidentally works in this case. When seeking by timestamp, theseekMessageIdis modified toearliest, which should not be compared withlastMessageIdInBrokerbecause the actual start position is determined by the seek timestamp, not theearliestmessage id.The 2nd bug was caused by #9652, when
startMessageIdInclusive()is configured to create a reader, it could seek to the position of the latest message whenlastDequeuedMessageIdisearliestandstartMessageIdislatest.Modifications
Add a boolean flag
hasSoughtByTimestampto represent whether the last seek call accepts a timestamp. If it's true, don't takestartMessageIdinto comparison withlastMessageIdInBroker, just compare the mark-delete position and last position in the GetLastMessageId response.Add
testHasMessageAvailableAfterSeekTimestampto verify the change.For the
readNextcall, if the reader has sought by timestamp, don't seek to the latest position inhasMessageAvailable. ModifytestReadMessageWithBatchingWithMessageInclusiveto verify the fix. However, this patch does not modify the existing behavior whenseekis not called because the inclusive reader relies on the implicit seek operation inhasMessageAvailable.Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: