[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.
---
