Skip to content

BP-69: Convert bookkeeper-common to slog (phase 1)#4754

Open
merlimat wants to merge 8 commits intoapache:masterfrom
merlimat:bp-69-slog-common
Open

BP-69: Convert bookkeeper-common to slog (phase 1)#4754
merlimat wants to merge 8 commits intoapache:masterfrom
merlimat:bp-69-slog-common

Conversation

@merlimat
Copy link
Copy Markdown
Contributor

First phase of the slog migration BP-69.
Tracking issue: #4750.

This PR adds the slog dependency and @CustomLog wiring, and converts the bookkeeper-common module. Subsequent PRs will convert the rest of the codebase module by module (stats + allocator, bookkeeper-server, bookkeeper-http + tools, stream/distributedlog, peripheral modules, and a final cleanup).

Summary

  • Add io.github.merlimat.slog:slog:0.9.7 to pom.xml (alongside the existing SLF4J API — SLF4J stays on the classpath as the rendering backend; no log-config change required to upgrade).
  • Add lombok.config at the repo root so @CustomLog generates a slog Logger instead of an SLF4J one:
    lombok.log.custom.declaration = io.github.merlimat.slog.Logger \
        io.github.merlimat.slog.Logger.get(TYPE)
    
  • Convert 6 files in bookkeeper-common:
    • AbstractLifecycleComponent, AutoCloseableLifecycleComponent
    • LogExceptionRunnable, OrderedExecutor, SafeRunnable
    • TestOrderedExecutorDecorators (test class)

Conventions (to be applied throughout the migration)

  • @Slf4j@CustomLog; LoggerFactory.getLogger(X.class)Logger.get(X.class); naming: static = LOG, instance-bound = log.
  • log.info("text {} {}", a, b) becomes log.info().attr("a", a).attr("b", b).log("text") — SLF4J {} params turn into named structured attrs, not printf-style format specifiers.
  • Classes with identity (ledgerId, bookieId, …) will (in later PRs) build instance-bound loggers in the constructor so every event carries the identity attributes without repeating them at each call site.
  • if (log.isDebugEnabled()) guards are removed; slog returns a no-op Event when the level is disabled.
  • Expensive attr values use slog's lambda form (log.debug(e -> ...) or .attr(k, () -> expensive)) so the work is only performed when the level is actually enabled.

Not included in this PR

  • No module outside bookkeeper-common is converted.
  • No wire-protocol, binary-format, metadata-format, metric, or CLI change.
  • Client API changes (withLoggerContext(Map<String, Object>) on CreateBuilder/OpenBuilder/DeleteBuilder) are deferred to a later PR in the BP-69 series.

Test plan

  • mvn -pl bookkeeper-common -am compile -DskipTests passes
  • Existing bookkeeper-common unit tests still pass
  • Rendered log output on a local run unchanged (slog's SLF4J handler forwards structured attrs as MDC, so existing Logback/Log4j2 configs keep working)

First phase of the slog migration for BP-69. Adds the slog dependency,
Lombok @CustomLog configuration, and converts the bookkeeper-common module.
@merlimat merlimat requested review from hangc0276, lhotari and zymap April 23, 2026 22:16
@merlimat merlimat added this to the 4.18.0 milestone Apr 23, 2026
CI flagged 4 ImportOrder violations in the Phase 1 conversion:
- SafeRunnable.java: io.github.merlimat.slog.Logger placed after java.*
- OrderedExecutor.java: lombok.CustomLog placed after org.*
- AbstractLifecycleComponent.java: same
- TestOrderedExecutorDecorators.java: same

Move each import to its correct alphabetical position per the project's
checkstyle rule (`io.` < `java.`; `lombok.` < `org.`).
CI check-binary-license flagged the new io.github.merlimat.slog-slog-0.9.7.jar
as unaccounted for. Add it to the Apache-2.0 bundled-jars list in both
LICENSE-all.bin.txt and LICENSE-bkctl.bin.txt, with a new numbered source
reference pointing at https://github.com/merlimat/slog/tree/v0.9.7.
The previous LICENSE fix only updated LICENSE-all.bin.txt and
LICENSE-bkctl.bin.txt, but the bin-server.xml assembly descriptor
packages LICENSE-server.bin.txt into the server tarball that the CI
license-check script inspects.
Both tests exercise SLF4J MDC context propagation through the
OrderedExecutor / OrderedScheduler decorators. With the slog migration
the logger API is different and these tests are not straightforward to
keep working against the new logger while still testing MDC behaviour
as seen by the underlying SLF4J backend.

Drop them for now. The MDC propagation code itself (MdcUtils,
OrderedExecutor's mdcContextMap snapshot/restore) is unchanged, so
end-to-end MDC-in-logs behaviour under Logback/Log4j2 backends is
preserved.
Pulls in the MDC propagation fix from merlimat/slog#6, which makes
log4j2 ThreadContext entries visible on slog events emitted via the
Log4j2Logger backend (so %X{key} layouts and appenders that read
event.getContextData().getValue(key) see the caller's MDC).
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.

1 participant