Skip to content

Conversation

@lhotari
Copy link
Member

@lhotari lhotari commented Mar 17, 2025

Motivation

The changes introduced in PR #23931 caused a regression in the entry count estimation logic for branch-3.0. While PR #24055 attempted to address this regression, it did not fully resolve the issues, leaving Pulsar's branch-3.0 in a broken state. A NullPointerException could occur when using ReadOnlyManagedLedgerImpl instances, as the currentLedger is null in these cases. ReadOnlyManagedLedgerImpl is used in branch-3.0 by PulsarSQL. In master branch, it's currently unused, but since the code exists, it makes sense to fix this bug in the master branch. This PR also contains other improvements.

A problem was also detected in using ManagedLedgerImpl.getNextValidPosition method to find the next valid position. This method is relatively costly and takes the current LAC into account which doesn't make much sense just for estimation purposes.

Modifications

  1. NPE protection: Added null checks for currentLedger to handle ReadOnlyManagedLedgerImpl instances properly
  2. Code reorganization: Extracted the estimation logic into a separate EntryCountEstimator class to improve testability and code clarity
  3. Parameter enhancements: Modified method signature to accept maxEntries and stop calculations when this limit is reached
  4. Return type change: Changed return type from long to int to match the actual usage in the codebase
  5. Documentation: Added clear comments about implementation details
  6. Tests: Added comprehensive unit tests in EntryCountEstimatorTest that validate various scenarios.

Documentation

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

@codecov-commenter
Copy link

codecov-commenter commented Mar 17, 2025

Codecov Report

Attention: Patch coverage is 96.15385% with 2 lines in your changes missing coverage. Please review.

Project coverage is 74.16%. Comparing base (bbc6224) to head (0b0c6a1).
Report is 1213 commits behind head on master.

Files with missing lines Patch % Lines
...e/bookkeeper/mledger/impl/EntryCountEstimator.java 96.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #24089      +/-   ##
============================================
+ Coverage     73.57%   74.16%   +0.59%     
+ Complexity    32624    32070     -554     
============================================
  Files          1877     1864      -13     
  Lines        139502   144334    +4832     
  Branches      15299    16463    +1164     
============================================
+ Hits         102638   107052    +4414     
+ Misses        28908    28824      -84     
- Partials       7956     8458     +502     
Flag Coverage Δ
inttests 26.63% <63.46%> (+2.04%) ⬆️
systests 23.12% <63.46%> (-1.21%) ⬇️
unittests 73.67% <96.15%> (+0.83%) ⬆️

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.94% <100.00%> (-0.36%) ⬇️
...keeper/mledger/impl/cache/RangeEntryCacheImpl.java 63.59% <ø> (+4.84%) ⬆️
...e/bookkeeper/mledger/impl/EntryCountEstimator.java 96.00% <96.00%> (ø)

... and 1064 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 marked this pull request as draft March 17, 2025 17:51
@lhotari
Copy link
Member Author

lhotari commented Mar 17, 2025

There seem to be other gaps in the estimation logic. I'll revisit this in this PR in further commits.

@lhotari lhotari force-pushed the lh-fix-issues-in-estimateEntryCountBySize branch from 214a2a2 to dd2d0b4 Compare March 19, 2025 13:51
@lhotari lhotari marked this pull request as ready for review March 19, 2025 13:51
@lhotari lhotari requested a review from poorbarcode March 19, 2025 14:00
@lhotari lhotari changed the title [fix][ml] Fix issues in estimateEntryCountBySize: iterating same positions until bytes limit is reached and a NPE [fix][ml] Fix issues in estimateEntryCountBySize Mar 19, 2025
@lhotari lhotari requested a review from heesung-sohn March 19, 2025 17:37
@lhotari lhotari merged commit a44b2cf into apache:master Mar 20, 2025
52 checks passed
lhotari added a commit that referenced this pull request Mar 20, 2025
lhotari added a commit that referenced this pull request Mar 20, 2025
lhotari added a commit that referenced this pull request Mar 20, 2025
bahetimansi pushed a commit to bahetimansi/pulsar that referenced this pull request Mar 23, 2025
(cherry picked from commit a44b2cf)
(cherry picked from commit 50754a6)
@lhotari
Copy link
Member Author

lhotari commented Mar 25, 2025

follow up in #24125, please review

nodece pushed a commit to nodece/pulsar that referenced this pull request Mar 27, 2025
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 27, 2025
(cherry picked from commit a44b2cf)
(cherry picked from commit 50754a6)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 28, 2025
(cherry picked from commit a44b2cf)
(cherry picked from commit 50754a6)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 28, 2025
(cherry picked from commit a44b2cf)
(cherry picked from commit 098d040)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 29, 2025
(cherry picked from commit a44b2cf)
(cherry picked from commit 098d040)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 31, 2025
(cherry picked from commit a44b2cf)
(cherry picked from commit 098d040)
walkinggo pushed a commit to walkinggo/pulsar that referenced this pull request Oct 8, 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.

5 participants