[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/2771 ---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
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. ---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user Ethanlm commented on a diff in the pull request: https://github.com/apache/storm/pull/2771#discussion_r207665264 --- Diff: storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java --- @@ -50,19 +61,19 @@ public static void startMetricsReporters(Map topoConf) { } } -private static T register(final String name, T metric) { -T ret; +@Override +//This is more similar to super#getOrAdd than super#register +public T register(final String name, T metric) throws IllegalArgumentException { --- End diff -- It is fine. But I would like to see more javadoc for this method. ---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/2771#discussion_r207663344 --- Diff: storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java --- @@ -50,19 +61,19 @@ public static void startMetricsReporters(Map topoConf) { } } -private static T register(final String name, T metric) { -T ret; +@Override +//This is more similar to super#getOrAdd than super#register +public T register(final String name, T metric) throws IllegalArgumentException { --- End diff -- I am okay with it this way for now. ---
[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_r207640687 --- 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 -- Our customization upon MetricsRegistry is actually very similar to the wrapping methods of `#getOrAdd`, such as `gauge`, `timer`, `meter`, and `histogram`. They do not have a way to eliminate double registration of MetricSet though, although we could avoid this altogether with a non-static registry. In addition, I think we should probably upgrading to a newer version of Dropwizard, since current version (3.1.0) is about to be EOL. Their 4.x has a lot of improvement and provides more features on top of Java 8. ---
[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_r207638261 --- 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 -- I'll take a look at this ---
[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_r207636496 --- Diff: storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java --- @@ -50,19 +61,19 @@ public static void startMetricsReporters(Map topoConf) { } } -private static T register(final String name, T metric) { -T ret; +@Override +//This is more similar to super#getOrAdd than super#register +public T register(final String name, T metric) throws IllegalArgumentException { --- End diff -- I added a test for registerMetricSet and unregisterMetricSet in #2754 and #2764 to show that this method has solved the issue of double registration. See https://github.com/apache/storm/pull/2754/commits/597c6bb2d41a7aa0d25c6aab9201b451f6b1eaf1 I don't know if we should move the change to `registerMetricSet` and `unregisterMetricSet` back to this PR, but the lack of them seems to be confusing people of the purpose of this change. @Ethanlm @srdo @revans2 ---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2771#discussion_r207601720 --- 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 -- I think we can make the registry non-static without it being a huge hassle https://github.com/apache/storm/pull/2783. I'll rebase it once https://github.com/apache/storm/pull/2752 goes in, and hopefully we can use it to resolve https://github.com/apache/storm/pull/2714. Regarding the subclassing, I'm wondering if we should instead go ask the Dropwizard guys whether the inability to use `getOrAdd` for MetricSets is intended, and if not whether they'd be open to adding a method to MetricRegistry to allow it. If we're hitting this issue, other people probably are too, and I'd much rather fix it in the library than try to hack around it here. What do you think @zd-project? ---
[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_r207599113 --- 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 -- The reason I changed the `static register()` to instance `register()` method is stated here. Also I should point out that because StormMetricsRegistry is a static singleton, it will persist throughout each UnitTest. So if a UnitTest class has multiple tests in it, all metrics will actually be registered multiple times. We either have to change all unit tests to have set up and tear down, or we have to completely revamp the StormMetricsRegistry class to make it non-static. As I don't have that much time to do either of them, I came up with this walkaround. @srdo @revans2 @Ethanlm ---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user Ethanlm commented on a diff in the pull request: https://github.com/apache/storm/pull/2771#discussion_r207594931 --- Diff: storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java --- @@ -50,19 +61,19 @@ public static void startMetricsReporters(Map topoConf) { } } -private static T register(final String name, T metric) { -T ret; +@Override +//This is more similar to super#getOrAdd than super#register +public T register(final String name, T metric) throws IllegalArgumentException { --- End diff -- if the metric is MetricSet, it will be having the same issue with the old code. ---
[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 #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2771#discussion_r205046141 --- Diff: storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java --- @@ -88,19 +110,24 @@ public static void startMetricsReporters(Map topoConf) { } } -private static T register(final String name, T metric) { -T ret; -try { -ret = DEFAULT_REGISTRY.register(name, metric); -} catch (IllegalArgumentException e) { -// swallow IllegalArgumentException when the metric exists already -ret = (T) DEFAULT_REGISTRY.getMetrics().get(name); -if (ret == null) { -throw e; -} else { -LOG.warn("Metric {} has already been registered", name); -} +public static String name(String prefix, String name) { +assert name != null; +return StringUtils.isEmpty(prefix) ? name : prefix + ':' + name; +} + +public static String name(Class klass, String names) { +return name(klass.getName(), names); +} + +@Override +//This is more similar to super#getOrAdd than super#register --- End diff -- Yes, I think otherwise you'd still have to put the try-catch block around the `super.register` call. ---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2771#discussion_r205045839 --- Diff: storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java --- @@ -65,21 +67,41 @@ public static Meter registerMeter(final String name) { * @param type of value the gauge measures */ public static void registerProvidedGauge(final String name, final Gauge gauge) { -register(name, gauge); +DEFAULT_REGISTRY.register(name, gauge); +} + +public static Histogram registerHistogram(String name) { --- End diff -- Yes, I thought it was a little weird that their API also did that. It's fine, thanks for explaining. ---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2771#discussion_r205045449 --- 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 -- Ok. I really dislike the contortions we have to go through in order to make this work as a static singleton, but thanks for explaining. ---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2771#discussion_r205044870 --- Diff: storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java --- @@ -12,79 +12,122 @@ 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 com.codahale.metrics.Timer; + import java.util.Map; import java.util.concurrent.Callable; + +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 { -public static final MetricRegistry DEFAULT_REGISTRY = new MetricRegistry(); +public class StormMetricsRegistry extends MetricRegistry { +private static final StormMetricsRegistry DEFAULT_REGISTRY = new StormMetricsRegistry(); private static final Logger LOG = LoggerFactory.getLogger(StormMetricsRegistry.class); -public static Meter registerMeter(String name) { -Meter meter = new Meter(); -return register(name, meter); -} +private StormMetricsRegistry() {/*Singleton pattern*/} -// 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) { +return DEFAULT_REGISTRY.register(name, () -> { +try { +return fn.call(); +} catch (Exception e) { +LOG.error("Error getting gauge value for {}", name, e); } -}; -return register(name, gauge); +return null; +}); +} + +/** + * Register a provided gauge. Use this method if a custom gauge is needed or + * exceptions have already been 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) { +DEFAULT_REGISTRY.register(name, gauge); } -public static void registerProvidedGauge(final String name, Gauge gauge) { -register(name, gauge); +public static Histogram registerHistogram(String name) { +return registerHistogram(name, new ExponentiallyDecayingReservoir()); } public static Histogram registerHistogram(String name, Reservoir reservoir) { -Histogram histogram = new Histogram(reservoir); -return register(name, histogram); +return DEFAULT_REGISTRY.register(name, new Histogram(reservoir)); } +public static Meter registerMeter(String name) { +return DEFAULT_REGISTRY.register(name, new Meter()); +} + +//Change the name to avoid name conflict in future Metrics release --- End diff -- You are right, my bad. ---
[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_r204920511 --- Diff: storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java --- @@ -88,19 +110,24 @@ public static void startMetricsReporters(Map topoConf) { } } -private static T register(final String name, T metric) { -T ret; -try { -ret = DEFAULT_REGISTRY.register(name, metric); -} catch (IllegalArgumentException e) { -// swallow IllegalArgumentException when the metric exists already --- End diff -- DEFAULT_REGISTRY.getMetrics().get(name) won't properly capture registration of a MetricSet because under the hood MetricRegistry registers each metric in MetricSet with name as prefix, instead of the whole MetricSet with name. ---
[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_r204917201 --- Diff: storm-server/src/main/java/org/apache/storm/daemon/supervisor/Supervisor.java --- @@ -312,7 +315,7 @@ public void launchDaemon() { launch(); Utils.addShutdownHookWithForceKillIn1Sec(this::close); -registerWorkerNumGauge("supervisor:num-slots-used-gauge", conf); +registerGauge(name(SUPERVISOR, "num-slots-used-gauge"), () -> SupervisorUtils.supervisorWorkerIds(conf).size()); --- End diff -- Okay will refactor this. ---
[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_r204916859 --- Diff: storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java --- @@ -12,79 +12,122 @@ 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 com.codahale.metrics.Timer; + import java.util.Map; import java.util.concurrent.Callable; + +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 { -public static final MetricRegistry DEFAULT_REGISTRY = new MetricRegistry(); +public class StormMetricsRegistry extends MetricRegistry { +private static final StormMetricsRegistry DEFAULT_REGISTRY = new StormMetricsRegistry(); private static final Logger LOG = LoggerFactory.getLogger(StormMetricsRegistry.class); -public static Meter registerMeter(String name) { -Meter meter = new Meter(); -return register(name, meter); -} +private StormMetricsRegistry() {/*Singleton pattern*/} -// 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) { +return DEFAULT_REGISTRY.register(name, () -> { +try { +return fn.call(); +} catch (Exception e) { +LOG.error("Error getting gauge value for {}", name, e); } -}; -return register(name, gauge); +return null; +}); +} + +/** + * Register a provided gauge. Use this method if a custom gauge is needed or + * exceptions have already been 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) { +DEFAULT_REGISTRY.register(name, gauge); } -public static void registerProvidedGauge(final String name, Gauge gauge) { -register(name, gauge); +public static Histogram registerHistogram(String name) { +return registerHistogram(name, new ExponentiallyDecayingReservoir()); } public static Histogram registerHistogram(String name, Reservoir reservoir) { -Histogram histogram = new Histogram(reservoir); -return register(name, histogram); +return DEFAULT_REGISTRY.register(name, new Histogram(reservoir)); } +public static Meter registerMeter(String name) { +return DEFAULT_REGISTRY.register(name, new Meter()); +} + +//Change the name to avoid name conflict in future Metrics release --- End diff -- I believe in java static keyword doesn't affect function signature. Therefore you'd get error if you have a static method in child class with same signature as an instance method in parent class, saying "static method cannot override instance method". ---
[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_r204915515 --- Diff: storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java --- @@ -65,21 +67,41 @@ public static Meter registerMeter(final String name) { * @param type of value the gauge measures */ public static void registerProvidedGauge(final String name, final Gauge gauge) { -register(name, gauge); +DEFAULT_REGISTRY.register(name, gauge); +} + +public static Histogram registerHistogram(String name) { --- End diff -- I observed that most of the histogram we used has ExponentiallyDecayingReservoir as default, so I was thinking we have a default method. Notice that ExponentiallyDecayingReservoir is also the the default when invoking `MetricRegistry#histogram(String name)`. ---
[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_r204894886 --- 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 -- Though, after the change, client still cannot call `register` directly, because the singleton is private. ---
[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_r204891653 --- Diff: storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java --- @@ -88,19 +110,24 @@ public static void startMetricsReporters(Map topoConf) { } } -private static T register(final String name, T metric) { -T ret; -try { -ret = DEFAULT_REGISTRY.register(name, metric); -} catch (IllegalArgumentException e) { -// swallow IllegalArgumentException when the metric exists already -ret = (T) DEFAULT_REGISTRY.getMetrics().get(name); -if (ret == null) { -throw e; -} else { -LOG.warn("Metric {} has already been registered", name); -} +public static String name(String prefix, String name) { +assert name != null; +return StringUtils.isEmpty(prefix) ? name : prefix + ':' + name; +} + +public static String name(Class klass, String names) { +return name(klass.getName(), names); +} + +@Override +//This is more similar to super#getOrAdd than super#register --- End diff -- Would you recommend switch back to try-catch block then? ---
[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_r204890243 --- 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 -- I switched to inheritance because I need to override parent's `register()` method and swallows the exceptions, instead of provide a static `register()` method which delegate the registration. This is because dropwizard handles registering MetricSet differently under the hood and double registration can't be caught by current `static register()` method. ---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2771#discussion_r204860271 --- Diff: storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java --- @@ -65,21 +67,41 @@ public static Meter registerMeter(final String name) { * @param type of value the gauge measures */ public static void registerProvidedGauge(final String name, final Gauge gauge) { -register(name, gauge); +DEFAULT_REGISTRY.register(name, gauge); +} + +public static Histogram registerHistogram(String name) { --- End diff -- Nit: Might be helpful to add a note to the javadoc here that it uses this particular reservoir, or maybe rename the method to describe it, e.g. `registerFiveMinuteHistogram` or something like that. ---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2771#discussion_r204876125 --- Diff: storm-server/src/main/java/org/apache/storm/daemon/supervisor/Supervisor.java --- @@ -312,7 +315,7 @@ public void launchDaemon() { launch(); Utils.addShutdownHookWithForceKillIn1Sec(this::close); -registerWorkerNumGauge("supervisor:num-slots-used-gauge", conf); +registerGauge(name(SUPERVISOR, "num-slots-used-gauge"), () -> SupervisorUtils.supervisorWorkerIds(conf).size()); --- End diff -- This is personal preference, but I think static method imports can tend to make the code less readable. It's fine when the static import is something well-known like `assertThat` in a test, but having something like `registerGauge` show up like this makes it look like the method exists on this class. ---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2771#discussion_r204862238 --- Diff: storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java --- @@ -65,21 +67,41 @@ public static Meter registerMeter(final String name) { * @param type of value the gauge measures */ public static void registerProvidedGauge(final String name, final Gauge gauge) { -register(name, gauge); +DEFAULT_REGISTRY.register(name, gauge); +} + +public static Histogram registerHistogram(String name) { +return registerHistogram(name, new ExponentiallyDecayingReservoir()); } public static Histogram registerHistogram(String name, Reservoir reservoir) { -return register(name, new Histogram(reservoir)); +return DEFAULT_REGISTRY.register(name, new Histogram(reservoir)); +} + +public static Meter registerMeter(String name) { +return DEFAULT_REGISTRY.register(name, new Meter()); +} + +//Change the name to avoid name conflict in future Metrics release +public static void registerMetricSet(MetricSet metrics) { +DEFAULT_REGISTRY.registerAll(metrics); } -public static void registerAll(final String prefix, MetricSet metrics) { -register(prefix, metrics); +public static void unregisterMetricSet(com.codahale.metrics.MetricSet metrics) { +for (String s : metrics.getMetrics().keySet()) { +DEFAULT_REGISTRY.remove(s); +} } -public static void unregister(final String name) { -DEFAULT_REGISTRY.remove(name); +public static Timer registerTimer(String name) { +return DEFAULT_REGISTRY.register(name, new Timer()); } +/** + * Start metrics reporter with this metric registry. --- End diff -- Nit: It's not clear what `this` is here. Consider rewording to e.g. `Start metrics reporters for the registry singleton`. ---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2771#discussion_r204867270 --- Diff: storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java --- @@ -88,19 +110,24 @@ public static void startMetricsReporters(Map topoConf) { } } -private static T register(final String name, T metric) { -T ret; -try { -ret = DEFAULT_REGISTRY.register(name, metric); -} catch (IllegalArgumentException e) { -// swallow IllegalArgumentException when the metric exists already -ret = (T) DEFAULT_REGISTRY.getMetrics().get(name); -if (ret == null) { -throw e; -} else { -LOG.warn("Metric {} has already been registered", name); -} +public static String name(String prefix, String name) { +assert name != null; +return StringUtils.isEmpty(prefix) ? name : prefix + ':' + name; +} + +public static String name(Class klass, String names) { +return name(klass.getName(), names); +} + +@Override +//This is more similar to super#getOrAdd than super#register --- End diff -- I think there's an issue with concurrency here now, thread 1 can do the existence check for `metric1`, thread 2 registers `metric1`, and thread 1 then calls `super.register("metric1")` and gets an uncaught IllegalArgumentException. ---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2771#discussion_r204861682 --- Diff: storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java --- @@ -65,21 +67,41 @@ public static Meter registerMeter(final String name) { * @param type of value the gauge measures */ public static void registerProvidedGauge(final String name, final Gauge gauge) { -register(name, gauge); +DEFAULT_REGISTRY.register(name, gauge); +} + +public static Histogram registerHistogram(String name) { +return registerHistogram(name, new ExponentiallyDecayingReservoir()); } public static Histogram registerHistogram(String name, Reservoir reservoir) { -return register(name, new Histogram(reservoir)); +return DEFAULT_REGISTRY.register(name, new Histogram(reservoir)); +} + +public static Meter registerMeter(String name) { +return DEFAULT_REGISTRY.register(name, new Meter()); +} + +//Change the name to avoid name conflict in future Metrics release +public static void registerMetricSet(MetricSet metrics) { +DEFAULT_REGISTRY.registerAll(metrics); } -public static void registerAll(final String prefix, MetricSet metrics) { -register(prefix, metrics); +public static void unregisterMetricSet(com.codahale.metrics.MetricSet metrics) { --- End diff -- Fully qualified class name seems unnecessary. ---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2771#discussion_r204875066 --- Diff: storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java --- @@ -12,79 +12,122 @@ 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 com.codahale.metrics.Timer; + import java.util.Map; import java.util.concurrent.Callable; + +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 { -public static final MetricRegistry DEFAULT_REGISTRY = new MetricRegistry(); +public class StormMetricsRegistry extends MetricRegistry { +private static final StormMetricsRegistry DEFAULT_REGISTRY = new StormMetricsRegistry(); private static final Logger LOG = LoggerFactory.getLogger(StormMetricsRegistry.class); -public static Meter registerMeter(String name) { -Meter meter = new Meter(); -return register(name, meter); -} +private StormMetricsRegistry() {/*Singleton pattern*/} -// 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) { +return DEFAULT_REGISTRY.register(name, () -> { +try { +return fn.call(); +} catch (Exception e) { +LOG.error("Error getting gauge value for {}", name, e); } -}; -return register(name, gauge); +return null; +}); +} + +/** + * Register a provided gauge. Use this method if a custom gauge is needed or + * exceptions have already been 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) { +DEFAULT_REGISTRY.register(name, gauge); } -public static void registerProvidedGauge(final String name, Gauge gauge) { -register(name, gauge); +public static Histogram registerHistogram(String name) { +return registerHistogram(name, new ExponentiallyDecayingReservoir()); } public static Histogram registerHistogram(String name, Reservoir reservoir) { -Histogram histogram = new Histogram(reservoir); -return register(name, histogram); +return DEFAULT_REGISTRY.register(name, new Histogram(reservoir)); } +public static Meter registerMeter(String name) { +return DEFAULT_REGISTRY.register(name, new Meter()); +} + +//Change the name to avoid name conflict in future Metrics release --- End diff -- There couldn't be a name conflict, because this method is static, and the one on the super class isn't. ---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2771#discussion_r204862534 --- 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 { +private static final StormMetricsRegistry DEFAULT_REGISTRY = new StormMetricsRegistry(); --- End diff -- Nit: I don't think we allow any non-default registry in this class, so consider renaming to just `REGISTRY` ---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2771#discussion_r204858543 --- 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 -- Why is it better to extend MetricRegistry rather than wrap an instance? ---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
GitHub user zd-project opened a pull request: https://github.com/apache/storm/pull/2771 STORM-3157: General improvement to StormMetricsRegistry This contains and address the issues in #2763. This should help standardize the process to register metrics and MetricSet in the future. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zd-project/storm STORM-3157 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2771.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2771 commit 802014c88a67ae5ef044c1f87889d976063eee71 Author: Zhengdai Hu Date: 2018-07-12T16:31:00Z STORM-3150: Improve Gauge registration methods and refactored code commit 2cd815011cb0bb37c124d2360c3bcc757d926038 Author: Zhengdai Hu Date: 2018-07-19T15:46:29Z STORM-3150: Refactoring commit ff45063c7ce1516c2d887d64affea636e2f031a1 Author: Zhengdai Hu Date: 2018-07-20T18:41:51Z STORM-3157: Improve StormMetricsRegistry in general ---