Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix][client] Fix multi-topics consumer could receive old messages after seek #21945

Conversation

BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Jan 22, 2024

Motivation

The multi-topics consumer supports seeking all internal consumers when accepting a timestamp as the argument. However, the multi-topics consumer could still read from earliest after seeking to a specific position. There are two reasons:

  1. incomingMessages is not cleared before seek(long timestamp)
  2. A race condition for a consumer whose start message ID is earliest:
    1. seekAsync was called and then incomingMessages was cleared
    2. Messages from the earliest position were prefetched to internal consumers and then added to incomingMessages
    3. Internal consumers were disconnected from the broker and then broker reset cursors
    4. Messages from the seek position were added to incomingMessages
    5. After seekAsync completed, Consumer#receive will poll the messages fetched before seek completed

Modifications

Add a duringSeek flag that is set with true when the seek operation starts. Then do not handle messages via messageReceive and stop receiveMessageFromConsumer if duringSeek is true. After the seek option finishes, set duringSeek back to false and restart receiveMessageFromConsumer for all consumers again.

Add testSeekToNewerPosition to verify receive, receiveAsync and message listener all work for seeking by timestamp to a newer position on a multi-topics consumer.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@BewareMyPower BewareMyPower added type/bug The PR fixed a bug or issue reported a bug release/3.0.3 release/3.1.3 release/3.2.1 labels Jan 22, 2024
@BewareMyPower BewareMyPower added this to the 3.3.0 milestone Jan 22, 2024
@BewareMyPower BewareMyPower self-assigned this Jan 22, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 22, 2024
@BewareMyPower BewareMyPower marked this pull request as draft January 23, 2024 07:21
@BewareMyPower
Copy link
Contributor Author

This issue should also exist for other seekAsync overrides, I will refactor the seek logic soon.

@BewareMyPower BewareMyPower force-pushed the bewaremypower/fix-multi-topics-consumer-seek branch from 1e7fa57 to 47237a5 Compare January 23, 2024 10:28
@BewareMyPower BewareMyPower changed the title [fix][client] Fix seeking by timestamp does not work for multi-topics [fix][client] Fix multi-topics consumer could receive old messages after seek Jan 23, 2024
@BewareMyPower BewareMyPower force-pushed the bewaremypower/fix-multi-topics-consumer-seek branch from 47237a5 to 2b2a6a8 Compare January 23, 2024 11:06
@BewareMyPower BewareMyPower marked this pull request as ready for review January 23, 2024 11:07
BewareMyPower added a commit to BewareMyPower/pulsar-client-cpp that referenced this pull request Jan 24, 2024
### Motivation

See apache/pulsar#21945

### Modifications

In C++ client, the multi-topics consumer receives messages by
configuring internal consumers with a message listener that adds
messages to `incomingMessages_`. So this patch pauses the listeners
before seek and resumes them after seek.

Add `MultiTopicsConsumerTest.testSeekToNewerPosition` for test.
Copy link
Member

@RobertIndie RobertIndie left a comment

Choose a reason for hiding this comment

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

Good work! Left one comment.

@BewareMyPower
Copy link
Contributor Author

@codelipenghui I have addressed your comments, PTAL again.

@codecov-commenter
Copy link

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (17bb322) 73.59% compared to head (1ee4441) 73.67%.
Report is 25 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #21945      +/-   ##
============================================
+ Coverage     73.59%   73.67%   +0.07%     
- Complexity    32417    32489      +72     
============================================
  Files          1861     1863       +2     
  Lines        138678   138784     +106     
  Branches      15188    15207      +19     
============================================
+ Hits         102060   102245     +185     
+ Misses        28715    28651      -64     
+ Partials       7903     7888      -15     
Flag Coverage Δ
inttests 24.08% <12.50%> (-0.05%) ⬇️
systests 23.65% <12.50%> (-0.03%) ⬇️
unittests 72.94% <84.37%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...n/java/org/apache/pulsar/broker/PulsarService.java 83.06% <100.00%> (+0.74%) ⬆️
...ache/pulsar/client/admin/internal/BrokersImpl.java 87.50% <100.00%> (ø)
...g/apache/pulsar/broker/admin/impl/BrokersBase.java 70.86% <50.00%> (ø)
...he/pulsar/client/impl/MultiTopicsConsumerImpl.java 78.41% <84.61%> (+0.31%) ⬆️

... and 113 files with indirect coverage changes

@Technoboy- Technoboy- merged commit fce0717 into apache:master Jan 30, 2024
47 checks passed
@BewareMyPower BewareMyPower deleted the bewaremypower/fix-multi-topics-consumer-seek branch January 30, 2024 17:51
BewareMyPower added a commit to apache/pulsar-client-cpp that referenced this pull request Feb 2, 2024
### Motivation

See apache/pulsar#21945

### Modifications

In C++ client, the multi-topics consumer receives messages by
configuring internal consumers with a message listener that adds
messages to `incomingMessages_`. So this patch pauses the listeners
before seek and resumes them after seek.

Add `MultiTopicsConsumerTest.testSeekToNewerPosition` for test.
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 1, 2024
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants