Github user srdo commented on a diff in the pull request:

    https://github.com/apache/storm/pull/2771#discussion_r207672905
  
    --- Diff: 
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
    @@ -12,28 +12,30 @@
     
     package org.apache.storm.metric;
     
    +import com.codahale.metrics.ExponentiallyDecayingReservoir;
     import com.codahale.metrics.Gauge;
     import com.codahale.metrics.Histogram;
     import com.codahale.metrics.Meter;
     import com.codahale.metrics.Metric;
     import com.codahale.metrics.MetricRegistry;
     import com.codahale.metrics.MetricSet;
     import com.codahale.metrics.Reservoir;
    +
     import java.util.Map;
     import java.util.concurrent.Callable;
    +
    +import com.codahale.metrics.Timer;
    +import org.apache.commons.lang.StringUtils;
     import org.apache.storm.daemon.metrics.MetricsUtils;
     import org.apache.storm.daemon.metrics.reporters.PreparableReporter;
     import org.slf4j.Logger;
     import org.slf4j.LoggerFactory;
     
    -@SuppressWarnings("unchecked")
    -public class StormMetricsRegistry {
    -    private static final MetricRegistry DEFAULT_REGISTRY = new 
MetricRegistry();
    +public class StormMetricsRegistry extends MetricRegistry {
    --- End diff --
    
    Yes, I agree that for the non-MetricSet metrics, we can just use the 
getOrAdd wrappers. If we don't need MetricSet with a non-static registry, we 
should be good if we merge the changes in 
https://github.com/apache/storm/pull/2783. 
    
    I agree that we should upgrade, but versions past 4.x have removed the 
metrics-ganglia module. I'm not sure if it's been spun off somewhere, or if 
it's just been deleted, but I didn't want to start removing stuff related to 
Ganglia in https://github.com/apache/storm/pull/2783 as well. If we want to 
upgrade past 3.1 I think we should do it in a separate PR.


---

Reply via email to