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 orphan ledgers in BucketDelayedDeliveryTracker #22802

Merged
merged 3 commits into from
Oct 10, 2024

Conversation

dao-jun
Copy link
Member

@dao-jun dao-jun commented May 30, 2024

Motivation

For BucketDelayedDeliveryTracker, we store the Bucket metadata(position) in Cursor#properties, and remove the property before delete ImmutableBucket.

There is a corner case that it could remove Cursor#properties succeed but delete ImmutableBucket failed.
Since we store the Bucket in Ledgers, if remove Cursor#properties succeed but delete ImmutableBucket failed, the ledger may have a change to be an orphan ledger, not reachable and not usable.

The PR is for the purpose of handle the case, remove Cursor#properties after delete ImmutableBucket, if the Bucket delete succeed but remove Cursor#properties failed we can detect it in the recover phase and remove the properties, to keep the data consistent and avoid orphan ledgers.

Modifications

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:

@dao-jun dao-jun added this to the 3.4.0 milestone May 30, 2024
@dao-jun dao-jun requested a review from coderzc May 30, 2024 05:12
@dao-jun dao-jun self-assigned this May 30, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 30, 2024
@dao-jun dao-jun changed the title [fix][broker] Remove cursor properties after Bucket deleted. [fix][broker] Avoid orphan ledgers in BucketDelayedDeliveryTracker May 30, 2024
@dao-jun dao-jun closed this May 30, 2024
@dao-jun dao-jun reopened this May 30, 2024
@lhotari lhotari added release/3.3.3 release/blocker Indicate the PR or issue that should block the release until it gets resolved labels Oct 9, 2024
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM. Good work @dao-jun.

@lhotari lhotari closed this Oct 9, 2024
@lhotari lhotari reopened this Oct 9, 2024
@dao-jun
Copy link
Member Author

dao-jun commented Oct 10, 2024

@lhotari Many thanks for your review!

merging...

@lhotari lhotari closed this Oct 10, 2024
@lhotari lhotari reopened this Oct 10, 2024
@lhotari
Copy link
Member

lhotari commented Oct 10, 2024

Needed #23431 for fixing Pulsar CI.

@codecov-commenter
Copy link

codecov-commenter commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 68.96552% with 9 lines in your changes missing coverage. Please review.

Project coverage is 74.24%. Comparing base (bbc6224) to head (13be881).
Report is 662 commits behind head on master.

Files with missing lines Patch % Lines
...r/delayed/bucket/BucketDelayedDeliveryTracker.java 63.63% 3 Missing and 1 partial ⚠️
...elayed/bucket/BookkeeperBucketSnapshotStorage.java 57.14% 1 Missing and 2 partials ⚠️
...broker/delayed/bucket/BucketNotExistException.java 50.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22802      +/-   ##
============================================
+ Coverage     73.57%   74.24%   +0.66%     
- Complexity    32624    34426    +1802     
============================================
  Files          1877     1951      +74     
  Lines        139502   147734    +8232     
  Branches      15299    16403    +1104     
============================================
+ Hits         102638   109682    +7044     
- Misses        28908    29552     +644     
- Partials       7956     8500     +544     
Flag Coverage Δ
inttests 27.92% <0.00%> (+3.33%) ⬆️
systests 24.37% <0.00%> (+0.05%) ⬆️
unittests 73.58% <68.96%> (+0.73%) ⬆️

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

Files with missing lines Coverage Δ
...r/delayed/BucketDelayedDeliveryTrackerFactory.java 93.47% <100.00%> (+2.04%) ⬆️
.../pulsar/broker/delayed/bucket/ImmutableBucket.java 85.71% <100.00%> (ø)
...broker/delayed/bucket/BucketNotExistException.java 50.00% <50.00%> (ø)
...elayed/bucket/BookkeeperBucketSnapshotStorage.java 81.25% <57.14%> (-2.49%) ⬇️
...r/delayed/bucket/BucketDelayedDeliveryTracker.java 82.53% <63.63%> (-1.17%) ⬇️

... and 625 files with indirect coverage changes

@lhotari lhotari merged commit 8b6b337 into apache:master Oct 10, 2024
93 of 97 checks passed
lhotari pushed a commit that referenced this pull request Oct 10, 2024
lhotari pushed a commit that referenced this pull request Oct 10, 2024
@dao-jun dao-jun deleted the dev/remove_cursor_properties branch October 10, 2024 18:53
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Oct 15, 2024
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker cherry-picked/branch-3.0 cherry-picked/branch-3.3 doc-not-needed Your PR changes do not impact docs ready-to-test release/blocker Indicate the PR or issue that should block the release until it gets resolved release/3.0.8 release/3.3.3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants