This is an automated email from the ASF dual-hosted git repository.
merlimat pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
The following commit(s) were added to refs/heads/master by this push:
new 6a4842e023 Add OpenBuilder.withKeepUpdateMetadata and
DeleteBuilder.withLoggerContext to the builder API (#4834)
6a4842e023 is described below
commit 6a4842e023ccfbde28b189380870cf43162d89f0
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]>
---
.../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();
+ }
}
