[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user zd-project commented on a diff in the pull request: https://github.com/apache/storm/pull/2771#discussion_r205520696 --- 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, that's why I opened #2714 to discuss about improvement. ---
[GitHub] storm pull request #2763: STORM-3150: Improve Gauge registration methods and...
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/2763#discussion_r205515487 --- Diff: storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java --- @@ -27,52 +28,67 @@ @SuppressWarnings("unchecked") public class StormMetricsRegistry { -public static final MetricRegistry DEFAULT_REGISTRY = new MetricRegistry(); +private static final MetricRegistry DEFAULT_REGISTRY = new MetricRegistry(); private static final Logger LOG = LoggerFactory.getLogger(StormMetricsRegistry.class); -public static Meter registerMeter(String name) { -Meter meter = new Meter(); -return register(name, meter); +public static Meter registerMeter(final String name) { +return register(name, new Meter()); } -// TODO: should replace Callable to Gauge when nimbus.clj is translated to java -public static Gauge registerGauge(final String name, final Callable fn) { -Gauge gauge = new Gauge() { -@Override -public Integer getValue() { -try { -return (Integer) fn.call(); -} catch (Exception e) { -LOG.error("Error getting gauge value for {}", name, e); -} -return 0; +/** + * Register a gauge with provided callback. This swallows all exceptions + * thrown from the callback, consider using {@link #registerProvidedGauge(String, Gauge)} + * if no exceptions will be thrown by the callable. + * + * @param name name of the gauge + * @param fn callback that measures + * @param type of measurement the callback returns, also the type of gauge + * @return registered gauge + */ +public static Gauge registerGauge(final String name, final Callable fn) { --- End diff -- Could we just change this to take a guage instead? ``` public static Gauge registerGauge(final String name, final Gauge gauge) { return register(name, gauge); } ``` We are not calling this from clojure anymore so we didn't need the Callable any more, and for the most part there are no code changes to make this happen. ---
[GitHub] storm pull request #2763: STORM-3150: Improve Gauge registration methods and...
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/2763#discussion_r205516198 --- Diff: storm-server/src/main/java/org/apache/storm/scheduler/blacklist/BlacklistScheduler.java --- @@ -89,13 +89,8 @@ public void prepare(Map conf) { cachedSupervisors = new HashMap<>(); blacklistHost = new HashSet<>(); - StormMetricsRegistry.registerGauge("nimbus:num-blacklisted-supervisor", new Callable() { -@Override -public Integer call() throws Exception { -//nimbus:num-blacklisted-supervisor + none blacklisted supervisor = nimbus:num-supervisors -return blacklistHost.size(); -} -}); +//nimbus:num-blacklisted-supervisor + none blacklisted supervisor = nimbus:num-supervisors --- End diff -- nit: I have no idea what this comment means, so could we remove it? Unless you understand it, then can we make it clearer? ---
[GitHub] storm pull request #2763: STORM-3150: Improve Gauge registration methods and...
Github user Ethanlm commented on a diff in the pull request: https://github.com/apache/storm/pull/2763#discussion_r205500685 --- Diff: storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java --- @@ -27,52 +28,63 @@ @SuppressWarnings("unchecked") public class StormMetricsRegistry { -public static final MetricRegistry DEFAULT_REGISTRY = new MetricRegistry(); +private static final MetricRegistry DEFAULT_REGISTRY = new MetricRegistry(); private static final Logger LOG = LoggerFactory.getLogger(StormMetricsRegistry.class); -public static Meter registerMeter(String name) { -Meter meter = new Meter(); -return register(name, meter); +public static Meter registerMeter(final String name) { +return register(name, new Meter()); } -// TODO: should replace Callable to Gauge when nimbus.clj is translated to java -public static Gauge registerGauge(final String name, final Callable fn) { -Gauge gauge = new Gauge() { -@Override -public Integer getValue() { -try { -return (Integer) fn.call(); -} catch (Exception e) { -LOG.error("Error getting gauge value for {}", name, e); -} -return 0; +/** + * Register a gauge with provided callback. + * @param name name of the gauge + * @param fn callback that measures + * @param type of measurement the callback returns, also the type of gauge + * @return registered gauge + */ +public static Gauge registerGauge(final String name, final Callable fn) { +return register(name, () -> { +try { +return fn.call(); +} catch (Exception e) { +LOG.error("Error getting gauge value for {}", name, e); } -}; -return register(name, gauge); +return null; +}); } -public static void registerProvidedGauge(final String name, Gauge gauge) { +/** + * Register a provided gauge. Use this method if custom gauges is needed or + * no checked exceptions should be handled. + * @param name name of the gauge + * @param gauge gauge + * @param type of value the gauge measures + */ +public static void registerProvidedGauge(final String name, final Gauge gauge) { register(name, gauge); } public static Histogram registerHistogram(String name, Reservoir reservoir) { -Histogram histogram = new Histogram(reservoir); -return register(name, histogram); +return register(name, new Histogram(reservoir)); +} + +public static void registerAll(final String prefix, MetricSet metrics) { --- End diff -- I like the idea of having `registerMetricSet` and `unregisterMetricSet` in #2771 better. But to make the commit history easier to understand, I would suggest one of the following options: 1. put the changes to the patch where is using these methods 2. Port the changes in #2771 about `registerMetricSet` and `unregisterMetricSet` here. This is to avoid to change the same methods multiple times. I prefer option 1. ---