wang-jiahua commented on code in PR #10485:
URL: https://github.com/apache/rocketmq/pull/10485#discussion_r3400421009
##########
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:
Good catch on the theoretical collision. In practice, RocketMQ topic names
and `TopicMessageType` enum values (`NORMAL`, `TRANSACTION`, `DELAY`, `FIFO`,
`MIXED`) never contain `|`. The cache key format is stable and bounded. A
composite key class would add an allocation per cache miss (the very thing
we're trying to avoid). If we ever introduce topic names with `|` (unlikely
given naming conventions), we can switch to a different separator or composite
key.
##########
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:
RocketMQ response codes are defined in `ResponseCode.java` and
`RemotingSysResponseCode.java` — all in the range 0–200. The `& 0xFFFF` mask is
a defensive guard against unexpected negative values from custom extensions,
ensuring the packed long key stays well-formed. No known code path produces a
response code > 65535.
##########
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:
Noted. The `NopStatusListener` addition is actually part of the parent PR
#10443 (AttributeKey static caching) — it appears in this PR's diff because
this branch is stacked on #10443. After #10443 is merged and this PR is
rebased, the logback change will disappear from this diff. That said, the
rationale: the default status listener logs 20+ lines of Logback internal INFO
during every broker startup, adding noise. Important configuration errors still
surface through application logs; `NopStatusListener` only suppresses the
Logback-internal status output.
--
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]
