Ethanlm commented on a change in pull request #3215: Storm3585 - New compact
Constraint config including maxNodeCoLocationCnt and incompatibleComponents
URL: https://github.com/apache/storm/pull/3215#discussion_r389921217
##########
File path:
storm-server/src/main/java/org/apache/storm/scheduler/resource/strategies/scheduling/ConstraintSolverStrategy.java
##########
@@ -593,13 +710,26 @@ public ExecutorDetails currentExec() {
return execs.get(execIndex);
}
+ /**
+ * Assign executor to worker and node.
+ * TODO: tryToSchedule is a misnomer, since it always schedules.
+ * Assignment validity check is done before the call to
tryToSchedule().
+ *
+ * @param execToComp Mapping from executor to component name.
+ * @param node RasNode on which to schedule.
+ * @param workerSlot WorkerSlot on which to schedule.
+ */
public void tryToSchedule(Map<ExecutorDetails, String> execToComp,
RasNode node, WorkerSlot workerSlot) {
ExecutorDetails exec = currentExec();
String comp = execToComp.get(exec);
LOG.trace("Trying assignment of {} {} to {}", exec, comp,
workerSlot);
- //It is possible that this component is already scheduled on this
node or worker. If so when we backtrack we cannot remove it
- okToRemoveFromWorker[execIndex] =
workerCompAssignment.computeIfAbsent(workerSlot, (k) -> new
HashSet<>()).add(comp);
- okToRemoveFromNode[execIndex] =
nodeCompAssignment.computeIfAbsent(node, (k) -> new HashSet<>()).add(comp);
+ // It is possible that this component is already scheduled on this
node or worker. If so when we backtrack we cannot remove it
+ Map<String, Integer> oneMap =
workerCompAssignmentCnts.computeIfAbsent(workerSlot, (k) -> new HashMap<>());
+ oneMap.put(comp, oneMap.getOrDefault(comp, 0) + 1); // increment
assignment count
+ okToRemoveFromWorker[execIndex] = true;
Review comment:
Let me explain my understanding of the original code and please let me know
what I am missing.
Let's say we are trying to assign `exec1` (whose component is `comp1`) to
`node1`. And let's discuss only `nodeCompAssignment` for simplicity.
First of all, we have `isExecAssignmentToWorkerValid` check before
`tryToSchedule`, so there won't be any cases where `comp1` is already in on
`node1` in `nodeCompAssignment` map, so `okToRemoveFromNoder[exec1]` is true.
When we backtrack, we find it `true` and set it `false`. It looks
`okToRemoveFromNoder` is not needed at all.
Secondly, if we don't check `isExecAssignmentToWorkerValid` before
`tryToSchedule`, let's say we already have `exec0` (whose component is `comp1`)
scheduled on `node1` and now we want to schedule `exec1`.
Now we will have `okToRemoveFromNoder[exec1]=false` since `comp1` is already
in `nodeCompAssignment` map. And when we backtrack, since
`okToRemoveFromNoder[exec1]=false`, we will not remove `comp1` from
`nodeCompAssignment` map since there is a `exec0` (also a `comp1`) scheduled on
this `node1`. This is also a correct behavior.
So I don't see a bug in the original code. Please elaborate the bug you
observed (asking this just for my knowledge). Thanks.
---
If we change `workerCompAssignment` and `nodeCompAssignment` from `<Map,
Set>` to `<Map, Map>`, I believe `okToRemoveFromWorker` and
`okToRemoveFromNode` are not needed anymore.
Another solution is to change `okToRemoveFromNode` from `boolean[]` to
`int[]` to keep track how many of the same component is on this node so far.
I can't say which solution is better. I believe `okToRemoveFromNode` was
introduced for performance but I don't know how much the performance gain is.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
