Skip to content

Conversation

@lhotari
Copy link
Member

@lhotari lhotari commented Mar 25, 2025

Motivation

Following up on #24089 changes. The changes cause a regression which is visible in branch-3.0 where a test fails (org.apache.pulsar.compaction.CompactionTest#testDispatcherMaxReadSizeBytes). This test is different in newer branches. The regression is caused by the logic added in #24089 to use DEFAULT_ESTIMATED_ENTRY_SIZE + BOOKKEEPER_READ_OVERHEAD_PER_ENTRY as the default average entry size.
Another gap is that when all ledgers are empty, no average size would be used. That is also fixed in this PR

Modifications

  • when the ledger is completely empty, don't estimate the number of entries, simply return 1 as the estimated entry count
  • when all ledgers are empty beyond the read position, find the latest ledger with entries to calculate the average entry count to be used in the estimation
  • add test coverage to cover these cases

Documentation

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

@codecov-commenter
Copy link

codecov-commenter commented Mar 25, 2025

Codecov Report

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

Project coverage is 74.17%. Comparing base (bbc6224) to head (65f8a87).
Report is 990 commits behind head on master.

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

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #24125      +/-   ##
============================================
+ Coverage     73.57%   74.17%   +0.59%     
+ Complexity    32624    32457     -167     
============================================
  Files          1877     1864      -13     
  Lines        139502   144452    +4950     
  Branches      15299    16472    +1173     
============================================
+ Hits         102638   107142    +4504     
+ Misses        28908    28844      -64     
- Partials       7956     8466     +510     
Flag Coverage Δ
inttests 26.69% <46.15%> (+2.11%) ⬆️
systests 23.14% <46.15%> (-1.19%) ⬇️
unittests 73.68% <84.61%> (+0.83%) ⬆️

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

Files with missing lines Coverage Δ
...e/bookkeeper/mledger/impl/EntryCountEstimator.java 93.44% <84.61%> (ø)

... and 1062 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 228a98f into apache:master Mar 26, 2025
60 checks passed
lhotari added a commit that referenced this pull request Mar 26, 2025
lhotari added a commit that referenced this pull request Mar 26, 2025
lhotari added a commit that referenced this pull request Mar 26, 2025
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
…turn 1 instead (apache#24125)

(cherry picked from commit 228a98f)
(cherry picked from commit c6a9bee)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 28, 2025
…turn 1 instead (apache#24125)

(cherry picked from commit 228a98f)
(cherry picked from commit c6a9bee)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 29, 2025
…turn 1 instead (apache#24125)

(cherry picked from commit 228a98f)
(cherry picked from commit e2e0128)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 31, 2025
…turn 1 instead (apache#24125)

(cherry picked from commit 228a98f)
(cherry picked from commit e2e0128)
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.

4 participants