[GitHub] storm issue #2794: STORM-3180 Total executors in Cluster Summary in main UI ...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2794 Okay, just wanted to be sure it wasn't an oversight. +1. ---
[GitHub] storm issue #2794: STORM-3180 Total executors in Cluster Summary in main UI ...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2794 @srdo No, unfortunately we don't apply consistent naming so `totalExecutors` is right for others. Renaming them would break clients which rely on the REST API, so I'm not sure we would be better to fix them though I think it is ideal to fix. ---
[GitHub] storm issue #2795: STORM-3182: Removing superfluous topo id check from owner...
Github user govind-menon commented on the issue: https://github.com/apache/storm/pull/2795 @HeartSaVioR Fix for 3182. Working on the others. ---
[GitHub] storm pull request #2795: STORM-3182: Removing superfluous topo id check fro...
GitHub user govind-menon opened a pull request: https://github.com/apache/storm/pull/2795 STORM-3182: Removing superfluous topo id check from owner resource AP⦠â¦I request You can merge this pull request into a Git repository by running: $ git pull https://github.com/govind-menon/storm STORM-3182 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2795.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 #2795 commit ecec9cf52e61ffd889e51309b1bc2c1df99d7366 Author: Govind Menon Date: 2018-08-06T20:44:57Z STORM-3182: Removing superfluous topo id check from owner resource API request ---
[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 issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
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 ...
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 issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
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 issue #2789: STORM-3173: flush metrics to ScheduledReporter on shutdow...
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 #2764: STORM-3147: Port ClusterSummary as metrics to StormMetric...
Github user zd-project commented on the issue: https://github.com/apache/storm/pull/2764 This can be merged after #2789 ---
[GitHub] storm issue #2794: STORM-3180 Total executors in Cluster Summary in main UI ...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2794 Grepping for "totalExecutors" also pops up a couple of other locations in storm-webapp. ``` ./storm-webapp/src/main/java/org/apache/storm/daemon/ui/UIHelpers.java:536: int totalExecutors = ./storm-webapp/src/main/java/org/apache/storm/daemon/ui/UIHelpers.java:574: result.put("totalExecutors", totalExecutors); ./storm-webapp/src/main/java/org/apache/storm/daemon/ui/UIHelpers.java:631: result.put("totalExecutors", ownerResourceSummary.get_total_executors()); ./storm-webapp/src/main/java/org/apache/storm/daemon/ui/WEB-INF/js/script.js:541: data: 'totalExecutors', ./storm-webapp/src/main/java/org/apache/storm/daemon/ui/WEB-INF/owner.html:148: //owner,totalTopologies,totalTasks,totalExecutors,totalWorkers ./storm-webapp/src/main/java/org/apache/storm/daemon/ui/WEB-INF/templates/owner-page-template.html:51: {{totalExecutors}} ``` Do any of the others also need to be changed? ---
[GitHub] storm issue #2783: [WIP] Make StormMetricsRegistry a regular class rather th...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2783 Thanks ---
[GitHub] storm issue #2771: STORM-3157: General improvement to StormMetricsRegistry
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2771 +1, though maybe some of this can be made unnecessary in the future if we decide to make StormMetricsRegistry non-static. Thanks for your patience @zd-project, merged to master. ---
[GitHub] storm issue #2783: [WIP] Make StormMetricsRegistry a regular class rather th...
Github user zd-project commented on the issue: https://github.com/apache/storm/pull/2783 I'm really sorry about the timing. I'll try to take a look at it once the merge is done. ---
[GitHub] storm pull request #2771: STORM-3157: General improvement to StormMetricsReg...
Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/2771 ---
[GitHub] storm pull request #2783: [WIP] Make StormMetricsRegistry a regular class ra...
Github user srdo closed the pull request at: https://github.com/apache/storm/pull/2783 ---
[GitHub] storm issue #2783: [WIP] Make StormMetricsRegistry a regular class rather th...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2783 I'll close this in the meantime. ---
[GitHub] storm issue #2783: [WIP] Make StormMetricsRegistry a regular class rather th...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2783 @zd-project Alright, maybe the best option is that we postpone looking at this until the added metrics have been merged. ---
[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 Squashed. ---
[GitHub] storm issue #2783: [WIP] Make StormMetricsRegistry a regular class rather th...
Github user zd-project commented on the issue: https://github.com/apache/storm/pull/2783 I can port changes on top of this and open another PR, but I hope existing PRs can be merged in first, as they provide actual metrics that are straight useful to system administrators and developers. While this is more of a improvement to internals. Additionally, my internship ends this week and I will go back to school by the end of August, so though best I try, I can't guarantee to contribute regularly thereafter. We should plan this accordingly. ---
[GitHub] storm pull request #2790: STORM-3175 - Allow usage of custom Callback.
Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/2790 ---
[GitHub] storm pull request #2794: STORM-3180 Total executors in Cluster Summary in m...
GitHub user HeartSaVioR opened a pull request: https://github.com/apache/storm/pull/2794 STORM-3180 Total executors in Cluster Summary in main UI page is not ⦠â¦exposed even a topology is running * rename the field of /cluster/summary output: `totalExecutors` to `executorsTotal` Please compare with doc regarding the output format of API: https://github.com/apache/storm/blob/master/docs/STORM-UI-REST-API.md#apiv1clustersummary-get You can merge this pull request into a Git repository by running: $ git pull https://github.com/HeartSaVioR/storm STORM-3180 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2794.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 #2794 commit fdfeb14166301fed82b045f41934b693e95577c4 Author: Jungtaek Lim Date: 2018-08-06T13:07:53Z STORM-3180 Total executors in Cluster Summary in main UI page is not exposed even a topology is running * rename the field of /cluster/summary output: totalExecutors to executorsTotal ---
[GitHub] storm issue #2790: STORM-3175 - Allow usage of custom Callback.
Github user dfdemar commented on the issue: https://github.com/apache/storm/pull/2790 @srdo Ready to merge! ---