You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by jerrypeng <gi...@git.apache.org> on 2016/01/14 21:25:56 UTC
[GitHub] storm pull request: Storm 1450
GitHub user jerrypeng opened a pull request:
https://github.com/apache/storm/pull/1016
Storm 1450
Code refactored:
1. Refactor RAS_Nodes. Pushed some of the functionality in to RAS_Nodes. Each RAS_Node will now be initialized with a map of all its assignments. Each RAS_Node will also figure out resources used and available. Removed unnecessary functions.
2. Made WorkerSlot immutable so that a scheduling strategy won't mistakenly modify it
3. Added a wrapping layer for RAS_Node to feed into scheduling strategies so that the semantics of what a scheduling strategy should do will be more clear. Each scheduling strategy shouldn't be actually assigning anything. The strategy should only calculate a scheduling.
Bug fixes:
1. Minor bug in displaying the assigned resources for a supervisor on the UI. The function updateSupervisorResources was placed in the wrong place
2. Minor bug fix in freeing memory in RAS_Node there was some wrong math that was done.
Added Additional unit tests to test
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jerrypeng/storm STORM-1450
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1016.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 #1016
----
commit 829ea117c8b382ef72d3c5df841534b89f06dffa
Author: Boyang Jerry Peng <je...@yahoo-inc.com>
Date: 2016-01-13T18:28:38Z
[STORM-1450] - Fix bugs and refactor code in ResourceAwareScheduler
commit 688366902d98a7fdc17d854a33ff975c6a60ccab
Author: Boyang Jerry Peng <je...@yahoo-inc.com>
Date: 2016-01-13T20:13:54Z
adding an additional test and cleaning up
----
---
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-1450] - Fix bugs and refactor code in R...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/1016#issuecomment-172046954
Looks good to me. Please do create a Jira to replace the Double[]. +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-1450] - Fix bugs and refactor code in R...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49787051
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/resource/RAS_Node.java ---
@@ -247,19 +253,23 @@ private void freeMemory(double amount) {
private void freeCPU(double amount) {
LOG.debug("freeing {} CPU on node...avail CPU: {}", amount, getHostname(), _availCPU);
- if ((_availCPU + amount) > getAvailableCpuResources()) {
- LOG.warn("Freeing more CPU than there exists!");
+ if ((_availCPU + amount) > getTotalCpuResources()) {
+ LOG.warn("Freeing more CPU than there exists! CPU trying to free: {} Total CPU on Node: {}", (_availMemory + amount), getTotalCpuResources());
return;
}
_availCPU += amount;
}
+ /**
+ * get the amount of memory used by a worker
+ */
public double getMemoryUsedByWorker(WorkerSlot ws) {
TopologyDetails topo = findTopologyUsingWorker(ws);
if (topo == null) {
return 0.0;
}
Collection<ExecutorDetails> execs = getExecutors(ws, _cluster);
+ LOG.info("getMemoryUsedByWorker executors: {}", execs);
--- End diff --
Is it useful for this to be INFO level? This could be a lot of text to log.
---
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-1450] - Fix bugs and refactor code in R...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49821028
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/resource/RAS_Node.java ---
@@ -43,32 +43,84 @@
*/
public class RAS_Node {
private static final Logger LOG = LoggerFactory.getLogger(RAS_Node.class);
- private Map<String, Set<WorkerSlot>> _topIdToUsedSlots = new HashMap<String, Set<WorkerSlot>>();
- private Set<WorkerSlot> _freeSlots = new HashSet<WorkerSlot>();
+
+ //A map consisting of all workers on the node.
+ //The key of the map is the worker id and the value is the corresponding workerslot object
+ Map<String, WorkerSlot> _slots = new HashMap<String, WorkerSlot> ();
+
+ // A map describing which topologies are using which slots on this node. The format of the map is the following:
+ // {TopologyId -> {WorkerId -> {Executors}}}
+ private Map<String, Map<String, Collection<ExecutorDetails>>> _topIdToUsedSlots = new HashMap<String, Map<String, Collection<ExecutorDetails>>>();
+
private final String _nodeId;
private String _hostname;
private boolean _isAlive;
private SupervisorDetails _sup;
- private Double _availMemory;
- private Double _availCPU;
- private Cluster _cluster;
- private Topologies _topologies;
+ private Double _availMemory = 0.0;
+ private Double _availCPU = 0.0;
+ private final Cluster _cluster;
+ private final Topologies _topologies;
- public RAS_Node(String nodeId, Set<Integer> allPorts, boolean isAlive,
- SupervisorDetails sup, Cluster cluster, Topologies topologies) {
+ public RAS_Node(String nodeId, SupervisorDetails sup, Cluster cluster, Topologies topologies, Map<String, WorkerSlot> workerIdToWorker, Map<String, Map<String, Collection<ExecutorDetails>>> assignmentMap) {
+ //Node ID and supervisor ID are the same.
_nodeId = nodeId;
- _isAlive = isAlive;
- if (_isAlive && allPorts != null) {
- for (int port : allPorts) {
- _freeSlots.add(new WorkerSlot(_nodeId, port));
- }
- _sup = sup;
+ if (sup == null) {
+ _isAlive = false;
+ } else {
+ _isAlive = !cluster.isBlackListed(_nodeId);
+ }
+
+ _cluster = cluster;
+ _topologies = topologies;
+
+ // initialize slots for this node
+ if (workerIdToWorker != null) {
+ _slots = workerIdToWorker;
+ }
+
+ //initialize assignment map
+ if (assignmentMap != null) {
+ _topIdToUsedSlots = assignmentMap;
+ }
+
+ //check if node is alive
+ if (_isAlive && sup != null) {
_hostname = sup.getHost();
+ _sup = sup;
_availMemory = getTotalMemoryResources();
_availCPU = getTotalCpuResources();
+
+ LOG.debug("Found a {} Node {} {}",
+ _isAlive ? "living" : "dead", _nodeId, sup.getAllPorts());
+ LOG.debug("resources_mem: {}, resources_CPU: {}", sup.getTotalMemory(), sup.getTotalCPU());
+ //intialize resource usages on node
+ intializeResources();
+ }
+ }
+
+ /**
+ * intializes resource usages on node
--- End diff --
will fix
---
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-1450] - Fix bugs and refactor code in R...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49785320
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/WorkerSlot.java ---
@@ -40,11 +47,8 @@ public int getPort() {
return port;
}
- public WorkerSlot allocateResource(double memOnHeap, double memOffHeap, double cpu) {
- this.memOnHeap += memOnHeap;
- this.memOffHeap += memOffHeap;
- this.cpu += cpu;
- return this;
+ public String getId() {
+ return this.getNodeId() + ":" + this.getPort();
--- End diff --
minor: Do not need `this.` when calling methods.
---
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-1450] - Fix bugs and refactor code in R...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/1016#issuecomment-171802669
Mostly minor comments, otherwise looks ok to me.
---
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-1450] - Fix bugs and refactor code in R...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49786868
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/resource/RAS_Node.java ---
@@ -103,92 +182,37 @@ public boolean isAlive() {
}
public boolean isTotallyFree() {
- return _topIdToUsedSlots.isEmpty();
+ return getUsedSlots().isEmpty();
}
public int totalSlotsFree() {
- return _freeSlots.size();
+ return getFreeSlots().size();
}
public int totalSlotsUsed() {
- int total = 0;
- for (Set<WorkerSlot> slots : _topIdToUsedSlots.values()) {
- total += slots.size();
- }
- return total;
+ return getUsedSlots().size();
}
public int totalSlots() {
- return totalSlotsFree() + totalSlotsUsed();
+ return _slots.size();
}
public int totalSlotsUsed(String topId) {
- int total = 0;
- Set<WorkerSlot> slots = _topIdToUsedSlots.get(topId);
- if (slots != null) {
- total = slots.size();
- }
- return total;
- }
-
- private void validateSlot(WorkerSlot ws) {
- if (!_nodeId.equals(ws.getNodeId())) {
- throw new IllegalArgumentException(
- "Trying to add a slot to the wrong node " + ws +
- " is not a part of " + _nodeId);
- }
- }
-
- void addOrphanedSlot(WorkerSlot ws) {
- if (_isAlive) {
- throw new IllegalArgumentException("Orphaned Slots " +
- "only are allowed on dead nodes.");
- }
- validateSlot(ws);
- if (_freeSlots.contains(ws)) {
- return;
- }
- for (Set<WorkerSlot> used : _topIdToUsedSlots.values()) {
- if (used.contains(ws)) {
- return;
- }
- }
- _freeSlots.add(ws);
- }
-
- boolean assignInternal(WorkerSlot ws, String topId, boolean dontThrow) {
- validateSlot(ws);
- if (!_freeSlots.remove(ws)) {
- if (dontThrow) {
- return true;
- }
- throw new IllegalStateException("Assigning a slot that was not free " + ws);
- }
- Set<WorkerSlot> usedSlots = _topIdToUsedSlots.get(topId);
- if (usedSlots == null) {
- usedSlots = new HashSet<WorkerSlot>();
- _topIdToUsedSlots.put(topId, usedSlots);
- }
- usedSlots.add(ws);
- return false;
+ return getUsedSlots(topId).size();
}
/**
* Free all slots on this node. This will update the Cluster too.
*/
- public void freeAllSlots() {
+ public void freeAllSlots() {
--- End diff --
revert?
---
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-1450] - Fix bugs and refactor code in R...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/1016#issuecomment-172988140
@jerrypeng Can you add this to the CHANGELOG?
---
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-1450] - Fix bugs and refactor code in R...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49783732
--- Diff: storm-core/src/clj/org/apache/storm/daemon/nimbus.clj ---
@@ -701,11 +701,7 @@
;; making a map from node+port to WorkerSlot with allocated resources
node+port->slot (into {} (for [[[node port] [mem-on-heap mem-off-heap cpu]] worker->resources]
{[node port]
- (doto (WorkerSlot. node port)
- (.allocateResource
- mem-on-heap
- mem-off-heap
- cpu))}))
+ (doto (WorkerSlot. node port mem-on-heap mem-off-heap cpu))}))
--- End diff --
will remove
---
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-1450] - Fix bugs and refactor code in R...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1016#issuecomment-173824017
@jerrypeng
Sorry I think I pushed merged commit to 1.x, but I forgot to merge branch then push only README commit. ;(
Since 1.x-branch is not diverged to master yet, it doesn't require additional pull request to backport.
It's backported now. Could you check below commit and close #1033 if something is not wrong?
https://github.com/apache/storm/commit/3e119eef2f5a81aca85ca9ee93a712cf0202ba0e
---
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-1450] - Fix bugs and refactor code in R...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49872181
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/resource/RAS_Node.java ---
@@ -103,92 +182,37 @@ public boolean isAlive() {
}
public boolean isTotallyFree() {
- return _topIdToUsedSlots.isEmpty();
+ return getUsedSlots().isEmpty();
--- End diff --
ok
---
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-1450] - Fix bugs and refactor code in R...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1016#issuecomment-173768849
Backported to 1.x-branch. =)
---
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-1450] - Fix bugs and refactor code in R...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49788091
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/resource/strategies/scheduling/IStrategy.java ---
@@ -36,7 +31,7 @@
/**
* initialize prior to scheduling
*/
- public void prepare(Topologies topologies, Cluster cluster, Map<String, User> userMap, RAS_Nodes nodes);
+ public void prepare(ClusterStateData clusterStateData);
--- End diff --
minor: public not needed
---
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-1450] - Fix bugs and refactor code in R...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49820988
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/SchedulerAssignment.java ---
@@ -55,4 +56,6 @@
public Set<ExecutorDetails> getExecutors();
public Set<WorkerSlot> getSlots();
+
+ public Map<WorkerSlot, Collection<ExecutorDetails>> getSlotToExecutors();
--- End diff --
@d2r should I delete all of the public declarations for all the methods in this class then?
---
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-1450] - Fix bugs and refactor code in R...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/1016#issuecomment-172022813
@d2r thank you for your review! I think I have address all of your comments. Do you have anymore concerns?
---
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-1450] - Fix bugs and refactor code in R...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/1016#issuecomment-173961349
@HeartSaVioR thanks! everything looks correct. I will close #1033
---
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-1450] - Fix bugs and refactor code in R...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49821063
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/resource/RAS_Node.java ---
@@ -103,92 +182,37 @@ public boolean isAlive() {
}
public boolean isTotallyFree() {
- return _topIdToUsedSlots.isEmpty();
+ return getUsedSlots().isEmpty();
}
public int totalSlotsFree() {
- return _freeSlots.size();
+ return getFreeSlots().size();
}
public int totalSlotsUsed() {
- int total = 0;
- for (Set<WorkerSlot> slots : _topIdToUsedSlots.values()) {
- total += slots.size();
- }
- return total;
+ return getUsedSlots().size();
}
public int totalSlots() {
- return totalSlotsFree() + totalSlotsUsed();
+ return _slots.size();
}
public int totalSlotsUsed(String topId) {
- int total = 0;
- Set<WorkerSlot> slots = _topIdToUsedSlots.get(topId);
- if (slots != null) {
- total = slots.size();
- }
- return total;
- }
-
- private void validateSlot(WorkerSlot ws) {
- if (!_nodeId.equals(ws.getNodeId())) {
- throw new IllegalArgumentException(
- "Trying to add a slot to the wrong node " + ws +
- " is not a part of " + _nodeId);
- }
- }
-
- void addOrphanedSlot(WorkerSlot ws) {
- if (_isAlive) {
- throw new IllegalArgumentException("Orphaned Slots " +
- "only are allowed on dead nodes.");
- }
- validateSlot(ws);
- if (_freeSlots.contains(ws)) {
- return;
- }
- for (Set<WorkerSlot> used : _topIdToUsedSlots.values()) {
- if (used.contains(ws)) {
- return;
- }
- }
- _freeSlots.add(ws);
- }
-
- boolean assignInternal(WorkerSlot ws, String topId, boolean dontThrow) {
- validateSlot(ws);
- if (!_freeSlots.remove(ws)) {
- if (dontThrow) {
- return true;
- }
- throw new IllegalStateException("Assigning a slot that was not free " + ws);
- }
- Set<WorkerSlot> usedSlots = _topIdToUsedSlots.get(topId);
- if (usedSlots == null) {
- usedSlots = new HashSet<WorkerSlot>();
- _topIdToUsedSlots.put(topId, usedSlots);
- }
- usedSlots.add(ws);
- return false;
+ return getUsedSlots(topId).size();
}
/**
* Free all slots on this node. This will update the Cluster too.
*/
- public void freeAllSlots() {
+ public void freeAllSlots() {
--- End diff --
will fix
---
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-1450] - Fix bugs and refactor code in R...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/1016#issuecomment-173382241
+1 for backporting to 1.x-branch.
---
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-1450] - Fix bugs and refactor code in R...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49787766
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/resource/strategies/scheduling/DefaultResourceAwareStrategy.java ---
@@ -121,7 +116,7 @@ public SchedulingResult schedule(TopologyDetails td) {
td.getTaskResourceReqList(exec), entry.getKey() });
WorkerSlot targetSlot = this.findWorkerForExec(exec, td, schedulerAssignmentMap);
if (targetSlot != null) {
- RAS_Node targetNode = this.idToNode(targetSlot.getNodeId());
+ NodeDetails targetNode = this.idToNode(targetSlot.getNodeId());
--- End diff --
It seems there is a chunk of duplicate code here and in the for loop below. We are adding one line to both, but maybe it would be better to pull them out into a helper method to avoid accidental mistakes in the future?
---
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-1450] - Fix bugs and refactor code in R...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49821516
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/resource/strategies/scheduling/DefaultResourceAwareStrategy.java ---
@@ -121,7 +116,7 @@ public SchedulingResult schedule(TopologyDetails td) {
td.getTaskResourceReqList(exec), entry.getKey() });
WorkerSlot targetSlot = this.findWorkerForExec(exec, td, schedulerAssignmentMap);
if (targetSlot != null) {
- RAS_Node targetNode = this.idToNode(targetSlot.getNodeId());
+ NodeDetails targetNode = this.idToNode(targetSlot.getNodeId());
--- End diff --
will refactor
---
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-1450] - Fix bugs and refactor code in R...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49821113
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/resource/RAS_Node.java ---
@@ -247,19 +253,23 @@ private void freeMemory(double amount) {
private void freeCPU(double amount) {
LOG.debug("freeing {} CPU on node...avail CPU: {}", amount, getHostname(), _availCPU);
- if ((_availCPU + amount) > getAvailableCpuResources()) {
- LOG.warn("Freeing more CPU than there exists!");
+ if ((_availCPU + amount) > getTotalCpuResources()) {
+ LOG.warn("Freeing more CPU than there exists! CPU trying to free: {} Total CPU on Node: {}", (_availMemory + amount), getTotalCpuResources());
return;
}
_availCPU += amount;
}
+ /**
+ * get the amount of memory used by a worker
+ */
public double getMemoryUsedByWorker(WorkerSlot ws) {
TopologyDetails topo = findTopologyUsingWorker(ws);
if (topo == null) {
return 0.0;
}
Collection<ExecutorDetails> execs = getExecutors(ws, _cluster);
+ LOG.info("getMemoryUsedByWorker executors: {}", execs);
--- End diff --
will get rid of
---
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-1450] - Fix bugs and refactor code in R...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49781622
--- Diff: storm-core/test/jvm/org/apache/storm/scheduler/resource/TestResourceAwareScheduler.java ---
@@ -939,6 +945,76 @@ public void TestEvictTopologyFromItself() {
* If users are above his or her guarantee, check if topology eviction works correct
*/
@Test
+ public void Test() {
--- End diff --
Rename?
---
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-1450] - Fix bugs and refactor code in R...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/1016#issuecomment-173821645
thanks @HeartSaVioR! I just put up a PR for the backport
https://github.com/apache/storm/pull/1033
---
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-1450] - Fix bugs and refactor code in R...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49888269
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/Cluster.java ---
@@ -662,22 +662,50 @@ public void setStatusMap(Map<String, String> statusMap) {
this.status.putAll(statusMap);
}
- public void setResources(String topologyId, Double[] resources) {
- this.resources.put(topologyId, resources);
+ /**
+ * Set the amount of resources used used by a topology. Used for displaying resource information on the UI
+ * @param topologyId
+ * @param resources describes the resources requested and assigned to topology in the following format in an array:
+ * {requestedMemOnHeap, requestedMemOffHeap, requestedCpu, assignedMemOnHeap, assignedMemOffHeap, assignedCpu}
+ */
+ public void setTopologyResources(String topologyId, Double[] resources) {
--- End diff --
Yes, please create a Jira for this. It will make things less error-prone in the future.
---
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-1450] - Fix bugs and refactor code in R...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49783662
--- Diff: storm-core/src/clj/org/apache/storm/daemon/nimbus.clj ---
@@ -701,11 +701,7 @@
;; making a map from node+port to WorkerSlot with allocated resources
node+port->slot (into {} (for [[[node port] [mem-on-heap mem-off-heap cpu]] worker->resources]
{[node port]
- (doto (WorkerSlot. node port)
- (.allocateResource
- mem-on-heap
- mem-off-heap
- cpu))}))
+ (doto (WorkerSlot. node port mem-on-heap mem-off-heap cpu))}))
--- End diff --
Can we remove the `doto` here?
---
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-1450] - Fix bugs and refactor code in R...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49786777
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/resource/RAS_Node.java ---
@@ -103,92 +182,37 @@ public boolean isAlive() {
}
public boolean isTotallyFree() {
- return _topIdToUsedSlots.isEmpty();
+ return getUsedSlots().isEmpty();
--- End diff --
Is this method used?
---
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-1450] - Fix bugs and refactor code in R...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/1016#issuecomment-173587159
+1 for 1.x
---
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-1450] - Fix bugs and refactor code in R...
Posted by zhuoliu <gi...@git.apache.org>.
Github user zhuoliu commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49785382
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/Cluster.java ---
@@ -662,22 +662,50 @@ public void setStatusMap(Map<String, String> statusMap) {
this.status.putAll(statusMap);
}
- public void setResources(String topologyId, Double[] resources) {
- this.resources.put(topologyId, resources);
+ /**
+ * Set the amount of resources used used by a topology. Used for displaying resource information on the UI
+ * @param topologyId
+ * @param resources describes the resources requested and assigned to topology in the following format in an array:
+ * {requestedMemOnHeap, requestedMemOffHeap, requestedCpu, assignedMemOnHeap, assignedMemOffHeap, assignedCpu}
+ */
+ public void setTopologyResources(String topologyId, Double[] resources) {
--- End diff --
Yep, I think that would be helpful. Both for topology and supervisor resources. We may file a separate JIRA for that or do it within this pull request.
---
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-1450] - Fix bugs and refactor code in R...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49821059
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/resource/RAS_Node.java ---
@@ -103,92 +182,37 @@ public boolean isAlive() {
}
public boolean isTotallyFree() {
- return _topIdToUsedSlots.isEmpty();
+ return getUsedSlots().isEmpty();
--- End diff --
its used in resource_aware_scheduler_test.clj
---
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-1450] - Fix bugs and refactor code in R...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/1016#issuecomment-172022837
@d2r thank you for your review! I think I have address all of your comments. Do you have anymore concerns?
---
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-1450] - Fix bugs and refactor code in R...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49784882
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/SchedulerAssignment.java ---
@@ -55,4 +56,6 @@
public Set<ExecutorDetails> getExecutors();
public Set<WorkerSlot> getSlots();
+
+ public Map<WorkerSlot, Collection<ExecutorDetails>> getSlotToExecutors();
--- End diff --
very minor: The interface is public, so it's redundant to mark each method declaration as public.
---
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-1450] - Fix bugs and refactor code in R...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49820997
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/WorkerSlot.java ---
@@ -40,11 +47,8 @@ public int getPort() {
return port;
}
- public WorkerSlot allocateResource(double memOnHeap, double memOffHeap, double cpu) {
- this.memOnHeap += memOnHeap;
- this.memOffHeap += memOffHeap;
- this.cpu += cpu;
- return this;
+ public String getId() {
+ return this.getNodeId() + ":" + this.getPort();
--- End diff --
will remove
---
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-1450] - Fix bugs and refactor code in R...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/1016#issuecomment-172988575
Will add to CHANGELOG
On Tue, Jan 19, 2016 at 3:11 PM, P. Taylor Goetz <no...@github.com>
wrote:
> @jerrypeng <https://github.com/jerrypeng> Can you add this to the
> CHANGELOG?
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/storm/pull/1016#issuecomment-172988140>.
>
---
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-1450] - Fix bugs and refactor code in R...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/1016#issuecomment-173458467
Ya sure I can back port to the 1.x branch as well
---
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-1450] - Fix bugs and refactor code in R...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1016#issuecomment-173378037
@jerrypeng @d2r
It was merged only to master (misplaced CHANGELOG, will fix it) but contains bug fixes so I think it should be backported to 1.x-branch as well.
What do you think?
---
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-1450] - Fix bugs and refactor code in R...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49783928
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/Cluster.java ---
@@ -662,22 +662,50 @@ public void setStatusMap(Map<String, String> statusMap) {
this.status.putAll(statusMap);
}
- public void setResources(String topologyId, Double[] resources) {
- this.resources.put(topologyId, resources);
+ /**
+ * Set the amount of resources used used by a topology. Used for displaying resource information on the UI
+ * @param topologyId
+ * @param resources describes the resources requested and assigned to topology in the following format in an array:
+ * {requestedMemOnHeap, requestedMemOffHeap, requestedCpu, assignedMemOnHeap, assignedMemOffHeap, assignedCpu}
+ */
+ public void setTopologyResources(String topologyId, Double[] resources) {
--- End diff --
Can we replace `Double[]` with a class that is appropriate to the data?
---
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-1450] - Fix bugs and refactor code in R...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49872156
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/SchedulerAssignment.java ---
@@ -55,4 +56,6 @@
public Set<ExecutorDetails> getExecutors();
public Set<WorkerSlot> getSlots();
+
+ public Map<WorkerSlot, Collection<ExecutorDetails>> getSlotToExecutors();
--- End diff --
Up to you. It's a really minor thing, and this might get resolved when we adopt a Java standard style later. I am fine if you want to leave it in or change it, either way.
---
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-1450] - Fix bugs and refactor code in R...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49873652
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/SchedulerAssignment.java ---
@@ -55,4 +56,6 @@
public Set<ExecutorDetails> getExecutors();
public Set<WorkerSlot> getSlots();
+
+ public Map<WorkerSlot, Collection<ExecutorDetails>> getSlotToExecutors();
--- End diff --
I think we can just leave it for now and when we come up with a standard/style guide we can change it then
---
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-1450] - Fix bugs and refactor code in R...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49786540
--- Diff: storm-core/src/jvm/org/apache/storm/scheduler/resource/RAS_Node.java ---
@@ -43,32 +43,84 @@
*/
public class RAS_Node {
private static final Logger LOG = LoggerFactory.getLogger(RAS_Node.class);
- private Map<String, Set<WorkerSlot>> _topIdToUsedSlots = new HashMap<String, Set<WorkerSlot>>();
- private Set<WorkerSlot> _freeSlots = new HashSet<WorkerSlot>();
+
+ //A map consisting of all workers on the node.
+ //The key of the map is the worker id and the value is the corresponding workerslot object
+ Map<String, WorkerSlot> _slots = new HashMap<String, WorkerSlot> ();
+
+ // A map describing which topologies are using which slots on this node. The format of the map is the following:
+ // {TopologyId -> {WorkerId -> {Executors}}}
+ private Map<String, Map<String, Collection<ExecutorDetails>>> _topIdToUsedSlots = new HashMap<String, Map<String, Collection<ExecutorDetails>>>();
+
private final String _nodeId;
private String _hostname;
private boolean _isAlive;
private SupervisorDetails _sup;
- private Double _availMemory;
- private Double _availCPU;
- private Cluster _cluster;
- private Topologies _topologies;
+ private Double _availMemory = 0.0;
+ private Double _availCPU = 0.0;
+ private final Cluster _cluster;
+ private final Topologies _topologies;
- public RAS_Node(String nodeId, Set<Integer> allPorts, boolean isAlive,
- SupervisorDetails sup, Cluster cluster, Topologies topologies) {
+ public RAS_Node(String nodeId, SupervisorDetails sup, Cluster cluster, Topologies topologies, Map<String, WorkerSlot> workerIdToWorker, Map<String, Map<String, Collection<ExecutorDetails>>> assignmentMap) {
+ //Node ID and supervisor ID are the same.
_nodeId = nodeId;
- _isAlive = isAlive;
- if (_isAlive && allPorts != null) {
- for (int port : allPorts) {
- _freeSlots.add(new WorkerSlot(_nodeId, port));
- }
- _sup = sup;
+ if (sup == null) {
+ _isAlive = false;
+ } else {
+ _isAlive = !cluster.isBlackListed(_nodeId);
+ }
+
+ _cluster = cluster;
+ _topologies = topologies;
+
+ // initialize slots for this node
+ if (workerIdToWorker != null) {
+ _slots = workerIdToWorker;
+ }
+
+ //initialize assignment map
+ if (assignmentMap != null) {
+ _topIdToUsedSlots = assignmentMap;
+ }
+
+ //check if node is alive
+ if (_isAlive && sup != null) {
_hostname = sup.getHost();
+ _sup = sup;
_availMemory = getTotalMemoryResources();
_availCPU = getTotalCpuResources();
+
+ LOG.debug("Found a {} Node {} {}",
+ _isAlive ? "living" : "dead", _nodeId, sup.getAllPorts());
+ LOG.debug("resources_mem: {}, resources_CPU: {}", sup.getTotalMemory(), sup.getTotalCPU());
+ //intialize resource usages on node
+ intializeResources();
+ }
+ }
+
+ /**
+ * intializes resource usages on node
--- End diff --
`initializes`
---
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-1450] - Fix bugs and refactor code in R...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1016
---
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-1450] - Fix bugs and refactor code in R...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/1016#discussion_r49781708
--- Diff: storm-core/test/jvm/org/apache/storm/scheduler/resource/TestResourceAwareScheduler.java ---
@@ -1083,6 +1159,15 @@ public void TestOverGuaranteeEviction() {
Assert.assertEquals("# of pending topologies", 0, rs.getUser("bobby").getTopologiesPending().size());
Assert.assertEquals("# of attempted topologies", 0, rs.getUser("bobby").getTopologiesAttempted().size());
Assert.assertEquals("# of invalid topologies", 0, rs.getUser("bobby").getTopologiesInvalid().size());
+
+ LOG.info("Assignments: {}", cluster.getAssignments());
+ for (Map.Entry<String, SchedulerAssignment> entry : cluster.getAssignments().entrySet()) {
+ LOG.info("Topology id: {}", entry.getKey());
+ for(WorkerSlot target: entry.getValue().getSlots()) {
+ LOG.info("target resources onheap: {} offheap: {} cpu: {}", target.getAllocatedMemOnHeap(), target.getAllocatedMemOffHeap(), target.getAllocatedCpu());
+ }
+
+ }
--- End diff --
Revert?
---
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.
---