[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 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! ---
