Copilot commented on code in PR #10443:
URL: https://github.com/apache/rocketmq/pull/10443#discussion_r3372047512
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/RemotingCodeDistributionHandler.java:
##########
@@ -32,19 +32,35 @@ public class RemotingCodeDistributionHandler extends
ChannelDuplexHandler {
private final ConcurrentMap<Integer, LongAdder> inboundDistribution;
private final ConcurrentMap<Integer, LongAdder> outboundDistribution;
+ private volatile int lastInCode = Integer.MIN_VALUE;
+ private volatile LongAdder lastInAdder;
+ private volatile int lastOutCode = Integer.MIN_VALUE;
+ private volatile LongAdder lastOutAdder;
public RemotingCodeDistributionHandler() {
inboundDistribution = new ConcurrentHashMap<>();
outboundDistribution = new ConcurrentHashMap<>();
}
private void countInbound(int requestCode) {
+ if (requestCode == lastInCode) {
+ lastInAdder.increment();
+ return;
+ }
LongAdder item = inboundDistribution.computeIfAbsent(requestCode, k ->
new LongAdder());
+ lastInCode = requestCode;
+ lastInAdder = item;
item.increment();
}
private void countOutbound(int responseCode) {
+ if (responseCode == lastOutCode) {
+ lastOutAdder.increment();
+ return;
+ }
LongAdder item = outboundDistribution.computeIfAbsent(responseCode, k
-> new LongAdder());
+ lastOutCode = responseCode;
+ lastOutAdder = item;
item.increment();
}
Review Comment:
The fast-path cache updates `last*Code` before `last*Adder`. If this handler
instance is ever accessed concurrently (e.g., shared handler, or cross-thread
calls), another thread can observe the new code but still see a stale/null
adder, leading to incorrect counting or NPE. Update ordering should ensure the
adder write happens-before the code is published (e.g., assign `last*Adder`
first, then `last*Code`, and consider guarding the fast path with `last*Adder
!= null`). Also consider the sentinel initialization: if a code can equal
`Integer.MIN_VALUE`, the initial fast-path hit would NPE.
##########
remoting/src/main/java/org/apache/rocketmq/remoting/metrics/RemotingMetricsManager.java:
##########
@@ -86,6 +97,44 @@ public List<Pair<InstrumentSelector, ViewBuilder>>
getMetricsView() {
return Lists.newArrayList(new Pair<>(selector, viewBuilder));
}
+ public Attributes getOrBuildAttributes(int requestCode, int responseCode,
+ boolean isLongPolling, String result) {
+ int resultIdx;
+ if (result == RESULT_SUCCESS) resultIdx = 0;
+ else if (result == RESULT_ONEWAY) resultIdx = 1;
+ else if (result == RESULT_WRITE_CHANNEL_FAILED) resultIdx = 2;
+ else if (result == RESULT_CANCELED) resultIdx = 3;
+ else resultIdx = -1;
Review Comment:
String comparison uses reference equality (`==`), which can fail when
`result` is a different String instance with the same content. This will
incorrectly bypass caching (and change the computed `resultIdx`). Use content
comparison (e.g., `RESULT_SUCCESS.equals(result)`) or a switch on the string
value.
##########
common/src/main/java/org/apache/rocketmq/common/attribute/TopicMessageType.java:
##########
@@ -67,6 +69,6 @@ public static TopicMessageType
parseFromMessageProperty(Map<String, String> mess
}
public String getMetricsValue() {
- return value.toLowerCase();
+ return metricsValue;
}
Review Comment:
Using `String.toLowerCase()` without an explicit locale makes the metrics
label value depend on the JVM default locale (and this change also locks that
value at enum initialization time). To make metrics stable across environments,
use `toLowerCase(Locale.ROOT)` when computing `metricsValue`.
##########
broker/src/main/java/org/apache/rocketmq/broker/metrics/BrokerMetricsManager.java:
##########
@@ -195,12 +202,31 @@ public AttributesBuilder newAttributesBuilder() {
return attributesBuilder;
}
+ public Attributes getOrBuildTopicAttributes(String topic, String
messageType, boolean isSystem) {
+ Attributes lastAttrs = this.lastTopicAttributes;
+ if (lastAttrs != null && topic.equals(this.lastTopicName) &&
messageType.equals(this.lastTopicMsgType)) {
+ return lastAttrs;
+ }
+ String cacheKey = topic + '|' + messageType;
+ Attributes attrs = topicAttributesCache.computeIfAbsent(cacheKey, k ->
+ newAttributesBuilder()
+ .put(LABEL_TOPIC_KEY, topic)
+ .put(LABEL_MESSAGE_TYPE_KEY, messageType)
+ .put(LABEL_IS_SYSTEM_KEY, isSystem)
+ .build()
+ );
+ this.lastTopicName = topic;
+ this.lastTopicMsgType = messageType;
+ this.lastTopicAttributes = attrs;
+ return attrs;
+ }
Review Comment:
The cache key and the last-value fast path ignore `isSystem`, even though
`LABEL_IS_SYSTEM_KEY` is included in the built Attributes. If `isSystem`
differs for the same `(topic, messageType)` across calls, this will return
Attributes with the wrong `isSystem` label value. Include `isSystem` in both
the fast-path comparison and the cache key (or maintain separate caches per
`isSystem`).
##########
broker/src/main/resources/rmq.broker.logback.xml:
##########
@@ -18,6 +18,8 @@
<configuration scan="true" scanPeriod="30 seconds">
+ <statusListener class="ch.qos.logback.core.status.NopStatusListener"/>
+
<appender name="DefaultSiftingAppender_inner"
class="ch.qos.logback.classic.sift.SiftingAppender">
Review Comment:
Adding `NopStatusListener` suppresses Logback status output, which can hide
configuration errors/warnings that are useful during troubleshooting
(especially when reloading with `scan=true`). Consider limiting this to
specific deployments/profiles, or using a status listener that can be
enabled/disabled via configuration rather than hard-disabling status reporting.
##########
common/src/main/java/org/apache/rocketmq/common/attribute/TopicMessageType.java:
##########
@@ -33,9 +33,11 @@ public enum TopicMessageType {
MIXED("MIXED");
private final String value;
+ private final String metricsValue;
TopicMessageType(String value) {
this.value = value;
+ this.metricsValue = value.toLowerCase();
}
Review Comment:
Using `String.toLowerCase()` without an explicit locale makes the metrics
label value depend on the JVM default locale (and this change also locks that
value at enum initialization time). To make metrics stable across environments,
use `toLowerCase(Locale.ROOT)` when computing `metricsValue`.
--
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]
