Ethanlm commented on a change in pull request #3346:
URL: https://github.com/apache/storm/pull/3346#discussion_r524462437
##########
File path:
storm-server/src/main/java/org/apache/storm/scheduler/resource/strategies/scheduling/BaseResourceAwareStrategy.java
##########
@@ -428,16 +465,39 @@ protected SchedulingResult
scheduleExecutorsOnNodes(List<ExecutorDetails> ordere
}
progressIdxForExec[execIndex]++;
+ int numBoundAckerAssigned
+ = assignBoundAckersForNewWorkerSlot(exec, node,
workerSlot);
+ if (numBoundAckerAssigned == -1) {
+ // This only happens when trying to assign bound
ackers to the worker slot and failed.
+ // Free the entire worker slot and put those bound
ackers back to unassigned list
+ LOG.debug("Failed to assign bound acker for exec: {}
of topo: {} to worker: {}. Backtracking.",
+ exec, topoName, workerSlot);
+ searcherState.freeWorkerSlotWithBoundAckers(node,
workerSlot);
+ continue;
+ }
+
if (!isExecAssignmentToWorkerValid(exec, workerSlot)) {
+ // This only happens when this exec can not fit in the
workerSlot
+ // and this is not the first exec to this workerSlot.
+ // So just go to next workerSlot and don't free the
worker.
+ if (numBoundAckerAssigned > 0) {
+ LOG.debug("Failed to assign exec: {} of topo: {}
with bound ackers to worker: {}. Backtracking.",
+ exec, topoName, workerSlot);
+ searcherState.freeWorkerSlotWithBoundAckers(node,
workerSlot);
+ }
continue;
}
searcherState.incStatesSearched();
searcherState.assignCurrentExecutor(execToComp, node,
workerSlot);
+ if (numBoundAckerAssigned > 0) {
+ // This exec with its bounded ackers have all been
successfully assigned
+ searcherState.getExecsWithBoundAckers().add(exec);
+ }
if (searcherState.areAllExecsScheduled()) {
//Everything is scheduled correctly, so no need to
search any more.
LOG.info("scheduleExecutorsOnNodes: Done at loopCnt={}
in {}ms, state.elapsedtime={}, backtrackCnt={}, topo={}",
- loopCnt, System.currentTimeMillis() -
startTimeMilli,
+ loopCnt, Time.currentTimeMillis() -
startTimeMilli,
Review comment:
`Time` is clock time if it is not in unit test.
I am not very clear why we need to use System here but not on the second one
----------------------------------------------------------------
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:
us...@infra.apache.org
