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] Avoid consumers receiving acknowledged messages from compacted topic after reconnection #21187

Merged
merged 15 commits into from Jan 26, 2024

Conversation

coderzc
Copy link
Member

@coderzc coderzc commented Sep 15, 2023

Fixes #21074

Motivation

In #11287, we use MessageId.earliest instead of the earliest available message to read on the topic when we first read after reconnection, this will lead to consumers receiving acknowledged messages from the compacted topic after reconnection, so I think for a durable cursor (exclude __compaction cursor), we should read messages from the mark-delete position.

Modifications

  • If a durable cursor (exclude __compaction cursor) has the mark-delete position, don't read messages from the earliest.
  • To avoid skipping the remaining data in the compacted ledger after rewind, we only skip invalid md-position when recovering cursor and add rewind(boolean readCompacted) method in ManagedCursor and just rewind to the next message of the mark delete position instead of resetting the read position to the next valid position of the original topic.
  • This change will also make subscriptionInitialPosition=Latest work normally since the mark-delete doesn't is invalid position (entryID==-1L).

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

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:

@coderzc coderzc marked this pull request as draft September 15, 2023 03:07
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 15, 2023
@coderzc coderzc changed the title [fix][broker] Avoid consumers receiving acknowledged messages from a compacted topic upon reconnection [fix][broker] Avoid consumers receiving acknowledged messages from compacted topic after reconnection Sep 15, 2023
@coderzc coderzc self-assigned this Sep 18, 2023
@coderzc coderzc added this to the 3.2.0 milestone Sep 18, 2023
@coderzc coderzc marked this pull request as ready for review September 18, 2023 10:39
@codelipenghui codelipenghui added the category/reliability The function does not work properly in certain specific environments or failures. e.g. data lost label Sep 22, 2023
poorbarcode
poorbarcode previously approved these changes Sep 27, 2023
@coderzc coderzc marked this pull request as draft September 29, 2023 00:46
@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Oct 29, 2023
@Technoboy- Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023
@coderzc coderzc marked this pull request as ready for review January 16, 2024 04:03
@coderzc coderzc force-pushed the fix_compaction_ack branch 2 times, most recently from 0624b09 to a9dcdfc Compare January 16, 2024 05:58
@coderzc coderzc added type/bug The PR fixed a bug or issue reported a bug area/broker labels Jan 16, 2024
@coderzc coderzc force-pushed the fix_compaction_ack branch 2 times, most recently from f09bc9a to ee9dd6c Compare January 16, 2024 06:17
@codelipenghui codelipenghui merged commit e81a20d into apache:master Jan 26, 2024
50 of 51 checks passed
@coderzc coderzc deleted the fix_compaction_ack branch January 26, 2024 09:31
@Technoboy- Technoboy- modified the milestones: 3.3.0, 3.2.0 Jan 27, 2024
Technoboy- pushed a commit that referenced this pull request Jan 27, 2024
Technoboy- pushed a commit that referenced this pull request Jan 31, 2024
@heesung-sn
Copy link
Contributor

@coderzc could you help to cherry-pick this to branch-3.0? I see conflict.

coderzc added a commit to coderzc/pulsar that referenced this pull request Feb 28, 2024
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 1, 2024
…mpacted topic after reconnection (apache#21187)

(cherry picked from commit 24d8d9a)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 6, 2024
…mpacted topic after reconnection (apache#21187)

(cherry picked from commit 24d8d9a)
heesung-sn pushed a commit 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
Labels
area/broker area/compaction category/reliability The function does not work properly in certain specific environments or failures. e.g. data lost cherry-picked/branch-3.0 cherry-picked/branch-3.1 doc-not-needed Your PR changes do not impact docs ready-to-test release/2.10.7 release/2.11.5 release/3.0.3 release/3.1.3 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Topic unloading during Compaction leads to messages loss and messages redelivery loop
9 participants