Copilot commented on code in PR #10485:
URL: https://github.com/apache/rocketmq/pull/10485#discussion_r3394773018
##########
remoting/src/main/java/org/apache/rocketmq/remoting/metrics/RemotingMetricsManager.java:
##########
@@ -86,6 +96,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_SUCCESS.equals(result)) resultIdx = 0;
+ else if (RESULT_ONEWAY.equals(result)) resultIdx = 1;
+ else if (RESULT_WRITE_CHANNEL_FAILED.equals(result)) resultIdx = 2;
+ else if (RESULT_CANCELED.equals(result)) resultIdx = 3;
+ else resultIdx = -1;
+
+ if (resultIdx < 0) {
+ return buildAttributes(requestCode, responseCode, isLongPolling,
result);
+ }
+
+ long key = ((long) requestCode << 19)
+ | ((long) (responseCode & 0xFFFF) << 3)
+ | (isLongPolling ? 4L : 0L)
+ | resultIdx;
+ Attributes cached = this.lastCachedAttrs;
+ if (cached != null && key == this.lastAttrsCacheKey) {
+ return cached;
+ }
+ Attributes attrs = attributesCache.computeIfAbsent(key,
+ k -> buildAttributes(requestCode, responseCode, isLongPolling,
result));
+ this.lastAttrsCacheKey = key;
+ this.lastCachedAttrs = attrs;
+ return attrs;
Review Comment:
The last-attributes fast-path cache is published via two separate volatile
fields (`lastAttrsCacheKey` and `lastCachedAttrs`). This can return mismatched
`Attributes` for a key under races (e.g., reader sees updated key but still
reads the previous attrs and passes the equality check). Publish key+attrs as a
single immutable holder (like `CachedCounter` in
`RemotingCodeDistributionHandler`) via one volatile write, or switch to an
`AtomicReference` holding a pair.
##########
remoting/src/main/java/org/apache/rocketmq/remoting/metrics/RemotingMetricsManager.java:
##########
@@ -86,6 +96,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_SUCCESS.equals(result)) resultIdx = 0;
+ else if (RESULT_ONEWAY.equals(result)) resultIdx = 1;
+ else if (RESULT_WRITE_CHANNEL_FAILED.equals(result)) resultIdx = 2;
+ else if (RESULT_CANCELED.equals(result)) resultIdx = 3;
+ else resultIdx = -1;
+
+ if (resultIdx < 0) {
+ return buildAttributes(requestCode, responseCode, isLongPolling,
result);
+ }
+
+ long key = ((long) requestCode << 19)
+ | ((long) (responseCode & 0xFFFF) << 3)
+ | (isLongPolling ? 4L : 0L)
+ | resultIdx;
Review Comment:
Masking `responseCode` with `0xFFFF` discards higher bits and can cause
cache-key collisions if a response code is negative or exceeds 65535. If the
response code range is guaranteed to fit in 16 bits, please document that
assumption here; otherwise, remove the narrowing (or allocate more bits) so
distinct codes always map to distinct cache entries.
##########
broker/src/main/java/org/apache/rocketmq/broker/metrics/BrokerMetricsManager.java:
##########
@@ -195,12 +203,32 @@ public AttributesBuilder newAttributesBuilder() {
return attributesBuilder;
}
+ public Attributes getOrBuildTopicAttributes(String topic, String
messageType, boolean isSystem) {
+ Attributes lastAttrs = this.lastTopicAttributes;
+ if (lastAttrs != null && isSystem == this.lastTopicIsSystem &&
topic.equals(this.lastTopicName) && messageType.equals(this.lastTopicMsgType)) {
+ return lastAttrs;
+ }
+ String cacheKey = topic + '|' + messageType + '|' + isSystem;
+ Attributes attrs = topicAttributesCache.computeIfAbsent(cacheKey, k ->
Review Comment:
Using a string-concatenation cache key can collide if `topic` or
`messageType` contains the delimiter character (`|`). Prefer a dedicated
composite key type (e.g., a small record/class with `equals`/`hashCode`) to
avoid accidental collisions and make the cache semantics explicit.
##########
broker/src/main/java/org/apache/rocketmq/broker/metrics/BrokerMetricsManager.java:
##########
@@ -195,12 +203,32 @@ public AttributesBuilder newAttributesBuilder() {
return attributesBuilder;
}
+ public Attributes getOrBuildTopicAttributes(String topic, String
messageType, boolean isSystem) {
+ Attributes lastAttrs = this.lastTopicAttributes;
+ if (lastAttrs != null && isSystem == this.lastTopicIsSystem &&
topic.equals(this.lastTopicName) && messageType.equals(this.lastTopicMsgType)) {
+ return lastAttrs;
+ }
+ String cacheKey = topic + '|' + messageType + '|' + isSystem;
+ 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.lastTopicIsSystem = isSystem;
+ this.lastTopicAttributes = attrs;
+ return attrs;
+ }
Review Comment:
The topic fast-path cache is spread across multiple volatile fields
(`lastTopicName`, `lastTopicMsgType`, `lastTopicIsSystem`,
`lastTopicAttributes`). Under concurrent access, a thread can observe an
updated key triple but still see an older `lastTopicAttributes`, causing
incorrect attribute reuse. Use a single immutable holder object
(topic/messageType/isSystem/attrs) published via one volatile write to
guarantee consistency.
##########
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"/>
Review Comment:
Adding `NopStatusListener` suppresses Logback status messages, which can
hide important configuration errors/warnings during startup and reload
(especially with `scan=\"true\"`). Consider making this conditional (e.g., only
for specific deployments) or using a listener that still surfaces WARN/ERROR
status events to aid troubleshooting.
##########
broker/src/main/java/org/apache/rocketmq/broker/metrics/BrokerMetricsManager.java:
##########
@@ -195,12 +203,32 @@ public AttributesBuilder newAttributesBuilder() {
return attributesBuilder;
}
+ public Attributes getOrBuildTopicAttributes(String topic, String
messageType, boolean isSystem) {
+ Attributes lastAttrs = this.lastTopicAttributes;
+ if (lastAttrs != null && isSystem == this.lastTopicIsSystem &&
topic.equals(this.lastTopicName) && messageType.equals(this.lastTopicMsgType)) {
+ return lastAttrs;
+ }
Review Comment:
The topic fast-path cache is spread across multiple volatile fields
(`lastTopicName`, `lastTopicMsgType`, `lastTopicIsSystem`,
`lastTopicAttributes`). Under concurrent access, a thread can observe an
updated key triple but still see an older `lastTopicAttributes`, causing
incorrect attribute reuse. Use a single immutable holder object
(topic/messageType/isSystem/attrs) published via one volatile write to
guarantee consistency.
--
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]
