You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by zd-project <gi...@git.apache.org> on 2018/08/02 20:42:01 UTC
[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 <hu...@...>
Date: 2018-08-02T20:38:48Z
STORM-3173: Refactored API for PreparableReporter
commit afc31b62ed59c6b2e1a1c00af8d7600bc92247e9
Author: Zhengdai Hu <hu...@...>
Date: 2018-08-02T20:40:46Z
STORM-3173: Enable flushing when closing a ScheduledReporter
----
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Posted by Ethanlm <gi...@git.apache.org>.
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<JmxReporter> {
+public class JmxPreparableReporter implements PreparableReporter {
--- End diff --
why not extending `ScheduledReporter`
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Posted by srdo <gi...@git.apache.org>.
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<String, Object> topoConf) {
- for (PreparableReporter reporter : MetricsUtils.getPreparableReporters(topoConf)) {
+ public static AutoCloseable startMetricsReporters(Map<String, Object> 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 ...
Posted by srdo <gi...@git.apache.org>.
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<String, Object> topoConf) {
- for (PreparableReporter reporter : MetricsUtils.getPreparableReporters(topoConf)) {
+ public static AutoCloseable startMetricsReporters(Map<String, Object> 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 issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by zd-project <gi...@git.apache.org>.
Github user zd-project commented on the issue:
https://github.com/apache/storm/pull/2789
I can't see changes in your link, so I looked into your latest change in branch `non-static-metrics`. Please correct me if I interpret your code wrong.
Your implementation didn't actually flush the metrics for ScheduledReporter on exit. You want to add `reporter.report()` in ConsolePreparableReporter or CsvPreparableReporter and test it out. See my commit [STORM-3173: Addressing nits](https://github.com/apache/storm/pull/2789/commits/e5fb9d80f4af8fee2b3cff915c5401a1e0e0cde2#diff-65a7d8c48dacfbb072a99f50df9775c6R40)
---
[GitHub] storm issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by zd-project <gi...@git.apache.org>.
Github user zd-project commented on the issue:
https://github.com/apache/storm/pull/2789
#2764 also depends on resolution of this issue. Please consider merging that as well as part of the patch.
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Posted by srdo <gi...@git.apache.org>.
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 issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2789
It should. Take a look at https://github.com/srdo/storm/compare/892179527b5a...0916fae11cb2#diff-9c4945e8e924565ca1f071435f4711e9. If you compare the daemon classes (Nimbus, Supervisor etc.) there should be a call to stopMetricsReporters in all of them, that should stop the reporters on shutdown.
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Posted by srdo <gi...@git.apache.org>.
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 issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by zd-project <gi...@git.apache.org>.
Github user zd-project commented on the issue:
https://github.com/apache/storm/pull/2789
Build logs report test crash [INFO] Running org.apache.storm.scheduler.resource.TestResourceAwareScheduler, but it's passing on my local machine. Don't know what's happening. It'll be great if you guys can offer some insight. @Ethanlm @srdo @HeartSaVioR
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Posted by zd-project <gi...@git.apache.org>.
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 ...
Posted by Ethanlm <gi...@git.apache.org>.
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<ConsoleReporter> {
- private static final Logger LOG = LoggerFactory.getLogger(ConsolePreparableReporter.class);
- ConsoleReporter reporter = null;
+public class ConsolePreparableReporter extends ScheduledPreparableReporter<ConsoleReporter> {
@Override
public void prepare(MetricRegistry metricsRegistry, Map<String, Object> 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 issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by agresch <gi...@git.apache.org>.
Github user agresch commented on the issue:
https://github.com/apache/storm/pull/2789
what was the status of this? I think work @srdo was doing addressed STORM-3173 possibly? Was that change committed?
---
[GitHub] storm issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2789
Right, sorry, didn't recheck the logs.
---
[GitHub] storm issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by zd-project <gi...@git.apache.org>.
Github user zd-project commented on the issue:
https://github.com/apache/storm/pull/2789
The lastest failure didn't provide an error code though.
---
[GitHub] storm issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2789
Before spending too long on it, consider if we should fix this issue and https://github.com/apache/storm/pull/2714 by making the metrics registry non-static instead (https://github.com/apache/storm/pull/2783). I think it will be much easier to do.
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Posted by Ethanlm <gi...@git.apache.org>.
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<String, Object> 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 issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2789
sure. Take your time please.
---
[GitHub] storm issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2789
I've added calls to report to the branch, the tests are running at https://travis-ci.org/srdo/storm/builds/413332618.
Unless the test specifies another configuration file, I would expect us to be using https://github.com/apache/storm/blob/master/conf/defaults.yaml.
I also don't understand why heartbeatTimer would still be running in AsyncLocalizerTest. JUnit doesn't shut down the JVM between tests, so it would make sense that a thread could leak from another test into AsyncLocalizerTest, but it would require someone opening a Supervisor in a test somewhere and forgetting to close it.
Happy to help out.
---
[GitHub] storm issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by zd-project <gi...@git.apache.org>.
Github user zd-project commented on the issue:
https://github.com/apache/storm/pull/2789
Also, I just dug into the error code you suggested. It turned out to be caused by heartbeatTimer (instance of StormTimer) in Supervisor in AsyncLocalizerTest.testKeyNotFoundException. But I don't see where Supervisor is created. Super weird.
> 2018-08-07 16:38:11.217 [heartbeatTimer] ERROR org.apache.storm.daemon.supervisor.DefaultUncaughtExceptionHandler - Error when processing event on thread Thread[heartbeatTimer
,10,main]
...
> 2018-08-07 16:38:11.218 [heartbeatTimer] ERROR org.apache.storm.utils.Utils - val 202
2018-08-07 16:38:11.218 [heartbeatTimer] ERROR org.apache.storm.utils.Utils - Halting process: Error when processing an event
java.lang.RuntimeException: Halting process: Error when processing an event
at org.apache.storm.utils.Utils.exitProcess(Utils.java:474) [storm-client-2.0.0-SNAPSHOT.jar:?]
at org.apache.storm.daemon.supervisor.DefaultUncaughtExceptionHandler.uncaughtException(DefaultUncaughtExceptionHandler.java:25) [classes/:?]
at org.apache.storm.StormTimer$StormTimerTask.run(StormTimer.java:253) [storm-client-2.0.0-SNAPSHOT.jar:?]
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Posted by srdo <gi...@git.apache.org>.
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<T extends Reporter & Closeable> {
+public interface PreparableReporter {
void prepare(MetricRegistry metricsRegistry, Map<String, Object> topoConf);
void start();
void stop();
+ static <T, U extends ScheduledReporter> void startScheduledReporter(Class<T> 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<T extends Reporter> {
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 issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by zd-project <gi...@git.apache.org>.
Github user zd-project commented on the issue:
https://github.com/apache/storm/pull/2789
https://issues.apache.org/jira/browse/STORM-3128 This bug in test appeared again and broke the test in this PR but I don't actually know the cause of it.
---
[GitHub] storm issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2789
Once the two PRs with new metrics are merged, I'll try to update the non-static branch and get a PR opened. It might take me a little while.
---
[GitHub] storm issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2789
I am also getting ` org.apache.maven.surefire.booter.SurefireBooterForkException: Error occurred in starting fork, check output in log` on my VM. And the tests passed without this patch. Still need to investigate the issue
---
[GitHub] storm issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2789
The exit code is 20. Try looking for uses of `Utils.exitProcess` with `20` as the argument. If you change the values to be unique and rerun the tests, you should be able to nail down where the exit is happening.
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Posted by zd-project <gi...@git.apache.org>.
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<String, Object> topoConf) {
- for (PreparableReporter reporter : MetricsUtils.getPreparableReporters(topoConf)) {
+ public static AutoCloseable startMetricsReporters(Map<String, Object> topoConf) {
--- End diff --
Okay. Do we want this to be a generic utility interface or specific to reporters then?
---
[GitHub] storm issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2789
That's right. Will take a look at non-static registry after #2764 and #2764
---
[GitHub] storm issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by zd-project <gi...@git.apache.org>.
Github user zd-project commented on the issue:
https://github.com/apache/storm/pull/2789
Interesting. Does your implementation also guarantee flushing all metrics upon exit?
---
[GitHub] storm issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2789
No, but like I said, I suspect that the exit code is being set by a call to `Utils.exitProcess`. If you try changing some of the exit codes to be unique, it's likely you'll be able to tell which call to `Utils.exitProcess` is responsible for killing the JVM, which could help you tell what's happening.
---
[GitHub] storm issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by zd-project <gi...@git.apache.org>.
Github user zd-project commented on the issue:
https://github.com/apache/storm/pull/2789
Thanks again for helping me out of this issue though. Hope this don't create too much trouble for you.
---
[GitHub] storm issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by zd-project <gi...@git.apache.org>.
Github user zd-project commented on the issue:
https://github.com/apache/storm/pull/2789
I managed to fix the original timeout error by wrapping around a nimbus gauge with exception handling code. However the PR still failed the storm-server test due to error in starting fork. I don't really know what caused this. @srdo Have you seen anything like this before?
Additionally, I think our tests have some flaws in manipulating zookeeper as I constantly see a component tries to connect to Zookeeper that is not set up in the test, causing timeout warning in https://issues.apache.org/jira/browse/STORM-3128
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Posted by zd-project <gi...@git.apache.org>.
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<T extends Reporter & Closeable> {
+public interface PreparableReporter {
void prepare(MetricRegistry metricsRegistry, Map<String, Object> topoConf);
void start();
void stop();
+ static <T, U extends ScheduledReporter> void startScheduledReporter(Class<T> 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 ...
Posted by Ethanlm <gi...@git.apache.org>.
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 issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2789
The change is here https://github.com/apache/storm/pull/2805 and hasn't been merged yet.
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Posted by srdo <gi...@git.apache.org>.
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<String, Object> topoConf) {
- for (PreparableReporter reporter : MetricsUtils.getPreparableReporters(topoConf)) {
+ public static AutoCloseable startMetricsReporters(Map<String, Object> 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 issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2789
@zd-project I'm happy to take a look at implementing https://github.com/apache/storm/pull/2764 once the non-static PR is merged. I don't think it makes sense to mix more stuff into the current PR, and if it doesn't get approved, it'll be wasted work.
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Posted by zd-project <gi...@git.apache.org>.
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<String, Object> topoConf) {
- for (PreparableReporter reporter : MetricsUtils.getPreparableReporters(topoConf)) {
+ public static AutoCloseable startMetricsReporters(Map<String, Object> 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 issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by zd-project <gi...@git.apache.org>.
Github user zd-project commented on the issue:
https://github.com/apache/storm/pull/2789
Okay that alerted me one thing. What's our config file for the testing? If it doesn't use ScheduledReporter for testing, this PR shouldn't make a matter at all.
---
[GitHub] storm pull request #2789: STORM-3173: flush metrics to ScheduledReporter on ...
Posted by Ethanlm <gi...@git.apache.org>.
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<String, Object> 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 issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2789
Tested it manually on a single-node cluster. `nimbus:num-shutdown-calls` was flushed now with this patch. Still need to figure out travis build failure before merging this in.
---
[GitHub] storm issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2789
Unless it turns out to be easy to fix the test errors, I'd like to propose not merging this and fixing this issue as part of making the metrics registry non-static. I added reporter shutdowns to all the daemons on the non-static-metrics branch, and it works with no need for bandaid fixes https://travis-ci.org/srdo/storm/builds/413283316.
---