Add OpenBuilder.withKeepUpdateMetadata and DeleteBuilder.withLoggerContext to the builder API#4834
Open
merlimat wants to merge 2 commits into
Open
Add OpenBuilder.withKeepUpdateMetadata and DeleteBuilder.withLoggerContext to the builder API#4834merlimat wants to merge 2 commits into
merlimat wants to merge 2 commits into
Conversation
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>
void-ptr974
approved these changes
Jul 11, 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.
Descriptions of the changes in this PR:
Follow-up to BP-69 (#4766), which added
CreateBuilder.withLoggerContext/OpenBuilder.withLoggerContextso 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:
BookKeeper.asyncOpenLedger(lId, digestType, passwd, cb, ctx, keepUpdateMetadata)can setkeepUpdateMetadata. Pulsar's recovery-open call sites (ManagedLedgerImplinitialize,ManagedCursorImplcursor recovery,BookkeeperSchemaStorage/CompactedTopicImpl/BookkeeperBucketSnapshotStorageopens) all passkeepUpdateMetadata=true, so they cannot migrate to the builder API and therefore cannot usewithLoggerContext.DeleteBuilderhad nowithLoggerContext, so deletes could not carry application context.Changes
OpenBuilder.withKeepUpdateMetadata(boolean)(first commit)thison the@Public/@Unstableinterface, keeping source compatibility for out-of-tree implementors (same approach aswithLoggerContext).OpenBuilderBasestores the flag;LedgerOpenOp.OpenBuilderImplcallsinitiateWithKeepUpdateMetadata()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.BookKeeperBuildersTestverify the ledger metadata listener is registered when opening with recovery and the flag set, and not registered without it.DeleteBuilder.withLoggerContext(Logger)(second commit)DeleteBuilderImplpasses the parent logger toLedgerDeleteOp, whose logger is now a per-op contextual logger (parent context via slog'sLoggerBuilder.ctx(Logger)plus the always-presentledgerIdattribute), likeLedgerOpenOp. The lombok@CustomLogstatic logger moved toDeleteBuilderImpl, its only remaining user (builder validation runs before an op exists).LoggerContextTestgains 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 theledgerId.Verified with
BookKeeperBuildersTest(31 tests) andLoggerContextTest(8 tests), andcheckstyle:check— all green.