[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 issue #2775: MINOR - Make raw type assignment type safe
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2775 +1 ---
[GitHub] storm pull request #2773: Blobstore sync bug fix
Github user danny0405 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2773#discussion_r204992962
--- Diff:
storm-server/src/main/java/org/apache/storm/blobstore/LocalFsBlobStore.java ---
@@ -197,7 +198,7 @@ public void run() {
throw new RuntimeException(e);
}
}
-}, 0,
ObjectReader.getInt(conf.get(DaemonConfig.NIMBUS_CODE_SYNC_FREQ_SECS)));
+}, 0,
ObjectReader.getInt(conf.get(DaemonConfig.NIMBUS_CODE_SYNC_FREQ_SECS))*1000);
--- End diff --
Nice catch,
`ObjectReader.getInt(conf.get(DaemonConfig.NIMBUS_CODE_SYNC_FREQ_SECS)) * 1000`
only the missing whitespace.
---
