-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Managed ledger uses ReadHandle in read path #1513
Conversation
BookKeeper 4.6 introduced a new API for reading, called ReadHandle. This API is an small interface, unlike LedgerHandle, so it is possible to provide implementations of ReadHandle that read from sources other than bookkeeper, while presenting the same interface as when reading from bookkeeper. This patch changes the managed ledger entry read path to use ReadHandle. This is rquired to implement tiered storage (PIP-17). Master issue: 1511
retest this please // BrokerBkEnsemblesTests.testSkipCorruptDataLedger |
|
||
checkNotNull(ml.getName()); | ||
checkNotNull(ml.getExecutor()); | ||
ml.getExecutor().executeOrdered(ml.getName(), safeRun(() -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you did in some other part of code, even here we could use whenCompleteAsync()
with the ml.executor().chooseThread(ml.getName()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't occur to me when I was first doing it, but done now.
To avoid creating yet another runnable.
And another merge conflict.
retest this please // BasicEndToEndTest.testStatsLatencies |
retest this please // ManagedCursorTest.cursorPersistenceAsyncMarkDeleteSameThread |
retest this please // ManagedCursorTest.cursorPersistenceAsyncMarkDeleteSameThread again (passes locally) & c++ test timeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
BookKeeper 4.6 introduced a new API for reading, called
ReadHandle. This API is an small interface, unlike LedgerHandle, so it
is possible to provide implementations of ReadHandle that read from
sources other than bookkeeper, while presenting the same interface as
when reading from bookkeeper.
This patch changes the managed ledger entry read path to use
ReadHandle. This is rquired to implement tiered storage (PIP-17).
Master issue: #1511