Ethanlm commented on a change in pull request #3371:
URL: https://github.com/apache/storm/pull/3371#discussion_r556924977
##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/TaskMetricRepo.java
##########
@@ -34,23 +34,33 @@
private SortedMap<String, Timer> timers = new TreeMap<>();
public void addCounter(String name, Counter counter) {
- counters.put(name, counter);
+ synchronized (this) {
+ counters.put(name, counter);
+ }
}
public void addGauge(String name, Gauge gauge) {
- gauges.put(name, gauge);
+ synchronized (this) {
+ gauges.put(name, gauge);
+ }
}
public void addMeter(String name, Meter meter) {
- meters.put(name, meter);
+ synchronized (this) {
Review comment:
Maybe you explained this before. But why do they need to be `SortedMap`?
Can we use ConcurrentHashMap?
Do we need synchronization in the `report` method
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
