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] [broker] Fix reader stuck when read from compacted topic with read compact mode disable #21969

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

thetumbled
Copy link
Member

@thetumbled thetumbled commented Jan 25, 2024

Motivation

reader.hasMessageAvailable() rely on getLastMessageId to determine whether there are any messages available to read.
org.apache.pulsar.broker.service.ServerCnx#handleGetLastMessageId will read the last message from compacted topic in some cases without check if the consumer is in read compact mode.
If the consumer/reader disable the read compact mode, and the cluster enable the compaction feature, messages will be compacted into the compacted topic, but the dispactcher will not dispatch messages from compacted topic to consumer. If the messages in the original topic is expired and the consumer has nothing to be read, while the messages in the compacted topic retain alive.
reader.hasMessageAvailable() will return true as there are messages in the compacted topic, but consumer/reader will not get any messages because there are nothing in the original topic.

Modifications

reader.hasMessageAvailable() should check if the consumer is in read compact mode to determine whether get the last message id from the compacted topic.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.
This change added tests and can be verified as follows:

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository: thetumbled#38

Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

Nice catch!

@thetumbled
Copy link
Member Author

/pulsarbot rerun-failure-checks

@codecov-commenter
Copy link

Codecov Report

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

Comparison is base (4198ed2) 73.61% compared to head (807fd1a) 73.58%.
Report is 11 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #21969      +/-   ##
============================================
- Coverage     73.61%   73.58%   -0.03%     
- Complexity    32424    32453      +29     
============================================
  Files          1861     1863       +2     
  Lines        138678   138780     +102     
  Branches      15184    15207      +23     
============================================
+ Hits         102089   102123      +34     
- Misses        28690    28756      +66     
- Partials       7899     7901       +2     
Flag Coverage Δ
inttests 24.10% <57.14%> (+0.01%) ⬆️
systests 23.62% <50.00%> (-0.13%) ⬇️
unittests 72.87% <78.57%> (-0.02%) ⬇️

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

Files Coverage Δ
...va/org/apache/pulsar/broker/service/ServerCnx.java 72.14% <78.57%> (-0.04%) ⬇️

... and 96 files with indirect coverage changes

@Technoboy- Technoboy- merged commit 09559c5 into apache:master Jan 30, 2024
47 checks passed
@Technoboy- Technoboy- changed the title [fix] [broker] reader stuck when read from compacted topic with read compact mode disable [fix] [broker] Fix reader stuck when read from compacted topic with read compact mode disable Jan 30, 2024
Technoboy- pushed a commit that referenced this pull request Jan 31, 2024
@heesung-sn
Copy link
Contributor

@thetumbled can you help to cherry-pick this PR to branch-3.0? I see conflicts.

@thetumbled
Copy link
Member Author

@thetumbled can you help to cherry-pick this PR to branch-3.0? I see conflicts.

We have better wait #22130 to be merged, or two pr will conflict. I will help to cherry pick this pr.

@thetumbled
Copy link
Member Author

thetumbled commented Mar 5, 2024

@thetumbled can you help to cherry-pick this PR to branch-3.0? I see conflicts.

cherry pick in this pr: #22199

mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 6, 2024
nodece pushed a commit to nodece/pulsar that referenced this pull request Dec 27, 2024
…ead compact mode disable (apache#21969)

(cherry picked from commit 09559c5)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
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.

8 participants