Skip to content

Add OpenBuilder.withKeepUpdateMetadata and DeleteBuilder.withLoggerContext to the builder API#4834

Open
merlimat wants to merge 2 commits into
apache:masterfrom
merlimat:wt/loving-haslett-bee15f
Open

Add OpenBuilder.withKeepUpdateMetadata and DeleteBuilder.withLoggerContext to the builder API#4834
merlimat wants to merge 2 commits into
apache:masterfrom
merlimat:wt/loving-haslett-bee15f

Conversation

@merlimat

Copy link
Copy Markdown
Contributor

Descriptions of the changes in this PR:

Follow-up to BP-69 (#4766), which added CreateBuilder.withLoggerContext / OpenBuilder.withLoggerContext so applications can attach their slog logging context to created/opened ledgers.

Motivation

Two gaps block applications (specifically Apache Pulsar) from carrying application logging context on all ledger operations through the builder API:

  1. Only the classic callback API BookKeeper.asyncOpenLedger(lId, digestType, passwd, cb, ctx, keepUpdateMetadata) can set keepUpdateMetadata. Pulsar's recovery-open call sites (ManagedLedgerImpl initialize, ManagedCursorImpl cursor recovery, BookkeeperSchemaStorage / CompactedTopicImpl / BookkeeperBucketSnapshotStorage opens) all pass keepUpdateMetadata=true, so they cannot migrate to the builder API and therefore cannot use withLoggerContext.
  2. DeleteBuilder had no withLoggerContext, so deletes could not carry application context.

Changes

OpenBuilder.withKeepUpdateMetadata(boolean) (first commit)

  • New default method returning this on the @Public/@Unstable interface, keeping source compatibility for out-of-tree implementors (same approach as withLoggerContext).
  • OpenBuilderBase stores the flag; LedgerOpenOp.OpenBuilderImpl calls initiateWithKeepUpdateMetadata() when the flag is set on a recovery open. The flag only has an effect for recovery opens — non-recovery opens always register the metadata listener immediately — matching the classic API, which only exposes the flag on the recovery-open overload.
  • Tests in BookKeeperBuildersTest verify the ledger metadata listener is registered when opening with recovery and the flag set, and not registered without it.

DeleteBuilder.withLoggerContext(Logger) (second commit)

  • New default no-op method mirroring BP-69's CreateBuilder/OpenBuilder support.
  • DeleteBuilderImpl passes the parent logger to LedgerDeleteOp, whose logger is now a per-op contextual logger (parent context via slog's LoggerBuilder.ctx(Logger) plus the always-present ledgerId attribute), like LedgerOpenOp. The lombok @CustomLog static logger moved to DeleteBuilderImpl, its only remaining user (builder validation runs before an op exists).
  • One behavioral addition: delete failures are now logged at debug level through the contextual logger — the delete path previously had no per-op log statements, so the context would have had nothing to ride on. Failures keep being reported to the caller via the callback, and debug level keeps the log silent at default levels (double deletes are routine for some applications).
  • LoggerContextTest gains the DeleteBuilder chain and null-is-no-op tests, plus a deterministic check that a failed delete's debug event carries the parent logger's attributes and the ledgerId.

Verified with BookKeeperBuildersTest (31 tests) and LoggerContextTest (8 tests), and checkstyle:check — all green.

merlimat added 2 commits June 10, 2026 21:23
Today only the classic callback API
BookKeeper.asyncOpenLedger(lId, digestType, passwd, cb, ctx,
keepUpdateMetadata) can request that an opened ledger keeps its
metadata updated when the auto-recovery component modifies the
ensemble. The builder API had no way to set the flag, which blocks
applications whose recovery-open call sites pass
keepUpdateMetadata=true (e.g. Apache Pulsar) from migrating those
opens to the builder API and therefore from carrying slog logger
context via withLoggerContext (BP-69).

- New default method on the @Public/@unstable OpenBuilder interface
  returning this, to keep source compatibility for out-of-tree
  implementors (same approach as withLoggerContext).
- OpenBuilderBase stores the flag and LedgerOpenOp.OpenBuilderImpl
  calls initiateWithKeepUpdateMetadata() when it is set on a recovery
  open. The flag only has an effect for recovery opens: non-recovery
  opens always register the metadata listener immediately.
- Tests in BookKeeperBuildersTest verify the ledger metadata listener
  is registered when opening with recovery and the flag set, and is
  not registered without it.

Signed-off-by: Matteo Merli <mmerli@apache.org>
Mirror BP-69's CreateBuilder/OpenBuilder support on DeleteBuilder so
delete operations can carry the application's slog context attributes
(request / tenant / trace identifiers) too.

- New default no-op method on the @Public/@unstable DeleteBuilder
  interface to keep source compatibility for out-of-tree implementors.
- DeleteBuilderImpl stores the parent Logger and passes it to
  LedgerDeleteOp, whose logger is now a per-op contextual logger built
  with the parent's context (via slog's LoggerBuilder.ctx(Logger)) plus
  the always-present ledgerId attribute, like LedgerOpenOp. The lombok
  @CustomLog static logger moves to DeleteBuilderImpl, which is the
  only remaining user (builder validation runs before an op exists).
- Delete failures are now logged at debug level through the contextual
  logger; failures keep being reported to the caller via the callback,
  so this stays quiet at default log levels (double deletes are a
  normal occurrence for some applications).

Tests: LoggerContextTest gains the DeleteBuilder variants of the
compile/chain and null-is-no-op checks, plus a deterministic check
that a failed delete's debug event carries the parent logger's attrs
and the ledgerId (the LedgerDeleteOp logger is raised to debug via
log4j2 Configurator for the duration of the test, and the capturing
appender is now installed at Level.ALL so debug events reach it).

Signed-off-by: Matteo Merli <mmerli@apache.org>
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