You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "merlimat (via GitHub)" <gi...@apache.org> on 2026/04/15 23:29:31 UTC
[PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
merlimat opened a new pull request, #25536:
URL: https://github.com/apache/pulsar/pull/25536
## Summary
- Convert 358 test files in the `pulsar-broker` module from SLF4J to slog structured logging
- Apply the same conversion patterns used in the main sources PR (#25535): structured `.attr(...)` calls, `.exception(...)` for throwables, and short form `log.level("msg")` when no attributes are present
- Strip stale SLF4J `{}` placeholder text from log messages
## Test plan
- [x] `./gradlew :pulsar-broker:compileTestJava` passes
- [x] `./gradlew :pulsar-broker:checkstyleTest` passes
- [ ] CI passes
### Motivation
PIP-467
> [!NOTE]
> Please label this PR with `area/test` and `ready-to-test`
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091436045
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java:
##########
@@ -5021,11 +5034,11 @@ public void onSendAcknowledgement(Producer producer, Message message, MessageId
// Create message payload, refCnf = 1 now.
ByteBuf payload = PulsarByteBufAllocator.DEFAULT.heapBuffer(1);
payloads[i] = payload;
- log.info("payload_{}.refCnf 1st: {}", i, payload.refCnt());
+ log.info().attr("payload", i).attr("st", payload.refCnt()).log("payload_ .refCnf 1st");
Review Comment:
```suggestion
log.info().attr("payloadIndex", i).attr("refCnt", payload.refCnt()).log("Payload refCnt at first check“);
```
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java:
##########
@@ -5021,11 +5034,11 @@ public void onSendAcknowledgement(Producer producer, Message message, MessageId
// Create message payload, refCnf = 1 now.
ByteBuf payload = PulsarByteBufAllocator.DEFAULT.heapBuffer(1);
payloads[i] = payload;
- log.info("payload_{}.refCnf 1st: {}", i, payload.refCnt());
+ log.info().attr("payload", i).attr("st", payload.refCnt()).log("payload_ .refCnf 1st");
Review Comment:
**`"st"`, `"nd"`, `"th"` are ordinal suffixes extracted from the message text — not valid attr names for a `refCnt()` value.**
```suggestion
log.info().attr("payloadIndex", i).attr("refCount", payload.refCnt()).log("payload refCount checkpoint 1");
```
Same for lines 5041 (`"nd"` → `"refCount"`, message `"checkpoint 2"`) and 5067 (`"th"` → `"refCount"`, message `"checkpoint 4"`).
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat merged PR #25536:
URL: https://github.com/apache/pulsar/pull/25536
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091447046
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java:
##########
@@ -5021,11 +5034,11 @@ public void onSendAcknowledgement(Producer producer, Message message, MessageId
// Create message payload, refCnf = 1 now.
ByteBuf payload = PulsarByteBufAllocator.DEFAULT.heapBuffer(1);
payloads[i] = payload;
- log.info("payload_{}.refCnf 1st: {}", i, payload.refCnt());
+ log.info().attr("payload", i).attr("st", payload.refCnt()).log("payload_ .refCnf 1st");
payload.writeByte(i);
// refCnf = 2 now.
payload.retain();
- log.info("payload_{}.refCnf 2nd: {}", i, payload.refCnt());
+ log.info().attr("payload", i).attr("nd", payload.refCnt()).log("payload_ .refCnf 2nd");
Review Comment:
```suggestion
log.info().attr("payload", i).attr("refCnt", payload.refCnt()).log("Payload refCnt (2nd)");
```
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091369753
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/LoadBalancerTest.java:
##########
@@ -708,18 +711,19 @@ public void testLeaderElection() throws Exception {
}
}
// Make sure all brokers see the same leader
- log.info("Old leader is : {}", oldLeader.getBrokerId());
+ log.info().attr("brokerId", oldLeader.getBrokerId()).log("Old leader");
Review Comment:
**`"Old leader"` lost the "is" context from `"Old leader is : {}"` — the message now reads ambiguously without the verb.**
```suggestion
log.info().attr("brokerId", oldLeader.getBrokerId()).log("Old leader is");
```
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/LoadBalancerTest.java:
##########
@@ -432,8 +433,10 @@ public void testTopicAssignmentWithExistingBundles() throws Exception {
long expectedValue = expectedAssignments[i];
double variation = Math.abs(actualValue - expectedValue) * 100.0 / expectedValue;
- log.info("Topic assignment - {}, actual: {}, expected baseline: {}, variation: {}/%",
- lookupAddresses[i], actualValue, expectedValue, String.format("%.2f", variation));
+ log.info().attr("address", lookupAddresses[i]).attr("actual", actualValue)
+ .attr("expectedBaseline", expectedValue)
+ .attr("variation", String.format("%.2f", variation))
Review Comment:
**`"variation"` attr lost the `%` unit from the original `"variation: {}/% "` message.**
Suggestion: rename the attr to make the unit self-documenting:
```suggestion
.attr("variationPct", String.format("%.2f", variation))
```
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091384103
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java:
##########
@@ -1214,8 +1213,12 @@ public void testActiveAndInActiveConsumerEntryCacheBehavior() throws Exception {
// Make the test fail if there's a cache miss
pulsarTestContext.getMockBookKeeper().setReadHandleInterceptor((ledgerId, firstEntry, lastEntry, entries) -> {
- log.error("Attempting to read from BK when cache should be used. {}:{} to {}:{}", ledgerId, firstEntry,
- ledgerId, lastEntry);
+ log.error()
+ .attr("used", ledgerId)
+ .attr("firstEntry", firstEntry)
+ .attr("ledgerId", ledgerId)
+ .attr("lastEntry", lastEntry)
+ .log("Attempting to read from BK when cache should be used. : to");
Review Comment:
```suggestion
log.error()
.attr("ledgerId", ledgerId)
.attr("firstEntry", firstEntry)
.attr("lastEntry", lastEntry)
.log("Attempting to read from BK when cache should be used");
```
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091367445
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/PatternTopicsConsumerImplTest.java:
##########
@@ -830,8 +829,10 @@ private void waitTopicListWatcherCreation(Consumer<?> consumer) {
Awaitility.await().untilAsserted(() -> {
CompletableFuture<TopicListWatcher> completableFuture =
((PatternMultiTopicsConsumerImpl) consumer).getWatcherFuture();
- log.info("isDone: {}, isCompletedExceptionally: {}", completableFuture.isDone(),
- completableFuture.isCompletedExceptionally());
+ log.info()
+ .attr("isDone", completableFuture.isDone())
+ .attr("isCompletedExceptionally", completableFuture.isCompletedExceptionally())
+ .log("isDone: , isCompletedExceptionally");
Review Comment:
**Malformed message — looks like an unconverted SLF4J template fragment with a stray colon and comma.**
```suggestion
.log("Future completed successfully");
```
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/MessageChunkingTest.java:
##########
@@ -627,7 +630,7 @@ public void testChunkSize() throws Exception {
.create();
for (int size = 1; size <= maxMessageSize; size++) {
final MessageId messageId = producer.send(createMessagePayload(size));
- log.info("Send {} bytes to {}", size, messageId);
+ log.info().attr("size", size).attr("messageId", messageId).log("Sent bytes");
Review Comment:
**`"Sent bytes"` lost the destination context from the original `"Send {} bytes to {}"` message.**
Since `messageId` is already an attr, a clearer message would be:
```suggestion
log.info().attr("size", size).attr("messageId", messageId).log("Sent bytes to messageId");
```
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091358292
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/DeadLetterTopicTest.java:
##########
@@ -125,16 +123,17 @@ public void testDeadLetterTopicWithMessageKey() throws Exception {
int totalReceived = 0;
do {
Message<byte[]> message = consumer.receive();
- log.info("consumer received message : {} {}", message.getMessageId(), new String(message.getData()));
+ log.info().attr("receivedMessage", message.getMessageId()).attr("value", new String(message.getData()))
Review Comment:
**`"receivedMessage"` is a verb phrase copied from the message text, not a name for a `MessageId` value. This pattern repeats ~30 times in this file.**
```suggestion
log.info().attr("messageId", message.getMessageId()).attr("data", new String(message.getData()))
```
All other `attr("receivedMessage", message.getMessageId())` occurrences should be renamed to `"messageId"` as well.
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091362822
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java:
##########
@@ -5021,11 +5034,11 @@ public void onSendAcknowledgement(Producer producer, Message message, MessageId
// Create message payload, refCnf = 1 now.
ByteBuf payload = PulsarByteBufAllocator.DEFAULT.heapBuffer(1);
payloads[i] = payload;
- log.info("payload_{}.refCnf 1st: {}", i, payload.refCnt());
+ log.info().attr("payload", i).attr("st", payload.refCnt()).log("payload_ .refCnf 1st");
Review Comment:
**`"st"`, `"nd"`, `"th"` are ordinal suffixes extracted from the message text — not valid attr names for a `refCnt()` value.**
```suggestion
log.info().attr("payloadIndex", i).attr("refCount", payload.refCnt()).log("payload refCount checkpoint 1");
```
Same for lines 5041 (`"nd"` → `"refCount"`, message `"checkpoint 2"`) and 5067 (`"th"` → `"refCount"`, message `"checkpoint 4"`).
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java:
##########
@@ -3195,7 +3204,7 @@ public EncryptionKeyInfo getPublicKey(String keyName, Map<String, String> keyMet
keyInfo.setKey(Files.readAllBytes(Paths.get(certFilePath)));
return keyInfo;
} catch (IOException e) {
- log.error("Failed to read certificate from {}", certFilePath);
+ log.error().attr("certificate", certFilePath).log("Failed to read certificate from");
Review Comment:
**`"certificate"` is misleading — the value is a file path, not a certificate object.**
```suggestion
log.error().attr("certFilePath", certFilePath).log("Failed to read certificate");
```
Same fix applies to line 3221.
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091365440
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/PartitionedProducerConsumerTest.java:
##########
@@ -76,7 +75,7 @@ public void cleanupExecutor() throws Exception {
@Test(timeOut = 30000)
public void testRoundRobinProducer() throws Exception {
- log.info("-- Starting {} test --", methodName);
+ log.info().attr("starting", methodName).log("-- Starting test");
Review Comment:
**`"starting"` and `"exiting"` are adjectives/verbs from the message text, not names for `methodName` value. This repeats 24+ times in the file.**
```suggestion
log.info().attr("testMethod", methodName).log("Starting test");
```
Same for all `attr("exiting", methodName)` → `attr("testMethod", methodName)`.
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/MessageChunkingTest.java:
##########
@@ -627,7 +630,7 @@ public void testChunkSize() throws Exception {
.create();
for (int size = 1; size <= maxMessageSize; size++) {
final MessageId messageId = producer.send(createMessagePayload(size));
- log.info("Send {} bytes to {}", size, messageId);
+ log.info().attr("size", size).attr("messageId", messageId).log("Sent bytes");
Review Comment:
**`"Sent bytes"` lost the destination context from the original `"Send {} bytes to {}"` message.**
Since `messageId` is already an attr, a clearer message would be:
```suggestion
log.info().attr("size", size).attr("messageId", messageId).log("Sent bytes to messageId");
```
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091358292
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/DeadLetterTopicTest.java:
##########
@@ -125,16 +123,17 @@ public void testDeadLetterTopicWithMessageKey() throws Exception {
int totalReceived = 0;
do {
Message<byte[]> message = consumer.receive();
- log.info("consumer received message : {} {}", message.getMessageId(), new String(message.getData()));
+ log.info().attr("receivedMessage", message.getMessageId()).attr("value", new String(message.getData()))
Review Comment:
**`"receivedMessage"` is a verb phrase copied from the message text, not a name for a `MessageId` value. This pattern repeats ~30 times in this file.**
```suggestion
log.info().attr("messageId", message.getMessageId()).attr("data", new String(message.getData()))
```
All other `attr("receivedMessage", message.getMessageId())` occurrences should be renamed to `"messageId"` as well.
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/RetryTopicTest.java:
##########
@@ -313,9 +325,11 @@ public void testAutoConsumeSchemaRetryLetter() throws Exception {
int totalReceived = 0;
do {
Message<GenericRecord> message = consumer.receive();
- log.info(
- "consumer received message (schema={}) : {} {}",
- message.getReaderSchema().get(), message.getMessageId(), new String(message.getData()));
+ log.info()
+ .attr("schema", message.getReaderSchema().get())
+ .attr("messageId", message.getMessageId())
+ .attr("data", new String(message.getData()))
+ .log("consumer received message (schema");
Review Comment:
**Truncated log message — missing closing `)`**
```suggestion
.log("consumer received message");
```
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091365455
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionProduceTest.java:
##########
@@ -183,7 +182,7 @@ private void produceTest(boolean endAction) throws Exception {
}
Assert.assertEquals(0, messageSet.size());
- log.info("produce and {} test finished.", endAction ? "commit" : "abort");
+ log.info().attr("produceAnd", endAction ? "commit" : "abort").log("produce and test finished.");
Review Comment:
**`"produceAnd"` is a partial verb phrase from the message text, not a name for the `endAction` value.**
```suggestion
log.info().attr("endAction", endAction ? "commit" : "abort").log("produce and commit/abort test finished");
```
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/PartitionedProducerConsumerTest.java:
##########
@@ -76,7 +75,7 @@ public void cleanupExecutor() throws Exception {
@Test(timeOut = 30000)
public void testRoundRobinProducer() throws Exception {
- log.info("-- Starting {} test --", methodName);
+ log.info().attr("starting", methodName).log("-- Starting test");
Review Comment:
**`"starting"` and `"exiting"` are adjectives/verbs from the message text, not names for `methodName` value. This repeats 24+ times in the file.**
```suggestion
log.info().attr("testMethod", methodName).log("Starting test");
```
Same for all `attr("exiting", methodName)` → `attr("testMethod", methodName)`.
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091345822
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java:
##########
@@ -1214,8 +1213,12 @@ public void testActiveAndInActiveConsumerEntryCacheBehavior() throws Exception {
// Make the test fail if there's a cache miss
pulsarTestContext.getMockBookKeeper().setReadHandleInterceptor((ledgerId, firstEntry, lastEntry, entries) -> {
- log.error("Attempting to read from BK when cache should be used. {}:{} to {}:{}", ledgerId, firstEntry,
- ledgerId, lastEntry);
+ log.error()
+ .attr("used", ledgerId)
+ .attr("firstEntry", firstEntry)
+ .attr("ledgerId", ledgerId)
+ .attr("lastEntry", lastEntry)
+ .log("Attempting to read from BK when cache should be used. : to");
Review Comment:
**Bug: `"used"` is a word from the message text, not the variable name. Also `ledgerId` appears twice and the message text is garbled.**
Original SLF4J:
```java
log.error("Attempting to read from BK when cache should be used. {}:{} to {}:{}", ledgerId, firstEntry, ledgerId, lastEntry);
```
Suggested fix:
```java
log.error()
.attr("ledgerId", ledgerId)
.attr("firstEntry", firstEntry)
.attr("lastEntry", lastEntry)
.log("Attempting to read from BK when cache should be used");
```
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091423133
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/DeadLetterTopicTest.java:
##########
@@ -1524,20 +1532,21 @@ public void testDeadLetterTopicWithMaxUnackedMessagesBlocking() throws Exception
Message<byte[]> message = deadLetterConsumer.receive();
if (message != null) {
String messageContent = new String(message.getData());
- log.info("Dead letter message received: {}", messageContent);
+ log.info().attr("messageReceived", messageContent).log("Dead letter message received");
deadLetterConsumer.acknowledge(message);
totalInDeadLetter++;
} else {
log.warn("No more messages in DLQ");
break;
}
} catch (Exception e) {
- log.warn("Exception while receiving from DLQ", e);
+ log.warn().exception(e).log("Exception while receiving from DLQ");
break;
}
}
- log.info("Total messages in dead letter queue: {}, Expected: {}", totalInDeadLetter, sendMessages);
+ log.info().attr("letterQueue", totalInDeadLetter).attr("expected", sendMessages)
+ .log("Total messages in dead letter queue, Expected");
Review Comment:
```suggestion
.log("Total messages in dead letter queue");
```
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091384103
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java:
##########
@@ -1214,8 +1213,12 @@ public void testActiveAndInActiveConsumerEntryCacheBehavior() throws Exception {
// Make the test fail if there's a cache miss
pulsarTestContext.getMockBookKeeper().setReadHandleInterceptor((ledgerId, firstEntry, lastEntry, entries) -> {
- log.error("Attempting to read from BK when cache should be used. {}:{} to {}:{}", ledgerId, firstEntry,
- ledgerId, lastEntry);
+ log.error()
+ .attr("used", ledgerId)
+ .attr("firstEntry", firstEntry)
+ .attr("ledgerId", ledgerId)
+ .attr("lastEntry", lastEntry)
+ .log("Attempting to read from BK when cache should be used. : to");
Review Comment:
```suggestion
log.error()
.attr("ledgerId", ledgerId)
.attr("firstEntry", firstEntry)
.attr("ledgerId", ledgerId)
.attr("lastEntry", lastEntry)
.log("Attempting to read from BK when cache should be used");
```
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java:
##########
@@ -1214,8 +1213,12 @@ public void testActiveAndInActiveConsumerEntryCacheBehavior() throws Exception {
// Make the test fail if there's a cache miss
pulsarTestContext.getMockBookKeeper().setReadHandleInterceptor((ledgerId, firstEntry, lastEntry, entries) -> {
- log.error("Attempting to read from BK when cache should be used. {}:{} to {}:{}", ledgerId, firstEntry,
- ledgerId, lastEntry);
+ log.error()
+ .attr("used", ledgerId)
+ .attr("firstEntry", firstEntry)
+ .attr("ledgerId", ledgerId)
+ .attr("lastEntry", lastEntry)
+ .log("Attempting to read from BK when cache should be used. : to");
Review Comment:
**Bug: `"used"` is a word from the message text, not the variable name. Also `ledgerId` appears twice and the message text is garbled.**
Original SLF4J:
```java
log.error("Attempting to read from BK when cache should be used. {}:{} to {}:{}", ledgerId, firstEntry, ledgerId, lastEntry);
```
Suggested fix:
```java
log.error()
.attr("ledgerId", ledgerId)
.attr("firstEntry", firstEntry)
.attr("lastEntry", lastEntry)
.log("Attempting to read from BK when cache should be used");
```
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091436045
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java:
##########
@@ -5021,11 +5034,11 @@ public void onSendAcknowledgement(Producer producer, Message message, MessageId
// Create message payload, refCnf = 1 now.
ByteBuf payload = PulsarByteBufAllocator.DEFAULT.heapBuffer(1);
payloads[i] = payload;
- log.info("payload_{}.refCnf 1st: {}", i, payload.refCnt());
+ log.info().attr("payload", i).attr("st", payload.refCnt()).log("payload_ .refCnf 1st");
Review Comment:
```suggestion
log.info().attr("payload", i).attr("refCnt", payload.refCnt()).log("Payload refCnt (1st)“);
```
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091360166
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/DeadLetterTopicTest.java:
##########
@@ -1524,20 +1532,21 @@ public void testDeadLetterTopicWithMaxUnackedMessagesBlocking() throws Exception
Message<byte[]> message = deadLetterConsumer.receive();
if (message != null) {
String messageContent = new String(message.getData());
- log.info("Dead letter message received: {}", messageContent);
+ log.info().attr("messageReceived", messageContent).log("Dead letter message received");
deadLetterConsumer.acknowledge(message);
totalInDeadLetter++;
} else {
log.warn("No more messages in DLQ");
break;
}
} catch (Exception e) {
- log.warn("Exception while receiving from DLQ", e);
+ log.warn().exception(e).log("Exception while receiving from DLQ");
break;
}
}
- log.info("Total messages in dead letter queue: {}, Expected: {}", totalInDeadLetter, sendMessages);
+ log.info().attr("letterQueue", totalInDeadLetter).attr("expected", sendMessages)
+ .log("Total messages in dead letter queue, Expected");
Review Comment:
**`"letterQueue"` is a noun for the queue itself, not a count variable name.**
```suggestion
log.info().attr("totalInDeadLetter", totalInDeadLetter).attr("expected", sendMessages)
```
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091362855
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java:
##########
@@ -3195,7 +3204,7 @@ public EncryptionKeyInfo getPublicKey(String keyName, Map<String, String> keyMet
keyInfo.setKey(Files.readAllBytes(Paths.get(certFilePath)));
return keyInfo;
} catch (IOException e) {
- log.error("Failed to read certificate from {}", certFilePath);
+ log.error().attr("certificate", certFilePath).log("Failed to read certificate from");
Review Comment:
**`"certificate"` is misleading — the value is a file path, not a certificate object.**
```suggestion
log.error().attr("certFilePath", certFilePath).log("Failed to read certificate");
```
Same fix applies to line 3221.
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091369778
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/LoadBalancerTest.java:
##########
@@ -432,8 +433,10 @@ public void testTopicAssignmentWithExistingBundles() throws Exception {
long expectedValue = expectedAssignments[i];
double variation = Math.abs(actualValue - expectedValue) * 100.0 / expectedValue;
- log.info("Topic assignment - {}, actual: {}, expected baseline: {}, variation: {}/%",
- lookupAddresses[i], actualValue, expectedValue, String.format("%.2f", variation));
+ log.info().attr("address", lookupAddresses[i]).attr("actual", actualValue)
+ .attr("expectedBaseline", expectedValue)
+ .attr("variation", String.format("%.2f", variation))
Review Comment:
**`"variation"` attr lost the `%` unit from the original `"variation: {}/% "` message.**
Suggestion: rename the attr to make the unit self-documenting:
```suggestion
.attr("variationPct", String.format("%.2f", variation))
```
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091369753
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/LoadBalancerTest.java:
##########
@@ -708,18 +711,19 @@ public void testLeaderElection() throws Exception {
}
}
// Make sure all brokers see the same leader
- log.info("Old leader is : {}", oldLeader.getBrokerId());
+ log.info().attr("brokerId", oldLeader.getBrokerId()).log("Old leader");
Review Comment:
**`"Old leader"` lost the "is" context from `"Old leader is : {}"` — the message now reads ambiguously without the verb.**
```suggestion
log.info().attr("brokerId", oldLeader.getBrokerId()).log("Old leader is");
```
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091360166
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/DeadLetterTopicTest.java:
##########
@@ -1524,20 +1532,21 @@ public void testDeadLetterTopicWithMaxUnackedMessagesBlocking() throws Exception
Message<byte[]> message = deadLetterConsumer.receive();
if (message != null) {
String messageContent = new String(message.getData());
- log.info("Dead letter message received: {}", messageContent);
+ log.info().attr("messageReceived", messageContent).log("Dead letter message received");
deadLetterConsumer.acknowledge(message);
totalInDeadLetter++;
} else {
log.warn("No more messages in DLQ");
break;
}
} catch (Exception e) {
- log.warn("Exception while receiving from DLQ", e);
+ log.warn().exception(e).log("Exception while receiving from DLQ");
break;
}
}
- log.info("Total messages in dead letter queue: {}, Expected: {}", totalInDeadLetter, sendMessages);
+ log.info().attr("letterQueue", totalInDeadLetter).attr("expected", sendMessages)
+ .log("Total messages in dead letter queue, Expected");
Review Comment:
**`"letterQueue"` is a noun for the queue itself, not a count variable name.**
```suggestion
log.info().attr("totalInDeadLetter", totalInDeadLetter).attr("expected", sendMessages)
```
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java:
##########
@@ -4998,7 +5009,9 @@ public Message beforeSend(Producer producer, Message message) {
public void onSendAcknowledgement(Producer producer, Message message, MessageId msgId,
Throwable exception) {
MessageImpl msgImpl = (MessageImpl) message;
- log.info("payload.refCnf on send acknowledgement: {}", msgImpl.getDataBuffer().refCnt());
+ log.info()
+ .attr("acknowledgement", msgImpl.getDataBuffer().refCnt())
Review Comment:
**`"acknowledgement"` is the context from the message text, not the name for the `refCnt()` value.**
```suggestion
.attr("refCount", msgImpl.getDataBuffer().refCnt())
```
--
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
Re: [PR] [improve][test] PIP-467: Convert pulsar-broker test files logging from SLF4J to slog [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #25536:
URL: https://github.com/apache/pulsar/pull/25536#discussion_r3091371793
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicPoliciesTest.java:
##########
@@ -2194,7 +2332,10 @@ public void testRemovePublishRate() throws Exception {
@Test
public void testCheckMaxConsumers() throws Exception {
Integer maxProducers = -1;
- log.info("MaxConsumers: {} will set to the topic: {}", maxProducers, testTopic);
+ log.info()
+ .attr("maxconsumers", maxProducers)
+ .attr("willSetToTheTopic", testTopic)
+ .log("MaxConsumers: will set to the topic");
Review Comment:
**Bug: `"maxconsumers"` attr key but value is the `maxProducers` variable — copy-paste mismatch. Also `"willSetToTheTopic"` is a sentence fragment, not an attr name.**
```suggestion
.log("MaxConsumers will be set to the topic");
```
And change the attr key on the line above:
- `attr("maxconsumers", maxProducers)` → `attr("maxConsumers", maxProducers)` (camelCase, and the value IS the maxConsumers limit being set)
- `attr("willSetToTheTopic", testTopic)` → `attr("topic", testTopic)`
This same pattern (`"willSetToTheTopic"`, `"willSetToTheNamespace"`) appears 20+ times in this file and should all be simplified to `"topic"` / `"namespace"`.
--
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