STORM-3150: Improve Gauge registration methods and refactored code #2763

zd-project · 2018-07-13T19:43:25Z

STORM-3150

srdo · 2018-07-19T13:03:23Z

storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java

@@ -2809,21 +2809,19 @@ public void launchServer() throws Exception {
                                        }
                                    });

+            //Be cautious using method reference instead of lambda. subexpression preceding :: will be evaluated only upon evaluation


Nit: Not sure about this comment, it's not really specific to this code, but applies to any use of method references anywhere.

Sorry I was writing this to myself because I was not super familiar with Java lambda. Should probably delete it as it's not relevant to code.

srdo · 2018-07-19T13:04:44Z

storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java

-                                                                                                 .sum());
-            StormMetricsRegistry.registerGauge("nimbus:available-cpu", () -> nodeIdToResources.get().values().parallelStream().mapToDouble(
-                SupervisorResources::getAvailableCpu).sum());
+                .mapToDouble(SupervisorResources::getAvailableMem).sum());


Nit: I think the readability of method chaining is improved by having one call per line, e.g.

x.stream .map .collect

rather than

x.stream .map.collect

It's a super minor complaint though.

srdo · 2018-07-19T13:13:00Z

storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java

            }
-        };
-        return register(name, gauge);
+            return null;


Why does it make sense to return null rather than 0?

Because we are using generic type now. In addition reporters from metrics package seem to be handling null case gracefully so I think it's safe to use. That being said I should probably add this in the doc or notify people in some way if they want to inherit custom reporter.

srdo · 2018-07-19T13:23:54Z

storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java

-                }
-                return 0;
+    /**
+     * Register a gauge with provided callback.


Nit: Consider adding a note that this handles exceptions thrown from the callback.

Should I note so in the JavaDoc then?

Yes please :)

srdo · 2018-07-19T13:24:27Z

storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java

-    public static void registerProvidedGauge(final String name, Gauge gauge) {
+    /**
+     * Register a provided gauge. Use this method if custom gauges is needed or
+     * no checked exceptions should be handled.


Nit: Consider rephrasing as "or exceptions should not be handled"

srdo · 2018-07-19T13:26:51Z

storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java

+        return register(name, new Histogram(reservoir));
+    }
+
+    public static void registerAll(final String prefix, MetricSet metrics) {


Why is this method necessary?

This is used in #2764. Should move the change there?

Having it here is fine, but I'm not sure I understand what it does differently from just calling the register method directly? Is it just for clarity?

For wrapper to the private register() method. I don't see any problem if we should make register public though.

I think it's a little weird to have this and unregister be public, but not the generic register method.

I don't have a strong opinion on this, but making the register method public might make for a nicer API, especially since unregister can unregister any kind of metric.

I'll have to think about this a bit more, since I will have a few more PR depending on this. The way MetricRegistry handles MetricSet under the hood is a bit wacky, and they haven't supported removeAll(MetricSet set) method in their release yet. Do you have any advice of improving StormMetricsRegistry in general?

Improved in #2771

I like the idea of having registerMetricSet and unregisterMetricSet in #2771 better.

But to make the commit history easier to understand, I would suggest one of the following options:

put the changes to the patch where is using these methods

Port the changes in STORM-3157: General improvement to StormMetricsRegistry #2771 about registerMetricSet and unregisterMetricSet here. This is to avoid to change the same methods multiple times.

I prefer option 1.

Okay, should I first merge change in this PR into #2771 and then port them to where they are actually invoked?

srdo · 2018-07-24T17:55:13Z

+1, thanks for your patience. Please squash to one commit, and I'll merge.

zd-project · 2018-07-24T17:57:41Z

Okay thanks. Can you also help reviewing #2771? If you think merging two PR is better.

srdo · 2018-07-24T18:55:37Z

Yes, I'll take a look.

Ethanlm

Overall it looks good to me. Left some comment

revans2

Just a few nits.

revans2 · 2018-07-26T16:09:02Z

storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java

+     * @param <V> type of measurement the callback returns, also the type of gauge
+     * @return registered gauge
+     */
+    public static <V> Gauge<V> registerGauge(final String name, final Callable<V> fn) {


Could we just change this to take a guage instead?

public static <V> Gauge<V> registerGauge(final String name, final Gauge<V> gauge) { return register(name, gauge); }

We are not calling this from clojure anymore so we didn't need the Callable any more, and for the most part there are no code changes to make this happen.

Callable seems to be existed only for swallowing exceptions now. I don't know if this is part of the original design but I don't see a problem eliminating so.

Please do I don't see a reason to keep it here

revans2 · 2018-07-26T16:11:21Z

storm-server/src/main/java/org/apache/storm/scheduler/blacklist/BlacklistScheduler.java

-                return blacklistHost.size();
-            }
-        });
+        //nimbus:num-blacklisted-supervisor + none blacklisted supervisor = nimbus:num-supervisors


nit: I have no idea what this comment means, so could we remove it? Unless you understand it, then can we make it clearer?

It looks to me that there's a typo: nimbus:num-blacklisted-supervisor + non-blacklisted supervisor = nimbus:num-supervisors

zd-project · 2018-07-30T18:57:31Z

All nits are addressed and PR is rebased on current master branch. Do you want to merge this PR right now or merge this together with #2771?

srdo · 2018-07-31T18:45:02Z

@zd-project I think we can merge this immediately. Could you please squash back to one commit?

zd-project · 2018-08-02T14:55:17Z

@srdo I've squashed the commits. Sorry about the delay.

STORM-3150: Simplify gauge registration method.

srdo · 2018-08-02T17:45:06Z

@zd-project Merged, thanks for your patience. JIRA is down right now, will update the ticket when it comes back up.

zd-project mentioned this pull request Jul 13, 2018

STORM-3147: Port ClusterSummary as metrics to StormMetricsRegistry #2764

Closed

srdo reviewed Jul 19, 2018

View reviewed changes

zd-project mentioned this pull request Jul 20, 2018

STORM-3157: General improvement to StormMetricsRegistry #2771

Merged

Ethanlm reviewed Jul 26, 2018

View reviewed changes

revans2 reviewed Jul 26, 2018

View reviewed changes

zd-project force-pushed the STORM-3150 branch from 2cd8150 to 402a9b8 Compare July 30, 2018 18:56

zd-project force-pushed the STORM-3150 branch from 402a9b8 to a87156a Compare July 30, 2018 19:04

zd-project force-pushed the STORM-3150 branch from a87156a to 065b72c Compare August 2, 2018 14:54

STORM-3150: Improve Gauge registration methods and refactored code

4d43947

STORM-3150: Simplify gauge registration method.

zd-project force-pushed the STORM-3150 branch from 065b72c to 4d43947 Compare August 2, 2018 14:57

asfgit merged commit 4d43947 into apache:master Aug 2, 2018

zd-project deleted the STORM-3150 branch August 2, 2018 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STORM-3150: Improve Gauge registration methods and refactored code #2763

STORM-3150: Improve Gauge registration methods and refactored code #2763

zd-project commented Jul 13, 2018

srdo Jul 19, 2018

zd-project Jul 19, 2018

srdo Jul 19, 2018

srdo Jul 19, 2018

zd-project Jul 19, 2018

srdo Jul 19, 2018

zd-project Jul 19, 2018

srdo Jul 19, 2018

srdo Jul 19, 2018

srdo Jul 19, 2018

zd-project Jul 19, 2018

srdo Jul 19, 2018

zd-project Jul 19, 2018

srdo Jul 19, 2018

srdo Jul 19, 2018

zd-project Jul 19, 2018

zd-project Jul 20, 2018

Ethanlm Jul 26, 2018

zd-project Jul 27, 2018

srdo commented Jul 24, 2018

zd-project commented Jul 24, 2018

srdo commented Jul 24, 2018

Ethanlm left a comment

revans2 left a comment

revans2 Jul 26, 2018

zd-project Jul 27, 2018 •

edited

Loading

revans2 Jul 30, 2018

revans2 Jul 26, 2018

zd-project Jul 27, 2018

zd-project commented Jul 30, 2018

srdo commented Jul 31, 2018

zd-project commented Aug 2, 2018

srdo commented Aug 2, 2018

STORM-3150: Improve Gauge registration methods and refactored code #2763

STORM-3150: Improve Gauge registration methods and refactored code #2763

Conversation

zd-project commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdo commented Jul 24, 2018

zd-project commented Jul 24, 2018

srdo commented Jul 24, 2018

Ethanlm left a comment

Choose a reason for hiding this comment

revans2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zd-project Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zd-project commented Jul 30, 2018

srdo commented Jul 31, 2018

zd-project commented Aug 2, 2018

srdo commented Aug 2, 2018

zd-project Jul 27, 2018 •

edited

Loading