[feat][ml] Add readEntries method to ML #24961
Conversation
# Conflicts: # managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerTest.java
| /** | ||
| * Async read entries. | ||
| * Notice: You MUST have a Durable cursor outside of the managed ledger, to protect the ledgers from being deleted. | ||
| * | ||
| * @param start | ||
| * @param numberOfEntriesToRead | ||
| * @param callback | ||
| * @param ctx | ||
| */ | ||
| void asyncReadEntries(Position start, long numberOfEntriesToRead, AsyncCallbacks.ReadEntriesCallback callback, | ||
| Object ctx); |
There was a problem hiding this comment.
No need to add a callback based API
| /** | ||
| * Async read entries. | ||
| * Notice: You MUST have a Durable cursor outside of the managed ledger, to protect the ledgers from being deleted. | ||
| * | ||
| * @param start | ||
| * @param numberOfEntriesToRead | ||
| */ |
There was a problem hiding this comment.
No document is better than documents that are just a template.
Regarding the ledger deletion, it should not be documented here. There is already a document for this: https://pulsar.apache.org/docs/next/cookbooks-retention-expiry/
| * @param start | ||
| * @param numberOfEntriesToRead | ||
| */ | ||
| CompletableFuture<List<Entry>> asyncReadEntries(Position start, long numberOfEntriesToRead); |
There was a problem hiding this comment.
numberOfEntriesToRead should be int.
Additionally, this method does not have maxSizeBytes limit like ManagedCursor#asyncReadEntries
BewareMyPower
left a comment
BewareMyPower
left a comment
There was a problem hiding this comment.
Please write a PIP since this proposal adds new methods to the ManagedLedger interface
| } | ||
|
|
||
| int actualReadEntries = getNumberOfEntries(readerPositions); | ||
| ReadEntriesCallback callback0 = new InternalReadEntriesCallback(actualReadEntries, callback); |
There was a problem hiding this comment.
Such callback is complicated, a better solution is to gather all futures of asyncReadEntry and wait for all of them
| * @return | ||
| */ | ||
| @VisibleForTesting | ||
| public Map<Long, Pair<Long, Long>> getReaderPositions(Position start, long numberOfEntries) { |
There was a problem hiding this comment.
If this method is exposed as a public method, is there still a reason to add asyncReadEntries?
2 participants
Motivation
For protocolHandlers, such as kop.
Modifications
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-complete