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 issue with consumer read uncommitted messages from compacted topic #21465

Merged

Conversation

coderzc
Copy link
Member

@coderzc coderzc commented Oct 29, 2023

Fixes #21464

Motivation

The consumer shouldn't read ongoing messages from compacted topic.

Modifications

Pass topic.getMaxReadPosition() to cursor when reading messages 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.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Documentation

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

Matching PR in forked repository

PR in forked repository:

@coderzc coderzc self-assigned this Oct 29, 2023
@coderzc coderzc requested review from liangyepianzhou, gaoran10 and codelipenghui and removed request for liangyepianzhou October 29, 2023 11:51
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 29, 2023
@coderzc coderzc added type/bug The PR fixed a bug or issue reported a bug area/broker labels Oct 29, 2023
@coderzc coderzc added this to the 3.2.0 milestone Oct 29, 2023
@coderzc coderzc closed this Nov 2, 2023
@coderzc coderzc reopened this Nov 2, 2023
@coderzc coderzc changed the title [fix][bug] Fix issue with consumer read uncommitted messages from compacted topic [fix][broker] Fix issue with consumer read uncommitted messages from compacted topic Nov 3, 2023
Transaction txn = pulsarClient.newTransaction()
.withTransactionTimeout(1, TimeUnit.MINUTES).build().get();
producer.newMessage(txn).key("K2").value("V2").send();
producer.newMessage(txn).key("K3").value("V3").send();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we send some messages with the same key?

Copy link
Member Author

Choose a reason for hiding this comment

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

It shouldn't be necessary, we just need to enable readCompacted for consumers then consumers will read messages from the compacted topic although the compaction task may not have run yet.

Copy link
Contributor

@gaoran10 gaoran10 left a comment

Choose a reason for hiding this comment

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

Lgtm

@codelipenghui codelipenghui merged commit b5925ed into apache:master Nov 3, 2023
71 of 82 checks passed
@BewareMyPower
Copy link
Contributor

@coderzc I think this PR should not be cherry-picked to release branches because the API of asyncReadCompactedEntries is modified.

@coderzc
Copy link
Member Author

coderzc commented Nov 8, 2023

@coderzc I think this PR should not be cherry-picked to release branches because the API of asyncReadCompactedEntries is modified.

Oh, we can cherry-pick it to release branches through add new asyncReadCompactedEntries.

coderzc added a commit to coderzc/pulsar that referenced this pull request Nov 13, 2023
nborisov pushed a commit to nborisov/pulsar that referenced this pull request Nov 13, 2023
coderzc added a commit that referenced this pull request Nov 13, 2023
coderzc added a commit to coderzc/pulsar that referenced this pull request Nov 20, 2023
coderzc added a commit to coderzc/pulsar that referenced this pull request Nov 21, 2023
Technoboy- pushed a commit that referenced this pull request Nov 22, 2023
@Technoboy-
Copy link
Contributor

Cherry-picked by #21570

coderzc added a commit that referenced this pull request Dec 14, 2023
@coderzc coderzc deleted the fix_compaction_readMaxPosition branch December 14, 2023 14:50
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 20, 2023
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 20, 2023
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 20, 2023
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 20, 2023
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 20, 2023
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 20, 2023
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 1, 2024
…es from compacted topic (apache#21465)"

This reverts commit 80f921a.

(cherry picked from commit c616deb)
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
…es from compacted topic (apache#21465)"

This reverts commit 80f921a.

(cherry picked from commit c616deb)
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.

[Bug] Consumer reads uncommitted messages from compacted topic
9 participants