Skip to content

[improve][ml]Release idle offloaded read handle only the ref count is 0 #24381

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

Merged

Conversation

poorbarcode
Copy link
Contributor

Motivation

#19783 introduced a feature: release idle offloaded ledger handle to release the direct memory occupied. But it left a potential issue, which has been discussed here, and it can happen this way

time/thread release idle ledger handle ledgerHandle.readAsync
1 touch(), update lastAccessTimestamp
2 append the reading request into executor.queue
3 the executor is very busy, leading to delayed executions of ledgerHandle.readAsync
4 Check how long the ledger handle has been idled
5 release the ledger handle
6 ledgerHandle.readAsync execute eventually, but the ledger handle has been closed

Since users can modify the threshold of marking the ledger handle as idle by the configuration managedLedgerInactiveOffloadedLedgerEvictionTimeSeconds, it may increase the probability of the issue happening

Modifications

  • Added a ref count named pendingRead to record how many pending reading requests are in progress, Pulsar release ledger handle when pendingRead is 0, and has idled for enough time
  • Instead of updating "lastAccessTimestamp" when a reading is starting, update it when a reading task is finished

Documentation

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

Matching PR in forked repository

PR in forked repository: x

@poorbarcode poorbarcode added this to the 4.1.0 milestone Jun 5, 2025
@poorbarcode poorbarcode self-assigned this Jun 5, 2025
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 5, 2025
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, great work @poorbarcode

@codecov-commenter
Copy link

codecov-commenter commented Jun 5, 2025

Codecov Report

Attention: Patch coverage is 68.75000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 74.26%. Comparing base (bbc6224) to head (00846f5).
Report is 1131 commits behind head on master.

Files with missing lines Patch % Lines
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java 42.85% 3 Missing and 1 partial ⚠️
...oad/jcloud/impl/BlobStoreBackedReadHandleImpl.java 87.50% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #24381      +/-   ##
============================================
+ Coverage     73.57%   74.26%   +0.68%     
+ Complexity    32624    32241     -383     
============================================
  Files          1877     1867      -10     
  Lines        139502   145267    +5765     
  Branches      15299    16610    +1311     
============================================
+ Hits         102638   107881    +5243     
+ Misses        28908    28852      -56     
- Partials       7956     8534     +578     
Flag Coverage Δ
inttests 26.66% <0.00%> (+2.08%) ⬆️
systests 23.27% <37.50%> (-1.06%) ⬇️
unittests 73.76% <68.75%> (+0.91%) ⬆️

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

Files with missing lines Coverage Δ
...ache/bookkeeper/mledger/OffloadedLedgerHandle.java 50.00% <100.00%> (ø)
...oad/jcloud/impl/BlobStoreBackedReadHandleImpl.java 77.77% <87.50%> (-2.37%) ⬇️
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java 81.03% <42.85%> (+0.37%) ⬆️

... and 1082 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 21406b5 into apache:master Jun 5, 2025
119 of 125 checks passed
@poorbarcode poorbarcode deleted the improve/idle_offloaded_ledger_handle branch June 5, 2025 10:29
lhotari pushed a commit that referenced this pull request Jun 9, 2025
lhotari pushed a commit that referenced this pull request Jun 9, 2025
lhotari pushed a commit that referenced this pull request Jun 9, 2025
manas-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 12, 2025
… 0 (apache#24381)

(cherry picked from commit 21406b5)
(cherry picked from commit 639521b)
manas-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 12, 2025
… 0 (apache#24381)

(cherry picked from commit 21406b5)
(cherry picked from commit dcbe23f)
manas-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 12, 2025
… 0 (apache#24381)

(cherry picked from commit 21406b5)
(cherry picked from commit 639521b)
manas-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 12, 2025
… 0 (apache#24381)

(cherry picked from commit 21406b5)
(cherry picked from commit 639521b)
manas-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 13, 2025
… 0 (apache#24381)

(cherry picked from commit 21406b5)
(cherry picked from commit 639521b)
manas-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 13, 2025
… 0 (apache#24381)

(cherry picked from commit 21406b5)
(cherry picked from commit 639521b)
manas-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 13, 2025
… 0 (apache#24381)

(cherry picked from commit 21406b5)
(cherry picked from commit dcbe23f)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 16, 2025
… 0 (apache#24381)

(cherry picked from commit 21406b5)
(cherry picked from commit 639521b)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 16, 2025
… 0 (apache#24381)

(cherry picked from commit 21406b5)
(cherry picked from commit dcbe23f)
nodece pushed a commit to nodece/pulsar that referenced this pull request Jun 18, 2025
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.

3 participants