RongtongJin commented on code in PR #10485:
URL: https://github.com/apache/rocketmq/pull/10485#discussion_r3496680000
##########
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) {
Review Comment:
This cache is unsafe for a @Sharable Netty handler. `lastInCode` and
`lastInAdder` are published as two independent volatile fields, so another
EventLoop thread can observe the updated code before the corresponding adder is
visible, which may lead to a null dereference or counting a request under the
wrong adder. Please publish the cached pair atomically, for example with a
single volatile holder containing both `code` and `LongAdder`.
##########
broker/src/main/java/org/apache/rocketmq/broker/metrics/BrokerMetricsManager.java:
##########
@@ -195,6 +202,25 @@ 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;
Review Comment:
The cache key and the last-value fast path ignore `isSystem`, but the
generated Attributes include `LABEL_IS_SYSTEM_KEY`. If the same topic and
message type are observed with both system and non-system values, this can
reuse Attributes with the wrong label. Please include `isSystem` in both the
cache key and the fast-path comparison, or remove this helper if it is not
intended to be wired in this PR.
--
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]
