[GitHub] storm pull request #2774: Bug fix for BaseResourceAwareStrategy to avoid Nul...
Github user danny0405 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2774#discussion_r204983665
--- Diff:
storm-server/src/main/java/org/apache/storm/scheduler/resource/strategies/scheduling/BaseResourceAwareStrategy.java
---
@@ -357,9 +357,12 @@ private AllResources createClusterAllResources() {
ObjectResources rack = new ObjectResources(rackId);
racks.add(rack);
for (String nodeHost : nodeHosts) {
-for (RAS_Node node : hostnameToNodes(nodeHost)) {
-
rack.availableResources.add(node.getTotalAvailableResources());
-
rack.totalResources.add(node.getTotalAvailableResources());
+List nodes = hostnameToNodes(nodeHost);
+if(nodes != null) {
+for (RAS_Node node : nodes) {
--- End diff --
Oh, my fault, the nodes may be null. +1 BTW.
---
[GitHub] storm pull request #2774: Bug fix for BaseResourceAwareStrategy to avoid Nul...
Github user danny0405 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2774#discussion_r204981713
--- Diff:
storm-server/src/main/java/org/apache/storm/scheduler/resource/strategies/scheduling/BaseResourceAwareStrategy.java
---
@@ -357,9 +357,12 @@ private AllResources createClusterAllResources() {
ObjectResources rack = new ObjectResources(rackId);
racks.add(rack);
for (String nodeHost : nodeHosts) {
-for (RAS_Node node : hostnameToNodes(nodeHost)) {
-
rack.availableResources.add(node.getTotalAvailableResources());
-
rack.totalResources.add(node.getTotalAvailableResources());
+List nodes = hostnameToNodes(nodeHost);
+if(nodes != null) {
+for (RAS_Node node : nodes) {
--- End diff --
we initialize the nodes with new HashMap<>(), so it will never be a null,
and this fix is not necessary
---
[GitHub] storm issue #2774: Bug fix for BaseResourceAwareStrategy to avoid NullPointe...
Github user danny0405 commented on the issue: https://github.com/apache/storm/pull/2774 Sorry, we initialize the nodes with new HashMap<>(), so it will never be a null, and this fix is not necessary ---
[GitHub] storm pull request #2775: MINOR - Make raw type assignment type safe
GitHub user hmcl opened a pull request: https://github.com/apache/storm/pull/2775 MINOR - Make raw type assignment type safe You can merge this pull request into a Git repository by running: $ git pull https://github.com/hmcl/storm-apache master_skc_RawTypeAssignSafe Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2775.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 #2775 commit 5084c9756a4d792c4ac3a2a64e0d0ec2da8530e5 Author: Hugo Louro Date: 2018-07-25T01:42:23Z MINOR - Make raw type assignment type safe ---
[GitHub] storm issue #2743: [STORM-3130]: Add Wrappers for Timer registration and tim...
Github user zd-project commented on the issue: https://github.com/apache/storm/pull/2743 You should be able to see the usage in [here](https://github.com/apache/storm/pull/2754/files#diff-9c4945e8e924565ca1f071435f4711e9R3711), [here](https://github.com/apache/storm/pull/2710/commits/73b934420c2fd7ad441d3c0467376871d915b230) and [here](https://github.com/apache/storm/pull/2710/commits/778f30711af35732a5a13e1317dda6df905133c4) ---
[GitHub] storm pull request #2770: Trivial refactoring on DRPC
Github user zd-project closed the pull request at: https://github.com/apache/storm/pull/2770 ---
[GitHub] storm issue #2770: Trivial refactoring on DRPC
Github user zd-project commented on the issue: https://github.com/apache/storm/pull/2770 Sure, will merge into another issue. ---
[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 issue #2771: STORM-3157: General improvement to StormMetricsRegistry
Github user zd-project commented on the issue: https://github.com/apache/storm/pull/2771 Still much work here is for better accommodating MetricSet. dropwizard handles MetricSet a bit fuzzy but it's useful for organizing metrics (e.g., this will affect #2764). I'm also starting to seeing values in centralized all metrics for a certain daemon because it's easier to maintain and we would avoid the issue of #2714. I'll soon put up another PR about adding metrics for exceptions in LogViewer, which exemplifies how centralized MetricSet can work better. ---
[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 issue #2743: [STORM-3130]: Add Wrappers for Timer registration and tim...
Github user zd-project commented on the issue: https://github.com/apache/storm/pull/2743 stopTiming is only be called once (manually or via `#close()`) so far. I think for extensibility we could have another implementation, say `ConcurrentTimed implements TimerDecorated`. I just don't see the need right 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_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 #2769: STORM-3159: Fixed a potential file resource leak.
Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/2769 ---
[GitHub] storm issue #2770: Trivial refactoring on DRPC
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2770 The change looks fine to me, but I'm not sure why we want to change it. If you want it to go in, please raise an issue for it and put the issue number in the commit message. ---
[GitHub] storm issue #2743: [STORM-3130]: Add Wrappers for Timer registration and tim...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2743 @zd-project In your last commit, you removed the `AtomicReference` wrapper for the `Timer.Context`. Is it because you expect the `Timer.Context` to only be used by one thread? ---
[GitHub] storm pull request #2743: [STORM-3130]: Add Wrappers for Timer registration ...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2743#discussion_r204882262
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/nimbus/TimedWritableByteChannel.java
---
@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license agreements.
+ * See the NOTICE file distributed with this work for additional
information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
(the "License");
+ * you may not use this file except in compliance with the License. You
may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an "AS IS"
BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
limitations under the License.
+ */
+
+package org.apache.storm.daemon.nimbus;
+
+import com.codahale.metrics.Timer;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.channels.WritableByteChannel;
+
+import org.apache.storm.metric.timed.TimedResource;
+
+public class TimedWritableByteChannel extends
TimedResource implements WritableByteChannel {
+
+public TimedWritableByteChannel(WritableByteChannel measured, Timer
timer) {
+super(measured, timer);
+}
+
+@Override
+public int write(ByteBuffer src) throws IOException {
+return getMeasured().write(src);
+}
+
+@Override
+public boolean isOpen() {
+return getMeasured().isOpen();
+}
+
+@Override
+public void close() throws IOException {
+try {
+super.close();
+} catch (Exception e) {
+//WritableByteChannel is a Channel which implements Closeable.
+// Hence although declared AutoCloseable super#close here
should only throws IOException
+//We rethrow to conform the signature
+throw (IOException) e;
--- End diff --
Just as a follow up to our earlier discussion here, I think you can avoid
the downcast by making TimedResource implement both AutoCloseable and
Closeable. Unless we have a class in mind that only implements AutoCloseable
(which seems rare), this shouldn't be a constraint that affects us.
---
[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_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_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 issue #2763: STORM-3150: Improve Gauge registration methods and refact...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2763 Yes, I'll take a look. ---
[GitHub] storm issue #2769: Fixed a potential file resource leak.
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2769 +1. Please raise an issue for this, and squash to one commit that contains the issue number. ---
[GitHub] storm issue #2763: STORM-3150: Improve Gauge registration methods and refact...
Github user zd-project commented on the issue: https://github.com/apache/storm/pull/2763 Okay thanks. Can you also help reviewing #2771? If you think merging two PR is better. ---
[GitHub] storm issue #2763: STORM-3150: Improve Gauge registration methods and refact...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2763 +1, thanks for your patience. Please squash to one commit, and I'll merge. ---
[GitHub] storm pull request #2769: Fixed a potential file resource leak.
Github user agresch commented on a diff in the pull request:
https://github.com/apache/storm/pull/2769#discussion_r204755463
--- Diff:
storm-server/src/main/java/org/apache/storm/utils/ServerUtils.java ---
@@ -635,16 +635,16 @@ public static void unZip(File inFile, File toDir)
throws IOException {
*
* @throws IOException
*/
+//Used only by logviewer, considering moving to web-app?
--- End diff --
even if used only in one place, if it is a general purpose function, I
think it makes sense to have in a global place. I'd remove this comment.
---
[GitHub] storm pull request #2774: Bug fix for BaseResourceAwareStrategy to avoid Nul...
GitHub user jiangzhileaf opened a pull request: https://github.com/apache/storm/pull/2774 Bug fix for BaseResourceAwareStrategy to avoid NullPointerException Bug fix for BaseResourceAwareStrategy to avoid NullPointerException, when a supervisor is in bad list but alive. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jiangzhileaf/storm resource-scheduler-bug-fix Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2774.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 #2774 commit 605144c3051f17923c9da97519b08a8d1eac9378 Author: jiangzhileaf Date: 2018-07-19T08:40:49Z Bug fix for BaseResourceAwareStrategy to avoid NullPointerException, when a supervisor is in badlist but alive. ---
[GitHub] storm pull request #2773: Blobstore sync bug fix
GitHub user jiangzhileaf opened a pull request: https://github.com/apache/storm/pull/2773 Blobstore sync bug fix 1.Bug fix for blob sync frequency with time unit error. 2.Bug fix for blob sync delete file, add catch NoSuchFileException. 3.Bug fix for blob sync update blob flie, add catch FileNotFoundException You can merge this pull request into a Git repository by running: $ git pull https://github.com/jiangzhileaf/storm blobstore-sync-bug-fix Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2773.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 #2773 commit b2d7f31de318109e708eda060d664acb184b31e7 Author: jiangzhileaf Date: 2018-07-19T08:37:22Z Bug fix for blobstore sync. commit 2b75ade3a6789e755945bbdb89c3ee708b5f30ea Author: jiangzhileaf Date: 2018-07-24T03:18:31Z Specially handle for FileNotFoundException, because it take time for leader nimbus to copy the jar to blobstore commit 849c74198774e95f7aaa1b89a96a8e28012d6119 Author: jiangzhileaf Date: 2018-07-24T07:45:46Z fix blob sync frequency. ---
