You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by kishorvpatil <gi...@git.apache.org> on 2018/11/06 21:59:14 UTC
[GitHub] storm pull request #2902: [STORM-3282] Fix RAS worker count estimation
GitHub user kishorvpatil opened a pull request:
https://github.com/apache/storm/pull/2902
[STORM-3282] Fix RAS worker count estimation
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kishorvpatil/incubator-storm storm3282
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2902.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 #2902
----
commit 8288d41b313126ce7a9da7b548801a58da98b7ae
Author: Kishor Patil <kp...@...>
Date: 2018-11-06T21:58:31Z
Fix RAS worker count estimation
----
---
[GitHub] storm issue #2902: [STORM-3282] Fix RAS worker count estimation
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the issue:
https://github.com/apache/storm/pull/2902
The jenkins failure seems unrelated.
---
[GitHub] storm pull request #2902: [STORM-3282] Fix RAS worker count estimation
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2902#discussion_r231581587
--- Diff: storm-server/src/main/java/org/apache/storm/utils/ServerUtils.java ---
@@ -691,8 +691,10 @@ public static boolean isRAS(Map<String, Object> conf) {
public static int getEstimatedWorkerCountForRASTopo(Map<String, Object> topoConf, StormTopology topology)
throws InvalidTopologyException {
- return (int) Math.ceil(getEstimatedTotalHeapMemoryRequiredByTopo(topoConf, topology) /
- ObjectReader.getDouble(topoConf.get(Config.WORKER_HEAP_MEMORY_MB)));
+ Double defaultWorkerMaxHeap = ObjectReader.getDouble(topoConf.get(Config.WORKER_HEAP_MEMORY_MB));
+ Double topologyWorkerMaxHeap = ObjectReader.getDouble(topoConf.get(Config.TOPOLOGY_WORKER_MAX_HEAP_SIZE_MB));
--- End diff --
Oh. ok. If there is a default value in yaml file, then that's fine. I think we don't want to have to maintain two default values both in default.yaml and also in the code.
---
[GitHub] storm pull request #2902: [STORM-3282] Fix RAS worker count estimation
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/2902#discussion_r231580524
--- Diff: storm-server/src/main/java/org/apache/storm/utils/ServerUtils.java ---
@@ -691,8 +691,10 @@ public static boolean isRAS(Map<String, Object> conf) {
public static int getEstimatedWorkerCountForRASTopo(Map<String, Object> topoConf, StormTopology topology)
throws InvalidTopologyException {
- return (int) Math.ceil(getEstimatedTotalHeapMemoryRequiredByTopo(topoConf, topology) /
- ObjectReader.getDouble(topoConf.get(Config.WORKER_HEAP_MEMORY_MB)));
+ Double defaultWorkerMaxHeap = ObjectReader.getDouble(topoConf.get(Config.WORKER_HEAP_MEMORY_MB));
+ Double topologyWorkerMaxHeap = ObjectReader.getDouble(topoConf.get(Config.TOPOLOGY_WORKER_MAX_HEAP_SIZE_MB));
--- End diff --
There is default of this value - https://github.com/apache/storm/blob/master/conf/defaults.yaml#L332 and https://github.com/apache/storm/blob/master/conf/defaults.yaml#L332 avoids it. But we can
So I don't anticipate that happening, but I am adding defaults.
---
[GitHub] storm pull request #2902: [STORM-3282] Fix RAS worker count estimation
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2902#discussion_r231542667
--- Diff: storm-server/src/main/java/org/apache/storm/utils/ServerUtils.java ---
@@ -691,8 +691,10 @@ public static boolean isRAS(Map<String, Object> conf) {
public static int getEstimatedWorkerCountForRASTopo(Map<String, Object> topoConf, StormTopology topology)
throws InvalidTopologyException {
- return (int) Math.ceil(getEstimatedTotalHeapMemoryRequiredByTopo(topoConf, topology) /
- ObjectReader.getDouble(topoConf.get(Config.WORKER_HEAP_MEMORY_MB)));
+ Double defaultWorkerMaxHeap = ObjectReader.getDouble(topoConf.get(Config.WORKER_HEAP_MEMORY_MB));
+ Double topologyWorkerMaxHeap = ObjectReader.getDouble(topoConf.get(Config.TOPOLOGY_WORKER_MAX_HEAP_SIZE_MB));
--- End diff --
Will `topoConf.get(Config.WORKER_HEAP_MEMORY_MB)` be null? If so, this will throw exception `IllegalArgumentException("Don't know how to convert null to double");`
---
[GitHub] storm pull request #2902: [STORM-3282] Fix RAS worker count estimation
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/2902#discussion_r231608903
--- Diff: storm-server/src/main/java/org/apache/storm/utils/ServerUtils.java ---
@@ -691,8 +691,10 @@ public static boolean isRAS(Map<String, Object> conf) {
public static int getEstimatedWorkerCountForRASTopo(Map<String, Object> topoConf, StormTopology topology)
throws InvalidTopologyException {
- return (int) Math.ceil(getEstimatedTotalHeapMemoryRequiredByTopo(topoConf, topology) /
- ObjectReader.getDouble(topoConf.get(Config.WORKER_HEAP_MEMORY_MB)));
+ Double defaultWorkerMaxHeap = ObjectReader.getDouble(topoConf.get(Config.WORKER_HEAP_MEMORY_MB));
+ Double topologyWorkerMaxHeap = ObjectReader.getDouble(topoConf.get(Config.TOPOLOGY_WORKER_MAX_HEAP_SIZE_MB));
--- End diff --
The default in the code is last resort, as will be picked if we ever remove defaults from yaml
---
[GitHub] storm pull request #2902: [STORM-3282] Fix RAS worker count estimation
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2902
---