Re: [PR] [improve] PIP-467: Convert managed-ledger module logging from SLF4J to slog [pulsar]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #25490:
URL: https://github.com/apache/pulsar/pull/25490#discussion_r3061842038


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ReadOnlyCursorImpl.java:
##########
@@ -28,7 +28,7 @@
 import org.apache.bookkeeper.mledger.ReadOnlyCursor;
 import org.apache.bookkeeper.mledger.proto.ManagedLedgerInfo;
 
-@Slf4j
+@CustomLog
 public class ReadOnlyCursorImpl extends ManagedCursorImpl implements ReadOnlyCursor {

Review Comment:
   ReadOnlyCursorImpl extends ManagedCursorImpl, which now has an instance `log` derived with cursor/ledger context. `@CustomLog` will generate a static `log` field that hides the superclass logger, making it easy to accidentally bypass the contextual logger. Prefer removing `@CustomLog` here and using the inherited `log` (or explicitly `super.log`).



##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/NonDurableCursorTest.java:
##########
@@ -463,10 +463,10 @@ void rewind() throws Exception {
         Position p3 = ledger.addEntry("dummy-entry-3".getBytes(Encoding));
         Position p4 = ledger.addEntry("dummy-entry-4".getBytes(Encoding));
 
-        log.debug("p1: {}", p1);
-        log.debug("p2: {}", p2);
-        log.debug("p3: {}", p3);
-        log.debug("p4: {}", p4);
+        log.debug("p1: " + p1);
+        log.debug("p2: " + p2);
+        log.debug("p3: " + p3);
+        log.debug("p4: " + p4);

Review Comment:
   These debug logs build strings via concatenation, which does work even when DEBUG is disabled and also loses the structured logging pattern used elsewhere in the migration. Prefer using structured attributes (e.g., `log.debug().attr("p1", p1).log(...)`) or a lazy lambda to avoid unnecessary allocations on the disabled path.



##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ReadOnlyManagedLedgerImpl.java:
##########
@@ -39,7 +39,7 @@
 import org.apache.bookkeeper.mledger.proto.ManagedLedgerInfo.LedgerInfo;
 import org.apache.pulsar.metadata.api.Stat;
 
-@Slf4j
+@CustomLog
 public class ReadOnlyManagedLedgerImpl extends ManagedLedgerImpl {

Review Comment:
   This class extends ManagedLedgerImpl which already provides an instance `log` derived with persistent context. Adding `@CustomLog` introduces a static `log` field that hides the superclass logger, which undermines the intended contextual logging. Consider removing `@CustomLog` and using the inherited `log` (or `super.log`) instead.



##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ShadowManagedLedgerImpl.java:
##########
@@ -46,7 +46,7 @@
 /**
  * Detailed design can be found in <a href="https://github.com/apache/pulsar/issues/16153">PIP-180</a>.
  */
-@Slf4j
+@CustomLog
 public class ShadowManagedLedgerImpl extends ManagedLedgerImpl {

Review Comment:
   `@CustomLog` generates a static `log` field which hides the inherited `ManagedLedgerImpl.log` (a derived slog logger with persistent context). As a result, logging in this subclass will not use the managed-ledger contextual logger. Prefer removing `@CustomLog` here and using the inherited `log` (or reference it explicitly as `super.log`) so the persistent context is preserved consistently.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org