RATIS-2405. Remove duplicate computeIfAbsent call in MessageMetrics.inc() method. #1346
Conversation
| } | ||
|
|
||
| private void inc(String metricNamePrefix, Type t) { | ||
| types.get(t) |
There was a problem hiding this comment.
The types.get(t) returns a ConcurrentHashMap, and computeIfAbsent itself is thread-safe. It guarantees that under concurrent access, only one LongCounter instance will be created and returned (the lambda function may be invoked multiple times, but only one value will be stored). The original synchronized block redundantly duplicated the responsibility of computeIfAbsent and also caused double counting. Now, keeping only one computeIfAbsent(...).inc() call ensures both thread safety and avoids duplicate counting.
|
@szetszwo Could you help review this PR? Thank you so much! |
| final LongCounter counter = types.get(t) | ||
| .computeIfAbsent(metricNamePrefix, prefix -> getRegistry().counter(prefix + t.getSuffix())); | ||
| counter.inc(); |
There was a problem hiding this comment.
@slfan1989 , thanks a lot for working on this!
You are right that the second computeIfAbsent is useless. The change looks good. Could you keep the first three lines unchanged as below?
private void inc(String metricNamePrefix, Type t) {
types.get(t)
.computeIfAbsent(metricNamePrefix, prefix -> getRegistry().counter(prefix + t.getSuffix()))
.inc();
}There was a problem hiding this comment.
Thank you for reviewing the code! I’ve improved it based on your feedback.
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
+1 the change looks good.
@szetszwo Thank you so much for reviewing the code! |
2 participants
What changes were proposed in this pull request?
Problem
The
MessageMetrics.inc()method contains duplicate code that calls computeIfAbsent twice for the same key, along with an unnecessarysynchronizedblock.This leads to:
What is the link to the Apache JIRA
RATIS-2405. Remove duplicate computeIfAbsent call in MessageMetrics.inc() method.
How was this patch tested?
Updated
TestGrpcMessageMetricsto verify the counter increment behavior more reliably by comparing before/after values instead of relying on absolute counts.