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 2015/06/10 22:26:44 UTC
[GitHub] storm pull request: STORM-862: Pluggable System Metrics
GitHub user revans2 opened a pull request:
https://github.com/apache/storm/pull/589
STORM-862: Pluggable System Metrics
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/revans2/incubator-storm STORM-862
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/589.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 #589
----
commit cdc9de4343ba4782dfea29495359b231dee73501
Author: Robert (Bobby) Evans <ev...@yahoo-inc.com>
Date: 2015-06-10T20:24:45Z
STORM-862: Pluggable System 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: STORM-862: Pluggable System Metrics
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/589#issuecomment-142009302
Now I can vote this PR. +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 pull request: STORM-862: Pluggable System 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/589#discussion_r32567646
--- Diff: storm-core/src/jvm/backtype/storm/Config.java ---
@@ -1087,6 +1087,17 @@
public static final String TOPOLOGY_METRICS_CONSUMER_REGISTER = "topology.metrics.consumer.register";
public static final Object TOPOLOGY_METRICS_CONSUMER_REGISTER_SCHEMA = ConfigValidation.MapsValidator;
+ /**
+ * A map of metric name to class name implementing IMetric that will be created once per worker JVM
+ */
+ public static final String TOPOLOGY_WORKER_METRICS = "topology.worker.metrics";
+ public static final Object TOPOLOGY_WORKER_METRICS_SCHEMA = Map.class;
+
+ /**
--- End diff --
I can see the benefit, thanks for the details. :)
---
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: STORM-862: Pluggable System 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/589#discussion_r32567961
--- Diff: storm-core/src/jvm/backtype/storm/Config.java ---
@@ -1087,6 +1087,17 @@
public static final String TOPOLOGY_METRICS_CONSUMER_REGISTER = "topology.metrics.consumer.register";
public static final Object TOPOLOGY_METRICS_CONSUMER_REGISTER_SCHEMA = ConfigValidation.MapsValidator;
+ /**
+ * A map of metric name to class name implementing IMetric that will be created once per worker JVM
+ */
+ public static final String TOPOLOGY_WORKER_METRICS = "topology.worker.metrics";
+ public static final Object TOPOLOGY_WORKER_METRICS_SCHEMA = Map.class;
+
+ /**
--- End diff --
It would be better to explain the intention from source code comment or configuration file, or doc, since users can't know that easily.
---
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: STORM-862: Pluggable System Metrics
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/589
---
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: STORM-862: Pluggable System 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/589#discussion_r32522459
--- Diff: storm-core/src/jvm/backtype/storm/Config.java ---
@@ -1087,6 +1087,17 @@
public static final String TOPOLOGY_METRICS_CONSUMER_REGISTER = "topology.metrics.consumer.register";
public static final Object TOPOLOGY_METRICS_CONSUMER_REGISTER_SCHEMA = ConfigValidation.MapsValidator;
+ /**
+ * A map of metric name to class name implementing IMetric that will be created once per worker JVM
+ */
+ public static final String TOPOLOGY_WORKER_METRICS = "topology.worker.metrics";
+ public static final Object TOPOLOGY_WORKER_METRICS_SCHEMA = Map.class;
+
+ /**
--- End diff --
They do provide the same thing, but the intended audience is different. Any config beginning with "topology." is intended for topology owners to set where as worker.metrics is intended for a cluster owner to set. That way I can force all topologies to collect a specific metric, but still allow end users to easily add in more of their own 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: STORM-862: Pluggable System 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/589#discussion_r32489327
--- Diff: storm-core/src/jvm/backtype/storm/Config.java ---
@@ -1087,6 +1087,17 @@
public static final String TOPOLOGY_METRICS_CONSUMER_REGISTER = "topology.metrics.consumer.register";
public static final Object TOPOLOGY_METRICS_CONSUMER_REGISTER_SCHEMA = ConfigValidation.MapsValidator;
+ /**
+ * A map of metric name to class name implementing IMetric that will be created once per worker JVM
+ */
+ public static final String TOPOLOGY_WORKER_METRICS = "topology.worker.metrics";
+ public static final Object TOPOLOGY_WORKER_METRICS_SCHEMA = Map.class;
+
+ /**
--- End diff --
Seems like TOPOLOGY_WORKER_METRICS and WORKER_METRICS provide the same things.
(Comment is also same.)
Are semantics of two configurations different?
---
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: STORM-862: Pluggable System Metrics
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/589#issuecomment-112561355
LGTM.
---
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.
---