[feat][ml] Support Bookkeeper Batch Read API#25280
[feat][ml] Support Bookkeeper Batch Read API#25280dao-jun wants to merge 3 commits intoapache:masterfrom
Conversation
dao-jun
added
type/feature
github-actions
bot
added
doc-not-needed
codelipenghui
left a comment
codelipenghui
left a comment
There was a problem hiding this comment.
The EntryCacheDisabled.java should also adopt the new method?
And from the test perspective, this PR has 0 test coverage for the core batch read logic.
| .thenApply(remainingEntries -> { | ||
| // Combine received and remaining entries | ||
| List<LedgerEntry> combined = new ArrayList<>(receivedCount + remainingCount); | ||
| combined.addAll(receivedList); |
There was a problem hiding this comment.
The entry in the receivedList will get released in line:115?
| doc = "Enable batch read API when reading entries from bookkeeper. " | ||
| + "Batch read allows reading multiple entries in a single RPC call, " | ||
| + "reducing network overhead for sequential reads.") | ||
| private boolean managedLedgerBatchReadEnabled = true; |
There was a problem hiding this comment.
It should be false as the PR's description?
| } | ||
|
|
||
| // Close the original entries since we've collected them into receivedList | ||
| entries.close(); |
There was a problem hiding this comment.
receivedList stores the original LedgerEntry references, but entries.close() is called before the merged result is returned. Since closing LedgerEntriesImpl also closes the contained entries, the merged result may end up holding entries whose buffers have already been released. Please preserve independent ownership of the retained entries (for example by retaining/duplicating them), or defer closing the original container until the merged result is consumed.
3 participants
Motivation
In BP-62, Bookkeeper starting to support batch read API,
the PR is to support Bookkeeper batch reading on the Pulsar side.
Modifications
bookkeeperEnableBatchReadto control enable BK batch read or not, the default value is false.Verifying this change
(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:)
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: