Skip to content

[improve][ml] Optimize ledger opening by skipping fully acknowledged ledgers #24655

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
coderzc merged 2 commits into from
Aug 25, 2025
Merged

[improve][ml] Optimize ledger opening by skipping fully acknowledged ledgers #24655

coderzc merged 2 commits into from
Aug 25, 2025

Conversation

@coderzc
Copy link
Member

@coderzc coderzc commented Aug 21, 2025 •
edited
Loading

Motivation

We have an optimization(#21739) that skips messages that have already been acknowledged, but that doesn't prevent the ledger been opened. We can optimize it further by skipping the opening of those ledgers where all entries have already been acknowledged

Modifications

Check if all entries in this ledger have been deleted (acknowledged). If so, skip opening the ledger and move to the next one.

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:

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 21, 2025
@coderzc coderzc force-pushed the skip_open_ledger_filly_acked branch 2 times, most recently from 75d463e to 9e68908 Compare August 21, 2025 10:47
@coderzc coderzc force-pushed the skip_open_ledger_filly_acked branch from 9e68908 to d469857 Compare August 21, 2025 10:49
@coderzc coderzc added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/ML labels Aug 21, 2025
@coderzc coderzc added this to the 4.1.0 milestone Aug 21, 2025
@coderzc coderzc self-assigned this Aug 21, 2025
@coderzc coderzc closed this Aug 21, 2025
@coderzc coderzc reopened this Aug 21, 2025
@codecov-commenter
Copy link

codecov-commenter commented Aug 21, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 83.87097% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.34%. Comparing base (d3c615c) to head (ac47c76).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java 81.48% 2 Missing and 3 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #24655      +/-   ##
============================================
- Coverage     74.35%   74.34%   -0.02%     
- Complexity    32711    33209     +498     
============================================
  Files          1884     1884              
  Lines        146913   146943      +30     
  Branches      16874    16882       +8     
============================================
+ Hits         109244   109245       +1     
- Misses        29002    29037      +35     
+ Partials       8667     8661       -6
Flag Coverage Δ
inttests 26.91% <38.70%> (+0.24%) ⬆️
systests 23.35% <38.70%> (-0.03%) ⬇️
unittests 73.82% <83.87%> (-0.02%) ⬇️

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

Files with missing lines Coverage Δ
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java 78.36% <100.00%> (+0.30%) ⬆️
...rg/apache/bookkeeper/mledger/impl/OpReadEntry.java 76.00% <100.00%> (+0.48%) ⬆️
...ava/org/apache/bookkeeper/mledger/impl/OpScan.java 82.53% <ø> (ø)
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java 81.36% <81.48%> (+0.23%) ⬆️

... and 87 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.

@codelipenghui
Copy link
Contributor

codelipenghui commented Aug 21, 2025

I have added label release/4.0.7 since this will also fix an OOM issue which caused by opened too many ledgers from Tiered storage in a short period of time.

codelipenghui
codelipenghui approved these changes Aug 22, 2025
View reviewed changes
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

LGTM

@coderzc coderzc merged commit da0d116 into apache:master Aug 25, 2025
118 of 121 checks passed
@coderzc coderzc deleted the skip_open_ledger_filly_acked branch August 25, 2025 02:21
coderzc added a commit that referenced this pull request Aug 25, 2025
@coderzc
[improve][ml] Optimize ledger opening by skipping fully acknowledged …
9b66080 
…ledgers (#24655)

(cherry picked from commit da0d116)
@lhotari lhotari mentioned this pull request Aug 25, 2025
Merged
14 tasks
@coderzc coderzc added the category/performance Performance issues fix or improvements label Aug 25, 2025
manas-ctds pushed a commit to datastax/pulsar that referenced this pull request Aug 26, 2025
@coderzc @manas-ctds
[improve][ml] Optimize ledger opening by skipping fully acknowledged …
9bfc31b 
…ledgers (apache#24655)

(cherry picked from commit da0d116)
(cherry picked from commit 9b66080)
manas-ctds pushed a commit to datastax/pulsar that referenced this pull request Aug 26, 2025
@coderzc @manas-ctds
[improve][ml] Optimize ledger opening by skipping fully acknowledged …
9f2beb8 
…ledgers (apache#24655)

(cherry picked from commit da0d116)
(cherry picked from commit 9b66080)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Aug 28, 2025
@coderzc @srinath-ctds
[improve][ml] Optimize ledger opening by skipping fully acknowledged …
a04beaf 
…ledgers (apache#24655)

(cherry picked from commit da0d116)
(cherry picked from commit 9b66080)
Technoboy- pushed a commit to Technoboy-/pulsar that referenced this pull request Sep 10, 2025
@coderzc @Technoboy-
[improve][ml] Optimize ledger opening by skipping fully acknowledged …
d5a2b96 
…ledgers (apache#24655)

(cherry picked from commit da0d116)
KannarFr pushed a commit to CleverCloud/pulsar that referenced this pull request Sep 22, 2025
@coderzc @KannarFr
[improve][ml] Optimize ledger opening by skipping fully acknowledged …
faa440c 
…ledgers (apache#24655)
walkinggo pushed a commit to walkinggo/pulsar that referenced this pull request Oct 8, 2025
@coderzc @walkinggo
[improve][ml] Optimize ledger opening by skipping fully acknowledged …
e6b2e90 
…ledgers (apache#24655)
coderzc added a commit that referenced this pull request Dec 7, 2025
@coderzc
[improve][ml] Optimize ledger opening by skipping fully acknowledged …
d392976 
…ledgers (#24655)

(cherry picked from commit da0d116)
lhotari added a commit that referenced this pull request Dec 15, 2025
@lhotari
[fix][branch-3.0] Fix cherry-picking issue
c369b05 
- in cherry-picking of #24655 in commit d392976
nodece pushed a commit to nodece/pulsar that referenced this pull request Dec 15, 2025
@coderzc @nodece
[improve][ml] Optimize ledger opening by skipping fully acknowledged …
c19f3fe 
…ledgers (apache#24655)

(cherry picked from commit da0d116)
nodece pushed a commit to nodece/pulsar that referenced this pull request Dec 15, 2025
@lhotari @nodece
[fix][branch-3.0] Fix cherry-picking issue
e093132 
- in cherry-picking of apache#24655 in commit d392976
manas-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 18, 2025
@coderzc @manas-ctds
[improve][ml] Optimize ledger opening by skipping fully acknowledged …
03d8b79 
…ledgers (apache#24655)

(cherry picked from commit da0d116)
(cherry picked from commit d392976)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 18, 2025
@coderzc @srinath-ctds
[improve][ml] Optimize ledger opening by skipping fully acknowledged …
fb173d6 
…ledgers (apache#24655)

(cherry picked from commit da0d116)
(cherry picked from commit d392976)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@codelipenghui codelipenghui codelipenghui approved these changes

@dao-jun dao-jun dao-jun approved these changes

Assignees

@coderzc coderzc

Labels

area/ML category/performance Performance issues fix or improvements cherry-picked/branch-3.0 cherry-picked/branch-4.0 doc-not-needed Your PR changes do not impact docs ready-to-test release/3.0.16 release/4.0.7 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages

Projects

None yet

Milestone

4.1.0

Development

Successfully merging this pull request may close these issues.

4 participants

@coderzc @codecov-commenter @codelipenghui @dao-jun