[fix][ml] Fix asyncReadEntries might never complete if empty entries are read from BK #24515

BewareMyPower · 2025-07-15T14:58:06Z

Main Issue: #24497

Motivation

When the OpReadEntry#readEntriesComplete is triggered by RangeEntryCacheImpl#readFromStorage with an empty list of entries, NPE will happen here:

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

Line 95 in bc5b933

? cursor.getNextAvailablePosition(lastPosition) : lastPosition.getNext();

Then updateReadPosition and checkReadCompletion will be skipped, where the result is very similar to #24497 (comment)

The callback of asyncReadEntries will never complete
The entriesReadCount will be updated but the readPosition won't
pendingReadOps will not be reduced to 0

Unfortunately, from the existing analysis, the issue of 24497 might not be caused by this bug, but this NPE is still possible.

Modifications

Remove the confusing lastPosition argument in internalReadEntriesComplete. It's only set when all entries have been filtered out and it needs to reset the readPosition in ManagedLedgerImpl#internalReadFromLedger. It can be replaced by a simple combination of updateReadPosition and checkReadCompletion.

In internalReadEntriesComplete, just call checkReadCompletion when no entries are read to trigger another round of read.

In addition, catch exceptions when triggering user-provided callback and print logs with better information. Without this change, the exception will be caught by SingleThreadTask#safeRunTask and the log will be Error while running task: that does not have description for a specific cursor or read position.

Verifying this change

Make EntryCache configurable in tests add add testReadNoEntries to verify that the asyncReadEntries will eventually succeed even if one read operation returns an empty entry list.

Documentation

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

Matching PR in forked repository

PR in forked repository:

…are read from BK

dao-jun

LGTM

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

BewareMyPower · 2025-07-16T02:24:36Z

testScan failed after this change, mark it as drafted first

codecov-commenter · 2025-07-16T06:15:42Z

Codecov Report

❌ Patch coverage is 69.23077% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.31%. Comparing base (bbc6224) to head (5515e1a).
⚠️ Report is 1266 commits behind head on master.

Files with missing lines	Patch %	Lines
...rg/apache/bookkeeper/mledger/impl/OpReadEntry.java	62.26%	15 Missing and 5 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #24515      +/-   ##
============================================
+ Coverage     73.57%   74.31%   +0.74%     
+ Complexity    32624    32552      -72     
============================================
  Files          1877     1869       -8     
  Lines        139502   146089    +6587     
  Branches      15299    16760    +1461     
============================================
+ Hits         102638   108569    +5931     
+ Misses        28908    28900       -8     
- Partials       7956     8620     +664

Flag	Coverage Δ
inttests	`26.69% <23.07%> (+2.11%)`	⬆️
systests	`23.28% <35.38%> (-1.04%)`	⬇️
unittests	`73.81% <69.23%> (+0.97%)`	⬆️

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

Files with missing lines	Coverage Δ
...kkeeper/mledger/impl/ManagedLedgerFactoryImpl.java	`59.50% <100.00%> (-21.89%)`	⬇️
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java	`81.47% <100.00%> (+0.80%)`	⬆️
...keeper/mledger/impl/cache/RangeEntryCacheImpl.java	`63.86% <ø> (+5.11%)`	⬆️
...rg/apache/bookkeeper/mledger/impl/OpReadEntry.java	`74.81% <62.26%> (-9.50%)`	⬇️

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

BewareMyPower · 2025-07-16T06:28:36Z

@lhotari Could you take a look again?

lhotari

I forgot to submit these minor comments earlier.

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

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

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

…are read from BK (#24515) (cherry picked from commit eea7c13)

lhotari · 2025-07-23T17:42:03Z

@BewareMyPower The "EntryCacheCreator" solution added in this PR is obsolete. There's already a way to intercept reads in Pulsar tests with PulsarMockReadHandleInterceptor.

example usage:

pulsar/managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerTest.java

Lines 3148 to 3156 in 86b036b

    
           bkc.setReadHandleInterceptor(new PulsarMockReadHandleInterceptor() { 
        
               @Override 
        
               public CompletableFuture<LedgerEntries> interceptReadAsync(long ledgerId, long firstEntry, long lastEntry, 
        
                                                                          LedgerEntries entries) { 
        
                   return CompletableFuture.supplyAsync(() -> { 
        
                       return entries; 
        
                   }, CompletableFuture.delayedExecutor(3, TimeUnit.SECONDS)); 
        
               } 
        
           });

Since the changes to add the "EntryCacheCreator" collide with #24363 changes, I'd suggest that we remove the "EntryCacheCreator" and update the tests added in this PR to use PulsarMockReadHandleInterceptor. I can do a PR to handle that.

lhotari · 2025-07-23T17:52:05Z

the replacement using ReadHandleInterceptor is

    bkc.setReadHandleInterceptor(new PulsarMockReadHandleInterceptor() {
            @Override
            public CompletableFuture<LedgerEntries> interceptReadAsync(long ledgerId, long firstEntry, long lastEntry,
                                                                       LedgerEntries entries) {
                if (firstRead.compareAndSet(true, false)) {
                    // LedgerEntriesImpl doesn't allow empty entries list.
                    // Implementing a dummy LedgerEntries that returns an empty list.
                    return CompletableFuture.completedFuture(new LedgerEntries() {
                        @Override
                        public org.apache.bookkeeper.client.api.LedgerEntry getEntry(long entryId) {
                            return null;
                        }

                        @Override
                        public Iterator<org.apache.bookkeeper.client.api.LedgerEntry> iterator() {
                            return EmptyIterator.INSTANCE;
                        }

                        @Override
                        public void close() {

                        }
                    });
                } else {
                    return CompletableFuture.completedFuture(entries);
                }
            }
        });

BewareMyPower · 2025-07-23T17:58:41Z

Let me try this approach tomorrow

lhotari · 2025-07-23T18:07:03Z

Let me try this approach tomorrow

I already submitted the PR in #24552 since I'll use that to be able to rebase and test #24363.

…are read from BK (apache#24515) (cherry picked from commit eea7c13) (cherry picked from commit c887259)

…are read from BK (apache#24515) (cherry picked from commit eea7c13) (cherry picked from commit f1974aa)

…are read from BK (apache#24515) (cherry picked from commit eea7c13) (cherry picked from commit c887259)

…are read from BK (apache#24515) (cherry picked from commit eea7c13) (cherry picked from commit f1974aa)

…are read from BK (apache#24515) (cherry picked from commit eea7c13)

…are read from BK (apache#24515)

[fix][ml] Fix asyncReadEntries might never complete if empty entries …

e5289ca

…are read from BK

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

BewareMyPower requested review from Demogorgon314, RobertIndie, Technoboy-, codelipenghui, lhotari, nodece and poorbarcode July 15, 2025 14:58

BewareMyPower self-assigned this Jul 15, 2025

BewareMyPower added release/3.0.13 release/4.0.6 release/3.3.8 labels Jul 15, 2025

dao-jun approved these changes Jul 15, 2025

View reviewed changes

lhotari reviewed Jul 15, 2025

View reviewed changes

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

Code style change

780a0d2

BewareMyPower marked this pull request as draft July 16, 2025 02:24

BewareMyPower added type/bug The PR fixed a bug or issue reported a bug area/ML labels Jul 16, 2025

Fix testScan

4d4dbda

BewareMyPower marked this pull request as ready for review July 16, 2025 02:58

BewareMyPower added the ready-to-test label Jul 16, 2025

lhotari reviewed Jul 16, 2025

View reviewed changes

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

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

Move recycle() back to try block

2d00fb0

poorbarcode reviewed Jul 17, 2025

View reviewed changes

BewareMyPower added 2 commits July 17, 2025 21:02

Improve logs

eed4ff1

Simplify code

d62290c

poorbarcode approved these changes Jul 18, 2025

View reviewed changes

Merge branch 'master' into bewaremypower/ml-read-stuck

5515e1a

BewareMyPower merged commit eea7c13 into apache:master Jul 19, 2025
185 of 192 checks passed

BewareMyPower deleted the bewaremypower/ml-read-stuck branch July 19, 2025 04:26

lhotari pushed a commit that referenced this pull request Jul 22, 2025

[fix][ml] Fix asyncReadEntries might never complete if empty entries …

f1974aa

…are read from BK (#24515) (cherry picked from commit eea7c13)

lhotari added the cherry-picked/branch-4.0 label Jul 22, 2025

lhotari pushed a commit that referenced this pull request Jul 23, 2025

[fix][ml] Fix asyncReadEntries might never complete if empty entries …

c887259

…are read from BK (#24515) (cherry picked from commit eea7c13)

lhotari added the cherry-picked/branch-3.0 label Jul 23, 2025

lhotari pushed a commit that referenced this pull request Jul 23, 2025

[fix][ml] Fix asyncReadEntries might never complete if empty entries …

d283d39

…are read from BK (#24515) (cherry picked from commit eea7c13)

lhotari added the cherry-picked/branch-3.3 label Jul 23, 2025

lhotari mentioned this pull request Jul 23, 2025

[improve][test] Remove EntryCacheCreator from ManagedLedgerFactoryImpl #24552

Merged

4 tasks

priyanshu-ctds pushed a commit to datastax/pulsar that referenced this pull request Jul 24, 2025

[fix][ml] Fix asyncReadEntries might never complete if empty entries …

7edeff7

…are read from BK (apache#24515) (cherry picked from commit eea7c13) (cherry picked from commit c887259)

priyanshu-ctds pushed a commit to datastax/pulsar that referenced this pull request Jul 24, 2025

[fix][ml] Fix asyncReadEntries might never complete if empty entries …

c2ffd4e

…are read from BK (apache#24515) (cherry picked from commit eea7c13) (cherry picked from commit c887259)

priyanshu-ctds pushed a commit to datastax/pulsar that referenced this pull request Jul 24, 2025

[fix][ml] Fix asyncReadEntries might never complete if empty entries …

1b81005

…are read from BK (apache#24515) (cherry picked from commit eea7c13) (cherry picked from commit f1974aa)

srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jul 24, 2025

[fix][ml] Fix asyncReadEntries might never complete if empty entries …

053d541

…are read from BK (apache#24515) (cherry picked from commit eea7c13) (cherry picked from commit c887259)

srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jul 24, 2025

[fix][ml] Fix asyncReadEntries might never complete if empty entries …

7c59568

…are read from BK (apache#24515) (cherry picked from commit eea7c13) (cherry picked from commit f1974aa)

nodece pushed a commit to ascentstream/pulsar that referenced this pull request Jul 28, 2025

[fix][ml] Fix asyncReadEntries might never complete if empty entries …

7c37b75

…are read from BK (apache#24515) (cherry picked from commit eea7c13)

nodece pushed a commit to ascentstream/pulsar that referenced this pull request Jul 28, 2025

[fix][ml] Fix asyncReadEntries might never complete if empty entries …

75fa9ad

…are read from BK (apache#24515) (cherry picked from commit eea7c13)

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

[fix][ml] Fix asyncReadEntries might never complete if empty entries …

fcf8792

…are read from BK (apache#24515)

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

[fix][ml] Fix asyncReadEntries might never complete if empty entries …

a7db4a8

…are read from BK (apache#24515)

[fix][ml] Fix asyncReadEntries might never complete if empty entries are read from BK #24515

[fix][ml] Fix asyncReadEntries might never complete if empty entries are read from BK #24515

Uh oh!

Conversation

BewareMyPower commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

Documentation

Matching PR in forked repository

Uh oh!

dao-jun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BewareMyPower commented Jul 16, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

BewareMyPower commented Jul 16, 2025

Uh oh!

lhotari left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lhotari commented Jul 23, 2025

Uh oh!

lhotari commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BewareMyPower commented Jul 23, 2025

Uh oh!

lhotari commented Jul 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

BewareMyPower commented Jul 15, 2025 •

edited

Loading

codecov-commenter commented Jul 16, 2025 •

edited

Loading

lhotari commented Jul 23, 2025 •

edited

Loading