Skip to content

[improve][ml] Open ledgers through the BookKeeper builder API - #26598

Merged
merlimat merged 3 commits into
apache:masterfrom
merlimat:mmerli/ml-open-ledger-builder
Sep 15, 2026
Merged

merlimat merged 3 commits into
apache:masterfrom
merlimat:mmerli/ml-open-ledger-builder

Conversation

@merlimat

Copy link
Copy Markdown
Contributor

Motivation

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, now on master, ships OpenBuilder.withKeepUpdateMetadata (apache/bookkeeper#4834) and withOrderingKey on the create and open builders (apache/bookkeeper#4881). Moving the remaining opens to newOpenLedgerOp() 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) and ManagedCursorImpl (cursor-ledger recovery): newOpenLedgerOp().withRecovery(true).withKeepUpdateMetadata(true), which maps onto the same LedgerOpenOp.initiateWithKeepUpdateMetadata() path as the previous asyncOpenLedger(..., keepUpdateMetadata=true) call.
  • ShadowManagedLedgerImpl (source ledger opens) and ManagedLedgerFactoryImpl (offline topic stats cursor ledger): newOpenLedgerOp().withRecovery(false).
  • The existing OpenCallback bodies are unchanged; the returned future is adapted with BKException.getExceptionCode, and the handle is cast to LedgerHandle since the real client returns a ReadOnlyLedgerHandle.
  • testmocks: PulsarMockBookKeeper.newOpenLedgerOp() returned a plain ReadHandle, which cannot be cast to LedgerHandle. 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 PulsarMockLedgerHandle. 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.

Broker-side users of asyncOpenLedger (schema storage, bucket snapshots, compaction) are unchanged; they do not run on a managed-ledger thread.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as ManagedCursorTest, ManagedLedgerTest, ManagedLedgerBkTest, ManagedLedgerFactoryChangeLedgerPathTest, ShadowManagedLedgerImplTest, ReadOnlyManagedLedgerImplTest, ManagedLedgerFactoryShutdownTest, BrokerServiceTest and BrokerEntryCacheTest.

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.
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 lhotari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread testmocks/src/main/java/org/apache/bookkeeper/client/PulsarMockBookKeeper.java Outdated
lhotari

This comment was marked as resolved.

… 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.

@lhotari lhotari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@lhotari lhotari added this to the 5.0.0 milestone Sep 15, 2026
@merlimat
merlimat merged commit 504c48c into apache:master Sep 15, 2026
81 of 83 checks passed
dao-jun pushed a commit to ascentstream/pulsar that referenced this pull request Sep 20, 2026
dao-jun pushed a commit to ascentstream/pulsar that referenced this pull request Sep 20, 2026
dao-jun pushed a commit to ascentstream/pulsar that referenced this pull request Sep 20, 2026
Radiancebobo pushed a commit to Radiancebobo/pulsar that referenced this pull request Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants