Copilot commented on code in PR #10761:
URL: https://github.com/apache/rocketmq/pull/10761#discussion_r3701481016
##########
proxy/src/main/java/org/apache/rocketmq/proxy/service/sysmessage/AbstractSystemMessageSyncer.java:
##########
@@ -109,18 +110,39 @@ protected void sendSystemMessage(Object data) {
Duration.ofSeconds(3).toMillis()
).whenCompleteAsync((result, throwable) -> {
if (throwable != null) {
- log.error("send system message failed. data: {}, topic:
{}", data, getBroadcastTopicName(), throwable);
+ log.error("send system message failed. dataSummary: {},
topic: {}",
+ dataSummary, getBroadcastTopicName(), throwable);
return;
}
if (SendStatus.SEND_OK != result.getSendStatus()) {
- log.error("send system message failed. data: {}, topic:
{}, sendResult:{}", data, getBroadcastTopicName(), result);
+ log.error("send system message failed. dataSummary: {},
topic: {}, sendResult:{}",
+ dataSummary, getBroadcastTopicName(), result);
}
Review Comment:
Inside the async completion handler the log uses `getBroadcastTopicName()`
instead of the already-captured `targetTopic`. If the configured topic changes
between send and callback execution, the error log could report the wrong
topic, making incidents harder to diagnose.
##########
proxy/src/main/java/org/apache/rocketmq/proxy/service/sysmessage/AbstractSystemMessageSyncer.java:
##########
@@ -109,18 +110,39 @@ protected void sendSystemMessage(Object data) {
Duration.ofSeconds(3).toMillis()
).whenCompleteAsync((result, throwable) -> {
if (throwable != null) {
- log.error("send system message failed. data: {}, topic:
{}", data, getBroadcastTopicName(), throwable);
+ log.error("send system message failed. dataSummary: {},
topic: {}",
+ dataSummary, getBroadcastTopicName(), throwable);
return;
}
if (SendStatus.SEND_OK != result.getSendStatus()) {
- log.error("send system message failed. data: {}, topic:
{}, sendResult:{}", data, getBroadcastTopicName(), result);
+ log.error("send system message failed. dataSummary: {},
topic: {}, sendResult:{}",
+ dataSummary, getBroadcastTopicName(), result);
}
});
} catch (Throwable t) {
- log.error("send system message failed. data: {}, topic: {}", data,
targetTopic, t);
+ log.error("send system message failed. dataSummary: {}, topic:
{}", dataSummary, targetTopic, t);
}
}
+ static String summarizeSystemMessageData(Object data) {
+ if (data == null) {
+ return "null";
+ }
+ if (data instanceof HeartbeatSyncerData) {
+ HeartbeatSyncerData heartbeatData = (HeartbeatSyncerData) data;
+ int subscriptionCount = heartbeatData.getSubscriptionDataSet() ==
null
+ ? 0 : heartbeatData.getSubscriptionDataSet().size();
+ return "HeartbeatSyncerData{"
+ + "heartbeatType=" + heartbeatData.getHeartbeatType()
+ + ", clientId=" + heartbeatData.getClientId()
+ + ", group=" + heartbeatData.getGroup()
+ + ", subscriptionCount=" + subscriptionCount
+ + ", channelDataPresent=" + (heartbeatData.getChannelData() !=
null)
+ + '}';
Review Comment:
The new heartbeat `dataSummary` still logs raw `clientId` and `group`. The
linked issue calls out that system-message payloads may contain client/runtime
metadata; if the goal is to avoid leaking such metadata in error logs, consider
redacting these fields (or logging stable hashes / lengths) and keeping only
type + minimal diagnostics (e.g., heartbeatType, subscriptionCount,
channelDataPresent).
##########
proxy/src/main/java/org/apache/rocketmq/proxy/service/sysmessage/AbstractSystemMessageSyncer.java:
##########
@@ -93,6 +93,7 @@ public RPCHook getRpcHook() {
protected void sendSystemMessage(Object data) {
String targetTopic = this.getBroadcastTopicName();
+ String dataSummary = summarizeSystemMessageData(data);
try {
Review Comment:
`dataSummary` is computed before the surrounding `try`, so any unexpected
exception while summarizing (even if unlikely today) would escape
`sendSystemMessage` and potentially prevent the system message from being sent.
Since this value is only for logging, it should be computed defensively so it
can never break the send path.
##########
proxy/src/main/java/org/apache/rocketmq/proxy/service/sysmessage/AbstractSystemMessageSyncer.java:
##########
@@ -109,18 +110,39 @@ protected void sendSystemMessage(Object data) {
Duration.ofSeconds(3).toMillis()
).whenCompleteAsync((result, throwable) -> {
if (throwable != null) {
- log.error("send system message failed. data: {}, topic:
{}", data, getBroadcastTopicName(), throwable);
+ log.error("send system message failed. dataSummary: {},
topic: {}",
+ dataSummary, getBroadcastTopicName(), throwable);
return;
}
if (SendStatus.SEND_OK != result.getSendStatus()) {
- log.error("send system message failed. data: {}, topic:
{}, sendResult:{}", data, getBroadcastTopicName(), result);
+ log.error("send system message failed. dataSummary: {},
topic: {}, sendResult:{}",
+ dataSummary, getBroadcastTopicName(), result);
}
});
} catch (Throwable t) {
- log.error("send system message failed. data: {}, topic: {}", data,
targetTopic, t);
+ log.error("send system message failed. dataSummary: {}, topic:
{}", dataSummary, targetTopic, t);
}
}
+ static String summarizeSystemMessageData(Object data) {
+ if (data == null) {
+ return "null";
+ }
+ if (data instanceof HeartbeatSyncerData) {
+ HeartbeatSyncerData heartbeatData = (HeartbeatSyncerData) data;
+ int subscriptionCount = heartbeatData.getSubscriptionDataSet() ==
null
+ ? 0 : heartbeatData.getSubscriptionDataSet().size();
+ return "HeartbeatSyncerData{"
+ + "heartbeatType=" + heartbeatData.getHeartbeatType()
+ + ", clientId=" + heartbeatData.getClientId()
+ + ", group=" + heartbeatData.getGroup()
+ + ", subscriptionCount=" + subscriptionCount
+ + ", channelDataPresent=" + (heartbeatData.getChannelData() !=
null)
+ + '}';
+ }
+ return data.getClass().getSimpleName();
+ }
Review Comment:
For non-`HeartbeatSyncerData` payloads, `data.getClass().getSimpleName()`
can return an empty string (e.g., anonymous classes), which would make
`dataSummary` unhelpful. Consider falling back to the fully-qualified class
name when the simple name is empty.
--
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]
