Skip to content

Conversation

@BewareMyPower
Copy link
Contributor

The computation for compareMarkDeletePosition is wrong that hasSoughtByTimestamp_ should only be checked when lastDequedMessageId_ is earliest, which means no messages have been received.

Additionally, reset startMessageId_ when seeking by timestamp, otherwise, if the initial startMessageId of a reader is a valid message id, that message could be filtered in

if (isPersistent_ && startMessageId &&
m.getMessageId().ledgerId() == startMessageId.value().ledgerId() &&
m.getMessageId().entryId() == startMessageId.value().entryId() &&
isPriorEntryIndex(m.getMessageId().entryId())) {
LOG_DEBUG(getName() << " Ignoring message from before the startMessageId: "
<< startMessageId.value());
return;
or
if (isPersistent_ && msgId.ledgerId() == startMessageId.value().ledgerId() &&
msgId.entryId() == startMessageId.value().entryId() &&
isPriorBatchIndex(msgId.batchIndex())) {
LOG_DEBUG(getName() << "Ignoring message from before the startMessageId"
<< msg.getMessageId());
++skippedMessages;
continue;

Improve testHasMessageAvailableAfterSeekTimestamp to cover the cases above.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a bug where hasMessageAvailable incorrectly returns true when reading to the latest message after seeking by timestamp. The fix involves correcting the logic for determining when to compare mark delete positions and properly resetting the start message ID when seeking by timestamp.

Key changes:

  • Modified compareMarkDeletePosition logic to only use hasSoughtByTimestamp_ check when no messages have been received
  • Reset startMessageId_ to prevent message filtering issues after timestamp seeks
  • Enhanced test coverage to validate the fix by reading all available messages after seeking

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
lib/ConsumerImpl.h Added helper method hasSoughtByTimestamp() for consistent atomic access
lib/ConsumerImpl.cc Fixed compareMarkDeletePosition logic and startMessageId_ handling for timestamp seeks
tests/ReaderTest.cc Enhanced test to validate fix by consuming all messages after seeking by timestamp

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@BewareMyPower BewareMyPower merged commit 17fcf2a into apache:main Aug 21, 2025
14 checks passed
@BewareMyPower BewareMyPower deleted the bewaremypower/seek branch September 17, 2025 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants