Skip to content

[fix][ml]Still got BK ledger, even though it has been deleted after offloaded #24432

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.

Sign up for GitHub

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

Jump to bottom
Merged
lhotari merged 3 commits into from
Jun 20, 2025
Merged

[fix][ml]Still got BK ledger, even though it has been deleted after offloaded #24432

lhotari merged 3 commits into from
Jun 20, 2025

Conversation

@poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Jun 19, 2025

Motivation

The reproduction steps of the issue:

  • There are 4 ledgers in the ManagedLedger
  • 3 ledgers are offloaded.
  • The BK ledgers are deleted because they were offloaded.
  • Issue:
    • Actual behavior: ManagedLedger.getLedgerHandle(long ledgerId) returns BK ledger handles, which have been deleted
    • Expected behavior: ManagedLedger.getLedgerHandle(long ledgerId) returns Offloader ledger handles.

Root cause
Trimming ledgers, BK ledgers are deleted, and LedgerCache of ManagedLedger does not remove the cached ledger handle in memory.

Modifications

  • In scope: Remove cached ledger handles when trimming offloaded ledgers.
  • Not in scope: This PR will not solve the issue: Ledger is being deleted, but a cursor still uses the ledger handle to read entries. This issue will be resolved automatically because ManagedLedger will recreate the ledger handle when it receives a failed read entry callback

Documentation

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

Matching PR in forked repository

PR in forked repository: x

@poorbarcode poorbarcode self-assigned this Jun 19, 2025
@poorbarcode poorbarcode added this to the 4.1.0 milestone Jun 19, 2025
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 19, 2025
@poorbarcode poorbarcode requested a review from lhotari June 19, 2025 14:31
lhotari
lhotari approved these changes Jun 19, 2025
View reviewed changes
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 @poorbarcode

@codelipenghui
Copy link
Contributor

codelipenghui commented Jun 19, 2025

/pulsarbot run-failure-checks

@poorbarcode poorbarcode requested a review from Technoboy- June 20, 2025 01:33
@codecov-commenter
Copy link

codecov-commenter commented Jun 20, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 61.11111% with 7 lines in your changes missing coverage. Please review.

Project coverage is 74.27%. Comparing base (bbc6224) to head (7f98cf6).
Report is 1158 commits behind head on master.

Files with missing lines Patch % Lines
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java 61.11% 7 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #24432      +/-   ##
============================================
+ Coverage     73.57%   74.27%   +0.69%     
+ Complexity    32624    32334     -290     
============================================
  Files          1877     1868       -9     
  Lines        139502   145397    +5895     
  Branches      15299    16636    +1337     
============================================
+ Hits         102638   107987    +5349     
+ Misses        28908    28858      -50     
- Partials       7956     8552     +596
Flag Coverage Δ
inttests 26.81% <22.22%> (+2.23%) ⬆️
systests 23.39% <55.55%> (-0.94%) ⬇️
unittests 73.75% <61.11%> (+0.91%) ⬆️

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

Files with missing lines Coverage Δ
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java 81.17% <61.11%> (+0.50%) ⬆️

... and 1085 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@lhotari lhotari merged commit 73a4ae4 into apache:master Jun 20, 2025
147 of 151 checks passed
lhotari pushed a commit that referenced this pull request Jun 20, 2025
@poorbarcode @lhotari
[fix][ml]Still got BK ledger, even though it has been deleted after o…
e046212 
…ffloaded (#24432)

(cherry picked from commit 73a4ae4)
lhotari pushed a commit that referenced this pull request Jun 20, 2025
@poorbarcode @lhotari
[fix][ml]Still got BK ledger, even though it has been deleted after o…
6b5fdbf 
…ffloaded (#24432)

(cherry picked from commit 73a4ae4)
lhotari pushed a commit that referenced this pull request Jun 20, 2025
@poorbarcode @lhotari
[fix][ml]Still got BK ledger, even though it has been deleted after o…
da319a9 
…ffloaded (#24432)

(cherry picked from commit 73a4ae4)
ganesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 21, 2025
@poorbarcode @ganesh-ctds
[fix][ml]Still got BK ledger, even though it has been deleted after o…
92f292c 
…ffloaded (apache#24432)

(cherry picked from commit 73a4ae4)
(cherry picked from commit e046212)
nodece pushed a commit to ascentstream/pulsar that referenced this pull request Jun 24, 2025
@poorbarcode @nodece
[fix][ml]Still got BK ledger, even though it has been deleted after o…
8d0c868 
…ffloaded (apache#24432)

(cherry picked from commit 73a4ae4)
ganesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 24, 2025
@poorbarcode @ganesh-ctds
[fix][ml]Still got BK ledger, even though it has been deleted after o…
95b83fa 
…ffloaded (apache#24432)

(cherry picked from commit 73a4ae4)
(cherry picked from commit 6b5fdbf)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 24, 2025
@poorbarcode @srinath-ctds
[fix][ml]Still got BK ledger, even though it has been deleted after o…
a10e9c8 
…ffloaded (apache#24432)

(cherry picked from commit 73a4ae4)
(cherry picked from commit e046212)
ganesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 27, 2025
@poorbarcode @ganesh-ctds
[fix][ml]Still got BK ledger, even though it has been deleted after o…
c38f197 
…ffloaded (apache#24432)

(cherry picked from commit 73a4ae4)
(cherry picked from commit 6b5fdbf)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 30, 2025
@poorbarcode @srinath-ctds
[fix][ml]Still got BK ledger, even though it has been deleted after o…
3387d98 
…ffloaded (apache#24432)

(cherry picked from commit 73a4ae4)
(cherry picked from commit 6b5fdbf)
KannarFr pushed a commit to CleverCloud/pulsar that referenced this pull request Sep 22, 2025
@poorbarcode @KannarFr
[fix][ml]Still got BK ledger, even though it has been deleted after o…
cb67b4b 
…ffloaded (apache#24432)
walkinggo pushed a commit to walkinggo/pulsar that referenced this pull request Oct 8, 2025
@poorbarcode @walkinggo
[fix][ml]Still got BK ledger, even though it has been deleted after o…
ec88c8e 
…ffloaded (apache#24432)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lhotari lhotari lhotari approved these changes

@Technoboy- Technoboy- Technoboy- approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@coderzc coderzc coderzc approved these changes

+1 more reviewer

@wangchao316 wangchao316 wangchao316 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@poorbarcode poorbarcode

Labels

cherry-picked/branch-3.0 cherry-picked/branch-3.3 cherry-picked/branch-4.0 doc-not-needed Your PR changes do not impact docs ready-to-test release/3.0.13 release/3.3.8 release/4.0.6

Projects

None yet

Milestone

4.1.0

Development

Successfully merging this pull request may close these issues.

7 participants

@poorbarcode @codelipenghui @codecov-commenter @lhotari @Technoboy- @coderzc @wangchao316