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 block markDeletePosition forward when skip lost entries #21210

Merged
merged 10 commits into from
Dec 16, 2024

Conversation

hanmz
Copy link
Contributor

@hanmz hanmz commented Sep 20, 2023

Motivation

The configuration autoSkipNonRecoverableData is designed to turn this feature on if we can accept partial data loss. When some entries is lost, the broker will still work. cursor.readPosition will move to next position for skip these positions. But now we have this problem: cursor.readPosition can move to next positition, but the cursor.markDeletePosition can not forward.

for example:

  1. topic-1 has some entries: [1:1],[1:2],[1:3],[1:4],[1:5],[1:6],[1:7],[1:8]
  2. entry [1,4] and [1,5] lost
  3. cursor read entry from ledger, then send message [1:1],[1:2],[1:3],[1:6],[1:7] to consumer. (Current: readPosition=[1:8], markDeletePosition=[1:1])
  4. consumer individual ack [1:1],[1:2],[1:3],[1:6],[1:7] (Current: readPosition=[1:8], markDeletePosition=[1:3])
  5. We can't delete the entry [1:4] and [1:5] because these lost. So markDeletePosition will stay at [1:3] and can not move forward to [1:8]

The root cause is: When entries lost, lost entries not be delete from cursor.

Modifications

When skip lost entries, delete these entries from cursor.

Documentation

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

Matching PR in forked repository

PR in forked repository: hanmz#4

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 20, 2023
Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

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

overall LGTM.

@poorbarcode poorbarcode added this to the 3.2.0 milestone Sep 26, 2023
Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

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

LGTM

@hanmz
Copy link
Contributor Author

hanmz commented Oct 8, 2023

/pulsarbot rerun-failure-checks

@hanmz
Copy link
Contributor Author

hanmz commented Oct 8, 2023

/pulsarbot rerun-failure-checks

@hanmz hanmz requested a review from Jason918 October 8, 2023 13:13
Copy link

github-actions bot commented Nov 9, 2023

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

@github-actions github-actions bot added the Stale label Nov 9, 2023
@lhotari lhotari requested a review from hangc0276 November 9, 2023 06:30
@lhotari lhotari closed this Nov 9, 2023
@lhotari lhotari reopened this Nov 9, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 9, 2023

Codecov Report

Attention: Patch coverage is 68.42105% with 6 lines in your changes missing coverage. Please review.

Project coverage is 74.45%. Comparing base (bbc6224) to head (5d8f8e3).
Report is 792 commits behind head on master.

Files with missing lines Patch % Lines
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java 72.22% 2 Missing and 3 partials ⚠️
...rg/apache/bookkeeper/mledger/impl/OpReadEntry.java 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #21210      +/-   ##
============================================
+ Coverage     73.57%   74.45%   +0.87%     
- Complexity    32624    35113    +2489     
============================================
  Files          1877     1945      +68     
  Lines        139502   147483    +7981     
  Branches      15299    16276     +977     
============================================
+ Hits         102638   109802    +7164     
- Misses        28908    29217     +309     
- Partials       7956     8464     +508     
Flag Coverage Δ
inttests 27.26% <0.00%> (+2.68%) ⬆️
systests 24.36% <0.00%> (+0.04%) ⬆️
unittests 73.84% <68.42%> (+1.00%) ⬆️

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

Files with missing lines Coverage Δ
...rg/apache/bookkeeper/mledger/impl/OpReadEntry.java 83.49% <0.00%> (-0.82%) ⬇️
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java 80.06% <72.22%> (+0.76%) ⬆️

... and 672 files with indirect coverage changes

@github-actions github-actions bot removed the Stale label Nov 10, 2023
@Technoboy- Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023
# Conflicts:
#	managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java
@hanmz
Copy link
Contributor Author

hanmz commented Apr 1, 2024

If you have time, please help me take a look, thank you very much. @Technoboy- @poorbarcode

@coderzc coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024
@lhotari lhotari added the release/blocker Indicate the PR or issue that should block the release until it gets resolved label Oct 9, 2024
@lhotari lhotari requested a review from rdhabalia October 9, 2024 13:53
@lhotari lhotari modified the milestones: 4.0.0, 4.1.0 Oct 11, 2024
# Conflicts:
#	managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java
@hanmz hanmz closed this Dec 9, 2024
@hanmz hanmz reopened this Dec 9, 2024
@Jason918 Jason918 merged commit 3761dc4 into apache:master Dec 16, 2024
54 checks passed
lhotari pushed a commit that referenced this pull request Dec 16, 2024
…ntries (#21210)

Co-authored-by: Lari Hotari <lhotari@users.noreply.github.com>
Co-authored-by: Lari Hotari <lhotari@apache.org>
(cherry picked from commit 3761dc4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-4.0 doc-not-needed Your PR changes do not impact docs release/blocker Indicate the PR or issue that should block the release until it gets resolved release/4.0.2 triage/lhotari/important lhotari's triaging label for important issues or PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants