You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by Ethanlm <gi...@git.apache.org> on 2017/09/15 15:09:31 UTC
[GitHub] storm pull request #2326: [STORM-2741] Add in config options for metrics con...
GitHub user Ethanlm opened a pull request:
https://github.com/apache/storm/pull/2326
[STORM-2741] Add in config options for metrics consumer cpu and memory
[jira-STORM-2741](https://issues.apache.org/jira/browse/STORM-2741)
Similar to https://github.com/apache/storm/pull/2324 , add cpu and memory configs for metrics consumer.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Ethanlm/storm STORM-2741
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2326.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 #2326
----
commit 4a29b57a1c58cc6fda54356ae5568843d1abbb3c
Author: Ethan Li <et...@gmail.com>
Date: 2017-09-15T14:45:34Z
[STORM-2741] Add in config options for metrics consumer cpu and memory
----
---
[GitHub] storm pull request #2326: [STORM-2741] Add in config options for metrics con...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2326#discussion_r139429698
--- Diff: storm-client/src/jvm/org/apache/storm/Constants.java ---
@@ -30,7 +30,7 @@
public static final List<Long> SYSTEM_EXECUTOR_ID = Arrays.asList(-1L, -1L);
public static final String SYSTEM_COMPONENT_ID = "__system";
public static final String SYSTEM_TICK_STREAM_ID = "__tick";
- public static final String METRICS_COMPONENT_ID_PREFIX = "__metrics";
+ public static final String METRICS_COMPONENT_ID_PREFIX = "__metrics_";
--- End diff --
That is fine, I think this is a really good fix. I am just saying that it might make it difficult for someone to find say the LoggingMetricsConsumer in an automated way, and so if we do make this change in 1.x we should do it thoughtfully.
---
[GitHub] storm pull request #2326: [STORM-2741] Add in config options for metrics con...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2326#discussion_r139427249
--- Diff: storm-client/src/jvm/org/apache/storm/Constants.java ---
@@ -30,7 +30,7 @@
public static final List<Long> SYSTEM_EXECUTOR_ID = Arrays.asList(-1L, -1L);
public static final String SYSTEM_COMPONENT_ID = "__system";
public static final String SYSTEM_TICK_STREAM_ID = "__tick";
- public static final String METRICS_COMPONENT_ID_PREFIX = "__metrics";
+ public static final String METRICS_COMPONENT_ID_PREFIX = "__metrics_";
--- End diff --
Yea... But I made this change because https://github.com/apache/storm/blob/master/docs/Metrics.md says,
```
The parallelism for each Bolt is set to parallelism.hint and component id for that Bolt is set to __metrics_<metrics consumer class name>
```
---
[GitHub] storm issue #2326: [STORM-2741] Add in config options for metrics consumer c...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2326
A test:
Config:
```
topology.metrics.consumer.register:
- class: "org.apache.storm.metric.LoggingMetricsConsumer"
parallelism.hint: 1
- class: "org.apache.storm.metric.LoggingMetricsConsumer"
parallelism.hint: 1
topology.metrics.consumer.resources.onheap.memory.mb: 100
topology.component.resources.onheap.memory.mb: 128
```
---
[GitHub] storm pull request #2326: [STORM-2741] Add in config options for metrics con...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2326#discussion_r139424403
--- Diff: storm-client/src/jvm/org/apache/storm/Constants.java ---
@@ -30,7 +30,7 @@
public static final List<Long> SYSTEM_EXECUTOR_ID = Arrays.asList(-1L, -1L);
public static final String SYSTEM_COMPONENT_ID = "__system";
public static final String SYSTEM_TICK_STREAM_ID = "__tick";
- public static final String METRICS_COMPONENT_ID_PREFIX = "__metrics";
+ public static final String METRICS_COMPONENT_ID_PREFIX = "__metrics_";
--- End diff --
This is a minor change, but it might impact some people. We probably want to just put this into 2.x for now. If we really want it in 1.x we can decide how much of an impact it is (probably minor, but I want to see what others thing about it).
---
[GitHub] storm pull request #2326: [STORM-2741] Add in config options for metrics con...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2326
---
[GitHub] storm pull request #2326: [STORM-2741] Add in config options for metrics con...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2326#discussion_r139430356
--- Diff: storm-client/src/jvm/org/apache/storm/Constants.java ---
@@ -30,7 +30,7 @@
public static final List<Long> SYSTEM_EXECUTOR_ID = Arrays.asList(-1L, -1L);
public static final String SYSTEM_COMPONENT_ID = "__system";
public static final String SYSTEM_TICK_STREAM_ID = "__tick";
- public static final String METRICS_COMPONENT_ID_PREFIX = "__metrics";
+ public static final String METRICS_COMPONENT_ID_PREFIX = "__metrics_";
--- End diff --
Yes sure. Thanks for the review.
---