Copilot commented on code in PR #10981:
URL: https://github.com/apache/rocketmq/pull/10981#discussion_r3871438224
##########
store/src/test/java/org/apache/rocketmq/store/pop/BatchAckMsgTest.java:
##########
@@ -21,6 +21,7 @@
import org.junit.Assert;
import org.junit.Test;
+import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.List;
Review Comment:
Import grouping/order is inconsistent here (java.* imports come after
org.junit.*). If the project has Checkstyle/Spotless rules for import ordering,
this can fail the build. Reorder imports to match the repository’s convention
(typically java.* first, then third-party, then org.*—or whatever the existing
files enforce).
##########
broker/src/main/java/org/apache/rocketmq/broker/processor/PopReviveService.java:
##########
@@ -400,11 +399,10 @@ protected void consumeReviveMessage(ConsumeReviveObj
consumeReviveObj) {
firstRt = point.getReviveTime();
}
} else if
(PopAckConstants.ACK_TAG.equals(messageExt.getTags())) {
- String raw = new String(messageExt.getBody(),
StandardCharsets.UTF_8);
if (brokerController.getBrokerConfig().isEnablePopLog()) {
- POP_LOGGER.info("reviveQueueId={}, find ack,
offset:{}, raw : {}", messageExt.getQueueId(), messageExt.getQueueOffset(),
raw);
+ POP_LOGGER.info("reviveQueueId={}, find ack,
offset:{}, raw : {}", messageExt.getQueueId(), messageExt.getQueueOffset(), new
String(messageExt.getBody(), StandardCharsets.UTF_8));
Review Comment:
These `new String(messageExt.getBody(), ...)` allocations happen whenever
`enablePopLog` is true, even if the logger’s INFO level is disabled (the String
is created before the logging call). To avoid unnecessary allocations in
production, guard with `POP_LOGGER.isInfoEnabled()` (or compute the raw string
into a local variable only inside an `isInfoEnabled()` branch) before calling
`info(...)`.
##########
broker/src/main/java/org/apache/rocketmq/broker/processor/PopReviveService.java:
##########
@@ -426,12 +424,11 @@ protected void consumeReviveMessage(ConsumeReviveObj
consumeReviveObj) {
}
}
} else if
(PopAckConstants.BATCH_ACK_TAG.equals(messageExt.getTags())) {
- String raw = new String(messageExt.getBody(),
StandardCharsets.UTF_8);
if (brokerController.getBrokerConfig().isEnablePopLog()) {
- POP_LOGGER.info("reviveQueueId={}, find batch ack,
offset:{}, raw : {}", messageExt.getQueueId(), messageExt.getQueueOffset(),
raw);
+ POP_LOGGER.info("reviveQueueId={}, find batch ack,
offset:{}, raw : {}", messageExt.getQueueId(), messageExt.getQueueOffset(), new
String(messageExt.getBody(), StandardCharsets.UTF_8));
Review Comment:
These `new String(messageExt.getBody(), ...)` allocations happen whenever
`enablePopLog` is true, even if the logger’s INFO level is disabled (the String
is created before the logging call). To avoid unnecessary allocations in
production, guard with `POP_LOGGER.isInfoEnabled()` (or compute the raw string
into a local variable only inside an `isInfoEnabled()` branch) before calling
`info(...)`.
##########
broker/src/main/java/org/apache/rocketmq/broker/processor/PopReviveService.java:
##########
@@ -385,11 +385,10 @@ protected void consumeReviveMessage(ConsumeReviveObj
consumeReviveObj) {
}
for (MessageExt messageExt : messageExts) {
if (PopAckConstants.CK_TAG.equals(messageExt.getTags())) {
- String raw = new String(messageExt.getBody(),
DataConverter.CHARSET_UTF8);
if (brokerController.getBrokerConfig().isEnablePopLog()) {
Review Comment:
These `new String(messageExt.getBody(), ...)` allocations happen whenever
`enablePopLog` is true, even if the logger’s INFO level is disabled (the String
is created before the logging call). To avoid unnecessary allocations in
production, guard with `POP_LOGGER.isInfoEnabled()` (or compute the raw string
into a local variable only inside an `isInfoEnabled()` branch) before calling
`info(...)`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
