You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by revans2 <gi...@git.apache.org> on 2017/06/30 21:35:32 UTC
[GitHub] storm pull request #2184: STORM-2610: Fixed the metrics
GitHub user revans2 opened a pull request:
https://github.com/apache/storm/pull/2184
STORM-2610: Fixed the metrics
I still need to add these to the docs, but I wanted to capture my progress before the long weekend.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/revans2/incubator-storm STORM-2610
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2184.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 #2184
----
commit 5a23997720a0fec60f137222182fa43d848a1e78
Author: Robert (Bobby) Evans <ev...@yahoo-inc.com>
Date: 2017-06-30T21:34:01Z
STORM-2610: Fixed the metrics
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2184: STORM-2610: Fixed the metrics
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2184
@HeartSaVioR I filed STORM-2616 to document all of the built in metrics, and I'll try to get a pull request for that up some time soon.
For now I will leave this as a 2.x change, and if someone really wants/needs this for 1.x I can add the ms metrics and fix the count metrics.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2184: STORM-2610: Fixed the metrics
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2184#discussion_r125291429
--- Diff: storm-client/src/jvm/org/apache/storm/daemon/metrics/SpoutThrottlingMetrics.java ---
@@ -26,32 +27,20 @@
private final CountMetric skippedInactive = new CountMetric();
public SpoutThrottlingMetrics() {
- this.metricMap.put("skipped-max-spout", skippedMaxSpout);
- this.metricMap.put("skipped-throttle", skippedThrottle);
- this.metricMap.put("skipped-inactive", skippedInactive);
- }
-
- public CountMetric getSkippedMaxSpout() {
- return skippedMaxSpout;
- }
-
- public CountMetric getSkippedThrottle() {
- return skippedThrottle;
- }
-
- public CountMetric getSkippedInactive() {
- return skippedInactive;
+ metricMap.put("skipped-max-spout-ms", skippedMaxSpout);
+ metricMap.put("skipped-throttle-ms", skippedThrottle);
+ metricMap.put("skipped-inactive-ms", skippedInactive);
}
- public void skippedMaxSpout(CommonStats stats) {
- this.skippedMaxSpout.incrBy(stats.getRate());
+ public void skippedMaxSpoutMs(long ms) {
+ this.skippedMaxSpout.incrBy(ms);
--- End diff --
Why not also rename the fields (skippedMaxSpout, skippedThrottle, skippedInactive) to add `Ms` as postfix?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2184: STORM-2610: Fixed the metrics
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2184#discussion_r125291013
--- Diff: storm-client/src/jvm/org/apache/storm/utils/NimbusClient.java ---
@@ -96,6 +96,10 @@ public static NimbusClient getConfiguredClientAs(Map<String, Object> conf, Strin
if (override != null) {
return new NimbusClient(override);
}
+ Map<String, Object> fullConf = Utils.readStormConfig();
--- End diff --
I'm not sure this change is related to the origin issue. Could you explain this change?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2184: STORM-2610: Fixed the metrics
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2184#discussion_r125715307
--- Diff: storm-client/src/jvm/org/apache/storm/utils/NimbusClient.java ---
@@ -96,6 +96,10 @@ public static NimbusClient getConfiguredClientAs(Map<String, Object> conf, Strin
if (override != null) {
return new NimbusClient(override);
}
+ Map<String, Object> fullConf = Utils.readStormConfig();
--- End diff --
This is something I ran into trying to get the ThroughputVsLatency topology to run, so I could test the metrics. It simply means that if you pass in an empty config that it will fill in the rest from the command line, storm.yaml, and defaults.yaml, just like when submitting a topology.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2184: STORM-2610: Fixed the metrics
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2184
We didn't document individual builtin metric so maybe no need to document the change or document whole builtin metrics. It is breaking change in point of builtin metrics (for users who use metrics consumer) indeed.
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2184: STORM-2610: Fixed the metrics
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2184
It looks good overall. This change doesn't touch critical path so don't need to consider performance side.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2184: STORM-2610: Fixed the metrics
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2184
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2184: STORM-2610: Fixed the metrics
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2184#discussion_r125715402
--- Diff: storm-client/src/jvm/org/apache/storm/daemon/metrics/SpoutThrottlingMetrics.java ---
@@ -26,32 +27,20 @@
private final CountMetric skippedInactive = new CountMetric();
public SpoutThrottlingMetrics() {
- this.metricMap.put("skipped-max-spout", skippedMaxSpout);
- this.metricMap.put("skipped-throttle", skippedThrottle);
- this.metricMap.put("skipped-inactive", skippedInactive);
- }
-
- public CountMetric getSkippedMaxSpout() {
- return skippedMaxSpout;
- }
-
- public CountMetric getSkippedThrottle() {
- return skippedThrottle;
- }
-
- public CountMetric getSkippedInactive() {
- return skippedInactive;
+ metricMap.put("skipped-max-spout-ms", skippedMaxSpout);
+ metricMap.put("skipped-throttle-ms", skippedThrottle);
+ metricMap.put("skipped-inactive-ms", skippedInactive);
}
- public void skippedMaxSpout(CommonStats stats) {
- this.skippedMaxSpout.incrBy(stats.getRate());
+ public void skippedMaxSpoutMs(long ms) {
+ this.skippedMaxSpout.incrBy(ms);
--- End diff --
Good point will do that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---