[fix][ml]Revert a behavior change of releasing idle offloaded ledger handle: only release idle BlobStoreBackedReadHandle #24384

poorbarcode · 2025-06-05T12:14:58Z

Motivation

#24381 changed the behavior below:

original: Pulsar only releases the offloaded ledger handle that is typed BlobStoreBackedReadHandle
after [improve][ml]Release idle offloaded read handle only the ref count is 0 #24381, Pulsar will release the offloaded ledger handle immediately if its type is not BlobStoreBackedReadHandle, because the method lastAccessTimestamp() always returns -1 if its type is not BlobStoreBackedReadHandle

Modifications

Revert the behavior change.
Modify lastAccessTimestamp first, and modify pendingReading second, to avoid getting a new pendingReading and an older lastAccessTimestamp, which leads to an incorrect ledger handle invalidation.

Potential issue

The following issue still exists. I will push a new PR to fix it in the future.

time/thread	release idle ledger handle	`ledgerHandle.readAsync`
1	Ledger handle has been idle for a long time
2	Start to release ledger handle
3		a new reading request comes in
4	closed ledger handle
5		get a ledger closed error

Documentation

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

Matching PR in forked repository

PR in forked repository: x

lhotari

LGTM

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java

codecov-commenter · 2025-06-05T13:35:40Z

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.25%. Comparing base (bbc6224) to head (0a4eca2).
Report is 1132 commits behind head on master.

Files with missing lines	Patch %	Lines
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #24384      +/-   ##
============================================
+ Coverage     73.57%   74.25%   +0.67%     
+ Complexity    32624    32301     -323     
============================================
  Files          1877     1867      -10     
  Lines        139502   145268    +5766     
  Branches      15299    16610    +1311     
============================================
+ Hits         102638   107867    +5229     
+ Misses        28908    28863      -45     
- Partials       7956     8538     +582

Flag	Coverage Δ
inttests	`26.79% <0.00%> (+2.21%)`	⬆️
systests	`23.34% <50.00%> (-0.99%)`	⬇️
unittests	`73.73% <66.66%> (+0.89%)`	⬆️

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

Files with missing lines	Coverage Δ
...oad/jcloud/impl/BlobStoreBackedReadHandleImpl.java	`77.77% <100.00%> (-2.37%)`	⬇️
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java	`81.28% <50.00%> (+0.61%)`	⬆️

... and 1080 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 · 2025-06-05T13:52:00Z

original: Pulsar only releases the offloaded ledger handle that is typed BlobStoreBackedReadHandle

after [improve][ml]Release idle offloaded read handle only the ref count is 0 #24381, Pulsar will release the offloaded ledger handle immediately if its type is not BlobStoreBackedReadHandle, because the method lastAccessTimestamp() always returns -1 if its type is not BlobStoreBackedReadHandle

the title "[fix][ml]Revert a behavior change of releasing idle offloaded ledger handle: only release idle BlobStoreBackedReadHandle " is slightly misleading since even before it would only release idle BlobStoreBackedReadHandleImpls since that's the only implementation of OffloadedLedgerHandle.
Revisiting the PR title to focus on an improvement to avoid a remaining race condition could be better.

…dle: only release idle BlobStoreBackedReadHandle

…handle: only release idle BlobStoreBackedReadHandle (#24384) (cherry picked from commit e0d7faa)

…handle: only release idle BlobStoreBackedReadHandle (apache#24384) (cherry picked from commit e0d7faa) (cherry picked from commit 9adbbbf)

…handle: only release idle BlobStoreBackedReadHandle (apache#24384) (cherry picked from commit e0d7faa) (cherry picked from commit 1294a56)

…handle: only release idle BlobStoreBackedReadHandle (apache#24384) (cherry picked from commit e0d7faa) (cherry picked from commit 9adbbbf)

…handle: only release idle BlobStoreBackedReadHandle (apache#24384) (cherry picked from commit e0d7faa) (cherry picked from commit 1294a56)

…handle: only release idle BlobStoreBackedReadHandle (apache#24384) (cherry picked from commit e0d7faa) (cherry picked from commit 1294a56) (cherry picked from commit 36b8450)

…handle: only release idle BlobStoreBackedReadHandle (apache#24384) (cherry picked from commit e0d7faa) (cherry picked from commit 9adbbbf)

…handle: only release idle BlobStoreBackedReadHandle (apache#24384) (cherry picked from commit e0d7faa) (cherry picked from commit 1294a56) (cherry picked from commit 36b8450)

…handle: only release idle BlobStoreBackedReadHandle (apache#24384) (cherry picked from commit e0d7faa)

…handle: only release idle BlobStoreBackedReadHandle (apache#24384)

poorbarcode self-assigned this Jun 5, 2025

poorbarcode added ready-to-test release/3.0.13 release/4.0.6 release/3.3.8 labels Jun 5, 2025

poorbarcode added this to the 4.1.0 milestone Jun 5, 2025

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 5, 2025

lhotari approved these changes Jun 5, 2025

View reviewed changes

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java Show resolved Hide resolved

[fix][ml]Revert a break change of releasing idle offloaded ledger han…

0a4eca2

…dle: only release idle BlobStoreBackedReadHandle

poorbarcode force-pushed the fix/offloaded_ledger_lastAccessTimestamp branch from 589edb7 to 0a4eca2 Compare June 6, 2025 07:51

lhotari merged commit e0d7faa into apache:master Jun 9, 2025
53 checks passed

lhotari pushed a commit that referenced this pull request Jun 9, 2025

[fix][ml]Revert a behavior change of releasing idle offloaded ledger …

1294a56

…handle: only release idle BlobStoreBackedReadHandle (#24384) (cherry picked from commit e0d7faa)

lhotari pushed a commit that referenced this pull request Jun 9, 2025

[fix][ml]Revert a behavior change of releasing idle offloaded ledger …

80c4ca2

…handle: only release idle BlobStoreBackedReadHandle (#24384) (cherry picked from commit e0d7faa)

lhotari added cherry-picked/branch-3.0 cherry-picked/branch-3.3 labels Jun 9, 2025

lhotari pushed a commit that referenced this pull request Jun 9, 2025

[fix][ml]Revert a behavior change of releasing idle offloaded ledger …

9adbbbf

…handle: only release idle BlobStoreBackedReadHandle (#24384) (cherry picked from commit e0d7faa)

lhotari added the cherry-picked/branch-4.0 label Jun 9, 2025

nodece pushed a commit to nodece/pulsar that referenced this pull request Jun 18, 2025

[fix][ml]Revert a behavior change of releasing idle offloaded ledger …

e47de09

…handle: only release idle BlobStoreBackedReadHandle (apache#24384) (cherry picked from commit e0d7faa)

KannarFr pushed a commit to CleverCloud/pulsar that referenced this pull request Sep 22, 2025

[fix][ml]Revert a behavior change of releasing idle offloaded ledger …

d893dfb

…handle: only release idle BlobStoreBackedReadHandle (apache#24384)

walkinggo pushed a commit to walkinggo/pulsar that referenced this pull request Oct 8, 2025

[fix][ml]Revert a behavior change of releasing idle offloaded ledger …

d7ad6a2

…handle: only release idle BlobStoreBackedReadHandle (apache#24384)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[fix][ml]Revert a behavior change of releasing idle offloaded ledger handle: only release idle BlobStoreBackedReadHandle #24384

[fix][ml]Revert a behavior change of releasing idle offloaded ledger handle: only release idle BlobStoreBackedReadHandle #24384

Uh oh!

poorbarcode commented Jun 5, 2025

Uh oh!

lhotari left a comment

Uh oh!

Uh oh!

codecov-commenter commented Jun 5, 2025 •

edited

Loading

Uh oh!

lhotari commented Jun 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[fix][ml]Revert a behavior change of releasing idle offloaded ledger handle: only release idle BlobStoreBackedReadHandle #24384

[fix][ml]Revert a behavior change of releasing idle offloaded ledger handle: only release idle BlobStoreBackedReadHandle #24384

Uh oh!

Conversation

poorbarcode commented Jun 5, 2025

Motivation

Modifications

Potential issue

Documentation

Matching PR in forked repository

Uh oh!

lhotari left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lhotari commented Jun 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Jun 5, 2025 •

edited

Loading