This is an automated email from the ASF dual-hosted git repository. merlimat pushed a commit to branch branch-4.18 in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
commit c60dfe1381d025109605ee71ac0aefc4d014a6e5 Author: Matteo Merli <[email protected]> AuthorDate: Tue Jul 14 08:25:56 2026 -0700 Add OpenBuilder.withKeepUpdateMetadata and DeleteBuilder.withLoggerContext to the builder API (#4834) * Add OpenBuilder.withKeepUpdateMetadata to the builder API 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 <[email protected]> * Add DeleteBuilder.withLoggerContext for slog logger context on deletes 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 <[email protected]> --------- Signed-off-by: Matteo Merli <[email protected]> (cherry picked from commit 6a4842e023ccfbde28b189380870cf43162d89f0) --- .../apache/bookkeeper/client/LedgerDeleteOp.java | 27 ++++++++- .../org/apache/bookkeeper/client/LedgerOpenOp.java | 6 +- .../bookkeeper/client/api/DeleteBuilder.java | 18 ++++++ .../apache/bookkeeper/client/api/OpenBuilder.java | 16 ++++++ .../bookkeeper/client/impl/OpenBuilderBase.java | 7 +++ .../client/api/BookKeeperBuildersTest.java | 43 +++++++++++++++ .../bookkeeper/client/api/LoggerContextTest.java | 64 +++++++++++++++++++++- 7 files changed, 175 insertions(+), 6 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerDeleteOp.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerDeleteOp.java index 7399d4064e..5fd2c73f27 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerDeleteOp.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerDeleteOp.java @@ -21,6 +21,8 @@ package org.apache.bookkeeper.client; +import io.github.merlimat.slog.Logger; +import io.github.merlimat.slog.LoggerBuilder; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.locks.ReentrantReadWriteLock; @@ -36,9 +38,10 @@ import org.apache.bookkeeper.versioning.Version; * Encapsulates asynchronous ledger delete operation. * */ -@CustomLog class LedgerDeleteOp { + private final Logger log; + final BookKeeper bk; final long ledgerId; final DeleteCallback cb; @@ -60,6 +63,16 @@ class LedgerDeleteOp { */ LedgerDeleteOp(BookKeeper bk, BookKeeperClientStats clientStats, long ledgerId, DeleteCallback cb, Object ctx) { + this(bk, clientStats, ledgerId, cb, ctx, null); + } + + LedgerDeleteOp(BookKeeper bk, BookKeeperClientStats clientStats, + long ledgerId, DeleteCallback cb, Object ctx, Logger parentLogger) { + LoggerBuilder builder = Logger.get(LedgerDeleteOp.class).with(); + if (parentLogger != null) { + builder = builder.ctx(parentLogger); + } + this.log = builder.attr("ledgerId", ledgerId).build(); this.bk = bk; this.ledgerId = ledgerId; this.cb = cb; @@ -77,6 +90,7 @@ class LedgerDeleteOp { bk.getLedgerManager().removeLedgerMetadata(ledgerId, Version.ANY) .whenCompleteAsync((ignore, exception) -> { if (exception != null) { + log.debug().exception(exception).log("Failed to delete ledger"); deleteOpLogger.registerFailedEvent(MathUtils.elapsedNanos(startTime), TimeUnit.NANOSECONDS); } else { deleteOpLogger.registerSuccessfulEvent(MathUtils.elapsedNanos(startTime), TimeUnit.NANOSECONDS); @@ -90,9 +104,11 @@ class LedgerDeleteOp { return String.format("LedgerDeleteOp(%d)", ledgerId); } + @CustomLog static class DeleteBuilderImpl implements DeleteBuilder { private Long builderLedgerId; + private Logger builderParentLogger; private final BookKeeper bk; DeleteBuilderImpl(BookKeeper bk) { @@ -105,6 +121,12 @@ class LedgerDeleteOp { return this; } + @Override + public DeleteBuilder withLoggerContext(Logger parentLogger) { + this.builderParentLogger = parentLogger; + return this; + } + @Override public CompletableFuture<Void> execute() { CompletableFuture<Void> future = new CompletableFuture<>(); @@ -126,7 +148,8 @@ class LedgerDeleteOp { cb.deleteComplete(BKException.Code.IncorrectParameterException, null); return; } - LedgerDeleteOp op = new LedgerDeleteOp(bk, bk.getClientCtx().getClientStats(), ledgerId, cb, null); + LedgerDeleteOp op = new LedgerDeleteOp(bk, bk.getClientCtx().getClientStats(), ledgerId, cb, null, + builderParentLogger); ReentrantReadWriteLock closeLock = bk.getCloseLock(); closeLock.readLock().lock(); try { diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java index 56667ee096..4bae409ec7 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java @@ -358,7 +358,11 @@ class LedgerOpenOp { return; } if (recovery) { - op.initiate(); + if (keepUpdateMetadata) { + op.initiateWithKeepUpdateMetadata(); + } else { + op.initiate(); + } } else { op.initiateWithoutRecovery(); } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/DeleteBuilder.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/DeleteBuilder.java index d88126a7bc..9ad06d91dd 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/DeleteBuilder.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/DeleteBuilder.java @@ -20,6 +20,7 @@ */ package org.apache.bookkeeper.client.api; +import io.github.merlimat.slog.Logger; import org.apache.bookkeeper.common.annotation.InterfaceAudience.Public; import org.apache.bookkeeper.common.annotation.InterfaceStability.Unstable; @@ -41,4 +42,21 @@ public interface DeleteBuilder extends OpBuilder<Void> { */ DeleteBuilder withLedgerId(long ledgerId); + /** + * Inherit the context attributes of the given slog {@link Logger} on the logger used by the delete operation. + * Every log statement emitted while executing the delete will carry the parent logger's context attributes, + * in addition to the {@code ledgerId} attribute that is always added by the client. + * + * <p>Useful for correlating bookkeeper-client log output with the application's own request / tenant / trace + * identifiers — typically the application has built a per-request logger via + * {@code Logger.get(...).with().attr(...)...build()} and passes it here. + * + * @param parentLogger logger whose context attributes to inherit; {@code null} is treated as no extra context + * + * @return the builder itself + */ + default DeleteBuilder withLoggerContext(Logger parentLogger) { + return this; + } + } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/OpenBuilder.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/OpenBuilder.java index 88b3fc863e..b0951b9972 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/OpenBuilder.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/OpenBuilder.java @@ -73,6 +73,22 @@ public interface OpenBuilder extends OpBuilder<ReadHandle> { */ OpenBuilder withDigestType(DigestType digestType); + /** + * Whether to keep updating the metadata of the resulting {@link ReadHandle} if the auto-recovery component + * modifies the ledger's ensemble (e.g. when it re-replicates the ledger's data after a bookie becomes + * unavailable). It defaults to 'false'. + * + * <p>This setting only has an effect when the ledger is opened in recovery mode (see + * {@link #withRecovery(boolean)}); a ledger opened in readonly mode always keeps its metadata updated. + * + * @param keepUpdateMetadata whether to keep the ledger metadata updated + * + * @return the builder itself + */ + default OpenBuilder withKeepUpdateMetadata(boolean keepUpdateMetadata) { + return this; + } + /** * Inherit the context attributes of the given slog {@link Logger} on the logger bound to * the resulting {@link ReadHandle}. Every log statement emitted by the handle (and by the open-time machinery diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/impl/OpenBuilderBase.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/impl/OpenBuilderBase.java index 99c997c180..45afcce91e 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/impl/OpenBuilderBase.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/impl/OpenBuilderBase.java @@ -36,6 +36,7 @@ public abstract class OpenBuilderBase implements OpenBuilder { protected long ledgerId = LedgerHandle.INVALID_LEDGER_ID; protected byte[] password; protected DigestType digestType = DigestType.CRC32; + protected boolean keepUpdateMetadata = false; protected Logger parentLogger; @Override @@ -62,6 +63,12 @@ public abstract class OpenBuilderBase implements OpenBuilder { return this; } + @Override + public OpenBuilder withKeepUpdateMetadata(boolean keepUpdateMetadata) { + this.keepUpdateMetadata = keepUpdateMetadata; + return this; + } + @Override public OpenBuilder withLoggerContext(Logger parentLogger) { this.parentLogger = parentLogger; diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/api/BookKeeperBuildersTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/api/BookKeeperBuildersTest.java index c039c1f77d..5a757bfd6c 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/api/BookKeeperBuildersTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/api/BookKeeperBuildersTest.java @@ -26,6 +26,11 @@ import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.fail; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import java.util.EnumSet; import java.util.HashMap; @@ -367,6 +372,35 @@ public class BookKeeperBuildersTest extends MockBookKeeperTestCase { }); } + @Test + public void testOpenLedgerWithRecoveryAndKeepUpdateMetadata() throws Exception { + registerMockLedgerMetadata(ledgerId, generateClosedLedgerMetadata()); + + ReadHandle reader = newOpenLedgerOp() + .withPassword(password) + .withLedgerId(ledgerId) + .withRecovery(true) + .withKeepUpdateMetadata(true) + .execute() + .get(); + assertEquals(ledgerId, reader.getId()); + verify(ledgerManager).registerLedgerMetadataListener(eq(ledgerId), any()); + } + + @Test + public void testOpenLedgerWithRecoveryNoKeepUpdateMetadata() throws Exception { + registerMockLedgerMetadata(ledgerId, generateClosedLedgerMetadata()); + + ReadHandle reader = newOpenLedgerOp() + .withPassword(password) + .withLedgerId(ledgerId) + .withRecovery(true) + .execute() + .get(); + assertEquals(ledgerId, reader.getId()); + verify(ledgerManager, never()).registerLedgerMetadataListener(anyLong(), any()); + } + @Test public void testDeleteLedgerNoLedgerId() throws Exception { assertThrows(BKIncorrectParameterException.class, () -> { @@ -420,6 +454,15 @@ public class BookKeeperBuildersTest extends MockBookKeeperTestCase { .newEnsembleEntry(0, generateNewEnsemble(ensembleSize)).build(); } + private LedgerMetadata generateClosedLedgerMetadata() throws BKException.BKNotEnoughBookiesException { + return LedgerMetadataBuilder.from(generateLedgerMetadata(ensembleSize, + writeQuorumSize, ackQuorumSize, password, customMetadata)) + .withClosedState() + .withLastEntryId(-1) + .withLength(0) + .build(); + } + @Test public void testCreateLedgerWithOpportunisticStriping() throws Exception { diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/api/LoggerContextTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/api/LoggerContextTest.java index fa3a06a95e..e0aeac4c11 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/api/LoggerContextTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/api/LoggerContextTest.java @@ -29,11 +29,13 @@ import java.util.Map; import java.util.Optional; import java.util.concurrent.atomic.AtomicReference; import org.apache.bookkeeper.client.MockBookKeeperTestCase; +import org.apache.logging.log4j.Level; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.core.LogEvent; import org.apache.logging.log4j.core.LoggerContext; import org.apache.logging.log4j.core.appender.AbstractAppender; import org.apache.logging.log4j.core.config.Configuration; +import org.apache.logging.log4j.core.config.Configurator; import org.apache.logging.log4j.core.config.LoggerConfig; import org.apache.logging.log4j.core.config.Property; import org.apache.logging.log4j.util.ReadOnlyStringMap; @@ -41,8 +43,9 @@ import org.junit.jupiter.api.Test; /** * Verifies that the slog {@link Logger} passed to {@link CreateBuilder#withLoggerContext} / - * {@link OpenBuilder#withLoggerContext} contributes its context attributes to every log event - * emitted by the resulting handle (and by the create/open machinery that produces it). + * {@link OpenBuilder#withLoggerContext} / {@link DeleteBuilder#withLoggerContext} contributes its + * context attributes to every log event emitted by the resulting handle (and by the + * create/open/delete machinery that produces it). */ public class LoggerContextTest extends MockBookKeeperTestCase { @@ -84,7 +87,7 @@ public class LoggerContextTest extends MockBookKeeperTestCase { appender.start(); config.addAppender(appender); LoggerConfig root = config.getRootLogger(); - root.addAppender(appender, root.getLevel(), null); + root.addAppender(appender, Level.ALL, null); ctx.updateLoggers(); return appender; } @@ -139,6 +142,37 @@ public class LoggerContextTest extends MockBookKeeperTestCase { // compilation of this file plus the other test methods. } + @Test + public void deleteBuilder_withLoggerContext_propagatesIntoLogEvents() throws Exception { + // Delete a ledger that does not exist. LedgerDeleteOp logs the failure at debug + // level via its contextual logger, which should carry both the parent logger's + // attrs and the always-present ledgerId. + Logger requestLog = requestLogger("req-del", "acme"); + + Configurator.setLevel("org.apache.bookkeeper.client.LedgerDeleteOp", Level.DEBUG); + CapturingAppender appender = installAppender("LedgerDeleteOp"); + try { + try { + newDeleteLedgerOp() + .withLedgerId(ledgerId) + .withLoggerContext(requestLog) + .execute() + .get(); + } catch (Exception ignored) { + // expected — ledger doesn't exist in the mock + } + } finally { + uninstallAppender(appender); + Configurator.setLevel("org.apache.bookkeeper.client.LedgerDeleteOp", Level.INFO); + } + + Map<String, String> ctxData = appender.firstMatch().orElseThrow( + () -> new AssertionError("expected to capture a LogEvent from LedgerDeleteOp")); + assertEquals("req-del", ctxData.get("requestId")); + assertEquals("acme", ctxData.get("tenant")); + assertEquals(String.valueOf(ledgerId), ctxData.get("ledgerId")); + } + @Test public void openBuilder_withLoggerContext_compilesAndChains() throws Exception { OpenBuilder ob = newOpenLedgerOp() @@ -155,6 +189,13 @@ public class LoggerContextTest extends MockBookKeeperTestCase { assertTrue(cb2 instanceof CreateBuilder); } + @Test + public void deleteBuilder_withLoggerContext_compilesAndChains() throws Exception { + DeleteBuilder db = newDeleteLedgerOp().withLedgerId(ledgerId); + DeleteBuilder db2 = db.withLoggerContext(requestLogger("req-1", "acme")); + assertTrue(db2 instanceof DeleteBuilder); + } + @Test public void parentLogger_attrsAreInheritedAndLedgerIdAdded() throws Exception { // Direct unit test: the slog LoggerBuilder.ctx(Logger) hook used by the @@ -192,4 +233,21 @@ public class LoggerContextTest extends MockBookKeeperTestCase { .get(); assertEquals(ledgerId, writer.getId()); } + + @Test + public void deleteBuilder_withLoggerContext_nullIsTreatedAsNoExtraContext() throws Exception { + setNewGeneratedLedgerId(ledgerId); + WriteHandle writer = newCreateLedgerOp() + .withEnsembleSize(3).withWriteQuorumSize(2).withAckQuorumSize(1) + .withPassword(password) + .execute() + .get(); + assertEquals(ledgerId, writer.getId()); + + newDeleteLedgerOp() + .withLedgerId(ledgerId) + .withLoggerContext(null) + .execute() + .get(); + } }
