[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2789#discussion_r209274375
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -96,4 +99,10 @@ public static void startMetricsReporters(Map topoConf) {
throw e;
}
}
+
+@FunctionalInterface
+public interface Session extends AutoCloseable {
--- End diff --
Do we need this? We can have `startMetricsReporters` returns
preparableReporters. It will be more clear.
But if you prefer to keep this, you might want to pick a better name
because this is way to general
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2789#discussion_r209269207
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/metrics/reporters/ConsolePreparableReporter.java
---
@@ -18,16 +18,12 @@
import java.util.Map;
import java.util.concurrent.TimeUnit;
import org.apache.storm.daemon.metrics.ClientMetricsUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-public class ConsolePreparableReporter implements
PreparableReporter {
-private static final Logger LOG =
LoggerFactory.getLogger(ConsolePreparableReporter.class);
-ConsoleReporter reporter = null;
+public class ConsolePreparableReporter extends
ScheduledPreparableReporter {
@Override
public void prepare(MetricRegistry metricsRegistry, Map topoConf) {
-LOG.debug("Preparing...");
+log.debug("Preparing...");
--- End diff --
`LOG` is used everywhere. I think it's better to use `LOG` here too
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2789#discussion_r209274273
--- Diff:
storm-server/src/main/java/org/apache/storm/pacemaker/Pacemaker.java ---
@@ -49,7 +49,7 @@ public Pacemaker(Map conf) {
heartbeats = new ConcurrentHashMap<>();
this.conf = conf;
StormMetricsRegistry.registerGauge("pacemaker:size-total-keys",
heartbeats::size);
-StormMetricsRegistry.startMetricsReporters(conf);
+
Utils.addShutdownHookWithForceKillIn1Sec(StormMetricsRegistry.startMetricsReporters(conf)::close);
--- End diff --
better to break this into two lines
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2789#discussion_r209301643
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/supervisor/Supervisor.java
---
@@ -316,7 +317,7 @@ public void launchDaemon() {
//This will only get updated once
StormMetricsRegistry.registerMeter("supervisor:num-launched").mark();
StormMetricsRegistry.registerMeter("supervisor:num-shell-exceptions",
ShellUtils.numShellExceptions);
-StormMetricsRegistry.startMetricsReporters(conf);
+metricsReporters =
StormMetricsRegistry.startMetricsReporters(conf);
--- End diff --
since you are not really returning metricsReporters here, better not to use
this name `metricsReporters`
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2789#discussion_r209271682
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/metrics/reporters/JmxPreparableReporter.java
---
@@ -22,9 +22,9 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-public class JmxPreparableReporter implements
PreparableReporter {
+public class JmxPreparableReporter implements PreparableReporter {
--- End diff --
why not extending `ScheduledReporter`
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2789#discussion_r208353770
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java ---
@@ -2807,7 +2808,14 @@ public void launchServer() throws Exception {
}
});
-StormMetricsRegistry.registerGauge("nimbus:num-supervisors",
() -> state.supervisors(null).size());
+StormMetricsRegistry.registerGauge("nimbus:num-supervisors",
() -> {
+try {
+return state.supervisors(null).size();
+} catch (Exception e) {
+e.printStackTrace();
--- End diff --
It's a band-aid fix for now to unblock flushing. I should probably redirect
e.printStackTrace() to LOG.error. To really fix the issue, we either have to
unregister this gauge manually, which is not ideal, or come up a way to notify
Nimbus of the shutdown of Zookeeper. This again relates back to the issue that
I brought up earlier about the connection refused exception from Nimbus to
Zookeeper, which I don't think have a good fix so far.
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2789#discussion_r208350201
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java ---
@@ -2807,7 +2808,14 @@ public void launchServer() throws Exception {
}
});
-StormMetricsRegistry.registerGauge("nimbus:num-supervisors",
() -> state.supervisors(null).size());
+StormMetricsRegistry.registerGauge("nimbus:num-supervisors",
() -> {
+try {
+return state.supervisors(null).size();
+} catch (Exception e) {
+e.printStackTrace();
--- End diff --
I don't think this is a reasonable fix, it's just hiding the error.
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2789#discussion_r208014755
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -53,12 +55,14 @@ public static Meter registerMeter(String name) {
*
* @param topoConf config that specifies reporter plugin
*/
-public static void startMetricsReporters(Map topoConf)
{
-for (PreparableReporter reporter :
MetricsUtils.getPreparableReporters(topoConf)) {
+public static AutoCloseable startMetricsReporters(Map
topoConf) {
--- End diff --
I'd probably just declare it in this file, we can always move it later if
we need to. If we make the registry non-static at some point, we probably won't
need it anymore, since we can just add a close method to the registry instead.
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2789#discussion_r208012336
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -53,12 +55,14 @@ public static Meter registerMeter(String name) {
*
* @param topoConf config that specifies reporter plugin
*/
-public static void startMetricsReporters(Map topoConf)
{
-for (PreparableReporter reporter :
MetricsUtils.getPreparableReporters(topoConf)) {
+public static AutoCloseable startMetricsReporters(Map
topoConf) {
--- End diff --
Okay. Do we want this to be a generic utility interface or specific to
reporters then?
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2789#discussion_r208010887
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -53,12 +55,14 @@ public static Meter registerMeter(String name) {
*
* @param topoConf config that specifies reporter plugin
*/
-public static void startMetricsReporters(Map topoConf)
{
-for (PreparableReporter reporter :
MetricsUtils.getPreparableReporters(topoConf)) {
+public static AutoCloseable startMetricsReporters(Map
topoConf) {
--- End diff --
No, I meant declare a new interface that extends AutoCloseable but doesn't
throw Exception
```
interface NotThrowingAutoCloseable extends AutoCloseable {
void close();
}
```
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2789#discussion_r208010012
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -53,12 +55,14 @@ public static Meter registerMeter(String name) {
*
* @param topoConf config that specifies reporter plugin
*/
-public static void startMetricsReporters(Map topoConf)
{
-for (PreparableReporter reporter :
MetricsUtils.getPreparableReporters(topoConf)) {
+public static AutoCloseable startMetricsReporters(Map
topoConf) {
--- End diff --
I guess we can use Runnable instead. But it doesn't look as semantically
correct as Autocloseable here (as we're closing the reporters)
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2789#discussion_r208008824
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/metrics/reporters/PreparableReporter.java
---
@@ -13,16 +13,35 @@
package org.apache.storm.daemon.metrics.reporters;
import com.codahale.metrics.MetricRegistry;
-import com.codahale.metrics.Reporter;
-import java.io.Closeable;
import java.util.Map;
+import java.util.concurrent.TimeUnit;
+import com.codahale.metrics.ScheduledReporter;
+import org.slf4j.Logger;
-public interface PreparableReporter {
+public interface PreparableReporter {
void prepare(MetricRegistry metricsRegistry, Map
topoConf);
void start();
void stop();
+static void
startScheduledReporter(Class enclosingClazz, U reporter, final Logger log) {
--- End diff --
Okay. I guess I'll just revert to the original implementation then, the
alternative seems to complicate code even more.
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2789#discussion_r208007711
--- Diff:
storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java ---
@@ -53,12 +55,14 @@ public static Meter registerMeter(String name) {
*
* @param topoConf config that specifies reporter plugin
*/
-public static void startMetricsReporters(Map topoConf)
{
-for (PreparableReporter reporter :
MetricsUtils.getPreparableReporters(topoConf)) {
+public static AutoCloseable startMetricsReporters(Map
topoConf) {
--- End diff --
I think it would be better to use another interface that extends
AutoCloseable so you don't have to deal with the non-existing Exception
everywhere.
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2789#discussion_r208005815
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/supervisor/Supervisor.java
---
@@ -446,6 +447,7 @@ public void
sendSupervisorAssignments(SupervisorAssignments assignments) {
public void close() {
try {
LOG.info("Shutting down supervisor {}", getId());
+metricsReporters.close();
--- End diff --
Nit: This should probably have a null check, since metricsReporters isn't
guaranteed to be non-null.
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2789#discussion_r208005061
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/metrics/reporters/PreparableReporter.java
---
@@ -13,16 +13,35 @@
package org.apache.storm.daemon.metrics.reporters;
import com.codahale.metrics.MetricRegistry;
-import com.codahale.metrics.Reporter;
-import java.io.Closeable;
import java.util.Map;
+import java.util.concurrent.TimeUnit;
+import com.codahale.metrics.ScheduledReporter;
+import org.slf4j.Logger;
-public interface PreparableReporter {
+public interface PreparableReporter {
void prepare(MetricRegistry metricsRegistry, Map
topoConf);
void start();
void stop();
+static void
startScheduledReporter(Class enclosingClazz, U reporter, final Logger log) {
--- End diff --
I don't think there's a good reason to have these static methods. If you
want to deduplicate the methods in the implementations, it would probably be
better to do as a collaborator object. If you make a new class that contains
the functionality from these two methods, you can avoid exposing these methods
on the interface, and likely get a nicer method signature as well.
What I mean is something like
```
class ReporterStarter {
private final T reporter;
public void startReporter() {
the implementation of startScheduledReporter goes here
}
}
```
and then you just make the PreparableReporter instances use instances of
that class.
On the other hand, I'd also be okay with not worrying about deduplicating
the methods, it's a very slight amount of code duplication, and I'm not sure
the extra abstraction helps readability.
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
GitHub user zd-project opened a pull request: https://github.com/apache/storm/pull/2789 STORM-3173: flush metrics to ScheduledReporter on shutdown https://issues.apache.org/jira/browse/STORM-3173 You can merge this pull request into a Git repository by running: $ git pull https://github.com/zd-project/storm STORM-3173 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2789.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 #2789 commit b2ec79c4dcb5beb60f9d5988cbf46431c451734d Author: Zhengdai Hu Date: 2018-08-02T20:38:48Z STORM-3173: Refactored API for PreparableReporter commit afc31b62ed59c6b2e1a1c00af8d7600bc92247e9 Author: Zhengdai Hu Date: 2018-08-02T20:40:46Z STORM-3173: Enable flushing when closing a ScheduledReporter ---
