You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by govind-menon <gi...@git.apache.org> on 2017/09/07 18:50:49 UTC
[GitHub] storm pull request #2312: YSTORM-2725: Generic Resource Scheduling - initial...
GitHub user govind-menon opened a pull request:
https://github.com/apache/storm/pull/2312
YSTORM-2725: Generic Resource Scheduling - initial config changes and…
… TopologyBuilder API
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/govind-menon/storm YSTORM-2725
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2312.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 #2312
----
commit ab2fb4663e8548f37707bf423668bfc9e5607710
Author: Govind Menon <go...@gmail.com>
Date: 2017-09-07T18:50:05Z
YSTORM-2725: Generic Resource Scheduling - initial config changes and TopologyBuilder API
----
---
[GitHub] storm pull request #2312: YSTORM-2725: Generic Resource Scheduling - initial...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2312#discussion_r138090802
--- Diff: storm-client/src/jvm/org/apache/storm/topology/ComponentConfigurationDeclarer.java ---
@@ -62,4 +69,14 @@
* @return this for chaining.
*/
T setNumTasks(Number val);
+
+ /**
+ * Add generic resources for this component
--- End diff --
style: need a `.`
---
[GitHub] storm pull request #2312: YSTORM-2725: Generic Resource Scheduling - initial...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2312#discussion_r138090258
--- Diff: storm-client/src/jvm/org/apache/storm/topology/BaseConfigurationDeclarer.java ---
@@ -83,4 +83,29 @@ public T setCPULoad(Number amount) {
}
return (T) this;
}
+
+ @SuppressWarnings("unchecked")
+ @Override
+ public T addResources(Map<String, Double> resources) {
+ if(resources != null) {
--- End diff --
style: space after the `if` and before the `(`
---
[GitHub] storm pull request #2312: YSTORM-2725: Generic Resource Scheduling - initial...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/2312#discussion_r138926679
--- Diff: storm-server/src/main/java/org/apache/storm/daemon/supervisor/timer/SupervisorHeartbeat.java ---
@@ -73,10 +73,24 @@ private SupervisorInfo buildSupervisorInfo(Map<String, Object> conf, Supervisor
private Map<String, Double> mkSupervisorCapacities(Map<String, Object> conf) {
Map<String, Double> ret = new HashMap<String, Double>();
+ // Put in legacy values
Double mem = ObjectReader.getDouble(conf.get(Config.SUPERVISOR_MEMORY_CAPACITY_MB), 4096.0);
ret.put(Config.SUPERVISOR_MEMORY_CAPACITY_MB, mem);
Double cpu = ObjectReader.getDouble(conf.get(Config.SUPERVISOR_CPU_CAPACITY), 400.0);
ret.put(Config.SUPERVISOR_CPU_CAPACITY, cpu);
+
+
--- End diff --
extra lines
---
[GitHub] storm issue #2312: YSTORM-2725: Generic Resource Scheduling - initial config...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2312
@govind-menon
Looks like duplicated PR and #2385 covers this. If I'm right let's close this.
---
[GitHub] storm pull request #2312: YSTORM-2725: Generic Resource Scheduling - initial...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2312#discussion_r138089602
--- Diff: storm-client/src/jvm/org/apache/storm/Config.java ---
@@ -205,6 +205,12 @@
public static final String TOPOLOGY_TASKS = "topology.tasks";
/**
+ * A map of resources used by each component e.g {"cpu" : 200.0. "onheap.memory.mb": 256.0, "gpu" : 0.5 }
--- End diff --
Is this the default resources? Is this intended for users to actually modify? A bit more explanation here would be good.
---
[GitHub] storm pull request #2312: YSTORM-2725: Generic Resource Scheduling - initial...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2312#discussion_r138090061
--- Diff: storm-client/src/jvm/org/apache/storm/Config.java ---
@@ -1193,6 +1199,12 @@
public static final String SUPERVISOR_CPU_CAPACITY = "supervisor.cpu.capacity";
/**
+ * A map of resources the Supervisor has e.g {"cpu" : 200.0. "memory.capacity.mb": 256.0, "gpu" : 0.5 }
--- End diff --
How does this compare to `supervisor.cpu.capacity`? or the memory capacity? What keys are valid? More explanation here would be good.
---
[GitHub] storm pull request #2312: YSTORM-2725: Generic Resource Scheduling - initial...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2312#discussion_r138091077
--- Diff: storm-client/src/jvm/org/apache/storm/topology/ComponentConfigurationDeclarer.java ---
@@ -62,4 +69,14 @@
* @return this for chaining.
*/
T setNumTasks(Number val);
+
+ /**
+ * Add generic resources for this component
+ */
+ T addResources(Map<String, Double> resources);
+
+ /**
+ * Add generic resource for this component
--- End diff --
Just like with the javadocs it would be good to explain what keys are valid.
---
[GitHub] storm pull request #2312: YSTORM-2725: Generic Resource Scheduling - initial...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/2312#discussion_r138924774
--- Diff: storm-client/src/jvm/org/apache/storm/topology/BaseConfigurationDeclarer.java ---
@@ -83,4 +83,29 @@ public T setCPULoad(Number amount) {
}
return (T) this;
}
+
+ @SuppressWarnings("unchecked")
--- End diff --
Not sure if we need to `SuppressWarnings` here.
---
[GitHub] storm pull request #2312: YSTORM-2725: Generic Resource Scheduling - initial...
Posted by govind-menon <gi...@git.apache.org>.
GitHub user govind-menon reopened a pull request:
https://github.com/apache/storm/pull/2312
YSTORM-2725: Generic Resource Scheduling - initial config changes and…
… TopologyBuilder API
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/govind-menon/storm YSTORM-2725
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2312.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 #2312
----
commit ab2fb4663e8548f37707bf423668bfc9e5607710
Author: Govind Menon <go...@gmail.com>
Date: 2017-09-07T18:50:05Z
YSTORM-2725: Generic Resource Scheduling - initial config changes and TopologyBuilder API
----
---
[GitHub] storm pull request #2312: YSTORM-2725: Generic Resource Scheduling - initial...
Posted by govind-menon <gi...@git.apache.org>.
Github user govind-menon closed the pull request at:
https://github.com/apache/storm/pull/2312
---
[GitHub] storm pull request #2312: YSTORM-2725: Generic Resource Scheduling - initial...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2312#discussion_r138090901
--- Diff: storm-client/src/jvm/org/apache/storm/topology/ComponentConfigurationDeclarer.java ---
@@ -62,4 +69,14 @@
* @return this for chaining.
*/
T setNumTasks(Number val);
+
+ /**
+ * Add generic resources for this component
+ */
+ T addResources(Map<String, Double> resources);
+
+ /**
+ * Add generic resource for this component
--- End diff --
style: needs a `.` here too.
---
[GitHub] storm pull request #2312: YSTORM-2725: Generic Resource Scheduling - initial...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2312#discussion_r138090750
--- Diff: storm-client/src/jvm/org/apache/storm/topology/ComponentConfigurationDeclarer.java ---
@@ -28,6 +28,13 @@
T addConfigurations(Map<String, Object> conf);
/**
+ * return the configuration
--- End diff --
style: need a `.` at the end of the first javadoc line.
---
[GitHub] storm pull request #2312: YSTORM-2725: Generic Resource Scheduling - initial...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2312#discussion_r138090558
--- Diff: storm-client/src/jvm/org/apache/storm/topology/BaseConfigurationDeclarer.java ---
@@ -83,4 +83,29 @@ public T setCPULoad(Number amount) {
}
return (T) this;
}
+
+ @SuppressWarnings("unchecked")
+ @Override
+ public T addResources(Map<String, Double> resources) {
+ if(resources != null) {
+ return addConfiguration(Config.TOPOLOGY_COMPONENT_RESOURCES_MAP, resources);
+ }
+ return (T) this;
+ }
+
+ @SuppressWarnings("unchecked")
+ @Override
+ public T addResource(String resourceName, Double resourceValue) {
+ Map<String, Double> resourcesMap = (Map<String, Double>) ((Map<String, Object>) this).get(Config.TOPOLOGY_COMPONENT_RESOURCES_MAP);
+
+ if (resourcesMap != null) {
--- End diff --
I think you mean `if (resourcesMap == null) {` or we are going to be getting NPEs all over the place.
---
[GitHub] storm pull request #2312: YSTORM-2725: Generic Resource Scheduling - initial...
Posted by govind-menon <gi...@git.apache.org>.
Github user govind-menon closed the pull request at:
https://github.com/apache/storm/pull/2312
---