Repository navigation
[improve][ml] Open ledgers through the BookKeeper builder API - #26598
Merged
Merged
Conversation
Every ledger open in managed-ledger still went through the legacy BookKeeper.asyncOpenLedger / asyncOpenLedgerNoRecovery overloads, while ledger creation already uses newCreateLedgerOp(). The builder API is where BookKeeper adds per-ledger options (4.18.1 ships OpenBuilder.withKeepUpdateMetadata and withOrderingKey), so this moves the remaining opens to newOpenLedgerOp() without changing behavior. The three opens that passed keepUpdateMetadata=true (ManagedLedgerImpl init-time open and concurrent-modification recheck, ManagedCursorImpl cursor-ledger recovery) use withRecovery(true).withKeepUpdateMetadata(true), which maps onto the same LedgerOpenOp.initiateWithKeepUpdateMetadata() path. The no-recovery opens in ShadowManagedLedgerImpl and the offline-stats cursor read in ManagedLedgerFactoryImpl use withRecovery(false). The existing OpenCallback bodies are kept and fed from the returned future, casting the handle to LedgerHandle as the real client returns a ReadOnlyLedgerHandle. PulsarMockBookKeeper's open builder returned a plain ReadHandle, which cannot be cast to the LedgerHandle the managed ledger and cursor keep. PulsarMockReadHandle is now a read-only LedgerHandle sharing the writer's entries, like ReadOnlyLedgerHandle: close is a no-op, reads still go through the read interceptor, it honors the mock's empty-ledger countdown, and the legacy asyncReadEntries is shared with the write handle. The mock's open builder completes on the mock executor, as the legacy mock path and the real client do. ManagedCursorTest and ManagedLedgerFactoryShutdownTest intercept newOpenLedgerOp() instead of the legacy overloads.
1 task done
The managed ledger now opens its last ledger and the cursor ledgers through newOpenLedgerOp() when a topic is reloaded, so the open-builder stub in this test also records those opens. Clear the recorded set right before compacting, and expect the penultimate ledger to have been opened for its stats after the reload, as the comment already said.
lhotari
reviewed
Sep 15, 2026
lhotari
left a comment
Member
There was a problem hiding this comment.
Great improvement!
A few things below are worth fixing or at least discussing before merge — mostly about how the new .execute().whenComplete(...) pattern handles exceptions and thread-completion timing, which is where "API switch only" turns out not to be quite exact.
… writes on the mock read view Review follow-ups: - The builder-based opens bridge their future to the legacy OpenCallback with whenComplete and dropped the returned stage, so anything the callback threw (for example a RejectedExecutionException when the managed ledger executor is already shut down) was swallowed instead of being logged the way BookKeeper's executor did. All six sites now go through ManagedLedgerImpl.completeOpenCallback, which logs it. - PulsarMockBookKeeper.newOpenLedgerOp() used thenComposeAsync, which short-circuits on an already failed programmed-failure future and completed the open inline on the caller's thread. It now always completes on the mock executor, as the legacy mock open path did. - PulsarMockReadHandle rejects writes with IllegalOpException, like ReadOnlyLedgerHandle, instead of falling through to LedgerHandle's write path and the mock's null client internals.
Radiancebobo
pushed a commit
to Radiancebobo/pulsar
that referenced
this pull request
Oct 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Every ledger open in managed-ledger still went through the legacy
BookKeeper.asyncOpenLedger/asyncOpenLedgerNoRecoveryoverloads, while ledger creation already usesnewCreateLedgerOp(). The builder API is where BookKeeper adds per-ledger options: 4.18.1, now on master, shipsOpenBuilder.withKeepUpdateMetadata(apache/bookkeeper#4834) andwithOrderingKeyon the create and open builders (apache/bookkeeper#4881). Moving the remaining opens tonewOpenLedgerOp()is the prerequisite for using those options. This PR only switches the API and does not change behavior.Modifications
ManagedLedgerImpl(init-time open of the last ledger, concurrent-modification recheck) andManagedCursorImpl(cursor-ledger recovery):newOpenLedgerOp().withRecovery(true).withKeepUpdateMetadata(true), which maps onto the sameLedgerOpenOp.initiateWithKeepUpdateMetadata()path as the previousasyncOpenLedger(..., keepUpdateMetadata=true)call.ShadowManagedLedgerImpl(source ledger opens) andManagedLedgerFactoryImpl(offline topic stats cursor ledger):newOpenLedgerOp().withRecovery(false).OpenCallbackbodies are unchanged; the returned future is adapted withBKException.getExceptionCode, and the handle is cast toLedgerHandlesince the real client returns aReadOnlyLedgerHandle.testmocks:PulsarMockBookKeeper.newOpenLedgerOp()returned a plainReadHandle, which cannot be cast toLedgerHandle.PulsarMockReadHandleis now a read-onlyLedgerHandlesharing the writer's entries, likeReadOnlyLedgerHandle: close is a no-op, reads still go through the read interceptor, it honors the mock's empty-ledger countdown, and the legacyasyncReadEntriesis shared withPulsarMockLedgerHandle. The mock's open builder completes on the mock executor, as the legacy mock path and the real client do.ManagedCursorTestandManagedLedgerFactoryShutdownTestinterceptnewOpenLedgerOp()instead of the legacy overloads.Broker-side users of
asyncOpenLedger(schema storage, bucket snapshots, compaction) are unchanged; they do not run on a managed-ledger thread.Verifying this change
This change is already covered by existing tests, such as
ManagedCursorTest,ManagedLedgerTest,ManagedLedgerBkTest,ManagedLedgerFactoryChangeLedgerPathTest,ShadowManagedLedgerImplTest,ReadOnlyManagedLedgerImplTest,ManagedLedgerFactoryShutdownTest,BrokerServiceTestandBrokerEntryCacheTest.