[GitHub] storm issue #2794: STORM-3180 Total executors in Cluster Summary in main UI ...

2018-08-06 Thread srdo
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 ...

2018-08-06 Thread HeartSaVioR
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...

2018-08-06 Thread govind-menon
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...

2018-08-06 Thread govind-menon
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 ...

2018-08-06 Thread srdo
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 ...

2018-08-06 Thread zd-project
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 ...

2018-08-06 Thread srdo
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 ...

2018-08-06 Thread zd-project
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 ...

2018-08-06 Thread zd-project
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...

2018-08-06 Thread srdo
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 ...

2018-08-06 Thread srdo
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 ...

2018-08-06 Thread srdo
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 ...

2018-08-06 Thread srdo
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...

2018-08-06 Thread zd-project
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...

2018-08-06 Thread zd-project
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...

2018-08-06 Thread zd-project
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 ...

2018-08-06 Thread srdo
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...

2018-08-06 Thread srdo
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

2018-08-06 Thread srdo
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...

2018-08-06 Thread zd-project
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...

2018-08-06 Thread asfgit
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...

2018-08-06 Thread srdo
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...

2018-08-06 Thread srdo
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...

2018-08-06 Thread srdo
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

2018-08-06 Thread zd-project
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...

2018-08-06 Thread zd-project
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.

2018-08-06 Thread asfgit
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...

2018-08-06 Thread HeartSaVioR
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.

2018-08-06 Thread dfdemar
Github user dfdemar commented on the issue:

https://github.com/apache/storm/pull/2790
  
@srdo Ready to merge!


---