Skip to content
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

Merged
merged 5 commits into from Apr 10, 2018

Conversation

ivankelly
Copy link
Contributor

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

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
@ivankelly
Copy link
Contributor Author

retest this please // BrokerBkEnsemblesTests.testSkipCorruptDataLedger


checkNotNull(ml.getName());
checkNotNull(ml.getExecutor());
ml.getExecutor().executeOrdered(ml.getName(), safeRun(() -> {
Copy link
Contributor

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()

Copy link
Contributor Author

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.

@ivankelly
Copy link
Contributor Author

retest this please // BasicEndToEndTest.testStatsLatencies

@merlimat merlimat added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Apr 9, 2018
@ivankelly
Copy link
Contributor Author

retest this please // ManagedCursorTest.cursorPersistenceAsyncMarkDeleteSameThread

@ivankelly
Copy link
Contributor Author

retest this please // ManagedCursorTest.cursorPersistenceAsyncMarkDeleteSameThread again (passes locally) & c++ test timeout

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@merlimat merlimat merged commit ab67386 into apache:master Apr 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants