wang-jiahua commented on code in PR #10539:
URL: https://github.com/apache/rocketmq/pull/10539#discussion_r3878054740
##########
remoting/src/main/java/org/apache/rocketmq/remoting/metrics/RemotingMetricsManager.java:
##########
@@ -87,6 +95,37 @@ 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;
+ return attributesCache.computeIfAbsent(key,
+ k -> buildAttributes(requestCode, responseCode, isLongPolling,
result));
Review Comment:
Good catch — negative response codes would collide after masking, and a
negative requestCode would sign-extend over the whole key. Fixed in 4832ff0c5:
the cache is now only used when both codes fit in 0..0xFFFF (and the result is
known); anything else falls back to building attributes directly, so distinct
inputs can never collide.
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingAbstract.java:
##########
@@ -235,19 +234,14 @@ public static void writeResponse(Channel channel,
RemotingCommand request, @Null
if (response == null) {
return;
}
- final AttributesBuilder attributesBuilder;
- if (remotingMetricsManager != null) {
- attributesBuilder = remotingMetricsManager.newAttributesBuilder();
- attributesBuilder.put(LABEL_IS_LONG_POLLING, request.isSuspended())
- .put(LABEL_REQUEST_CODE,
RemotingHelper.getRequestCodeDesc(request.getCode()))
- .put(LABEL_RESPONSE_CODE,
RemotingHelper.getResponseCodeDesc(response.getCode()));
- } else {
- attributesBuilder = null;
- }
+ final int requestCode = request.getCode();
+ final int responseCode = response.getCode();
+ final boolean isLongPolling = request.isSuspended();
if (request.isOnewayRPC()) {
- if (attributesBuilder != null) {
- attributesBuilder.put(LABEL_RESULT, RESULT_ONEWAY);
-
remotingMetricsManager.getRpcLatency().record(request.getProcessTimer().elapsed(TimeUnit.MILLISECONDS),
attributesBuilder.build());
+ if (remotingMetricsManager != null) {
+ Attributes attrs = remotingMetricsManager.getOrBuildAttributes(
+ requestCode, responseCode, isLongPolling, RESULT_ONEWAY);
+
remotingMetricsManager.getRpcLatency().record(request.getProcessTimer().elapsed(TimeUnit.MILLISECONDS),
attrs);
Review Comment:
The code is correct as-is:
`getProcessTimer().elapsed(TimeUnit.MILLISECONDS)` is allocation-free and not
deprecated, so a separate `processTimerElapsedMs()` accessor would add API
surface for no gain. The issue text was stale (it described an earlier plan) —
I've updated #10537 to drop that sentence.
--
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]
