-
Notifications
You must be signed in to change notification settings - Fork 187
Evenly-spread task placement #1576
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -333,7 +333,8 @@ public void testFrozenSlaveCanBeDecommissioned() { | |||
|
|||
saveAndSchedule(request.toBuilder().setSlavePlacement(Optional.of(SlavePlacement.OPTIMISTIC)).setInstances(Optional.of(2))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feel free to change this to GREEDY or SEPARATE so that the frozen slave test isn't relying on calculations from optimistic
|
||
final boolean isSlaveOk = numOnSlave < numPerSlave; | ||
// If no tasks are active for this request yet, we can fall back to greedy. | ||
if (currentlyActiveTasksForRequestClusterwide.size() > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to make this more explicit with a == 0 check + early return rather than having to add a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, so the general pattern in this method is to return only if the SlaveMatchState
is determined to be something other than OK
. Otherwise, flow is allowed to trickle down to that final return SlaveMatchState.OK
statement.
final boolean isSlaveOk = numOnSlave <= numPerSlave; | ||
|
||
if (!isSlaveOk) { | ||
LOG.trace("Rejecting OPTIMISTIC task {} from slave {} ({}) due to numOnSlave {}", taskRequest.getRequest().getId(), slaveId, host, numOnSlave); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might want to add the other values you're using in he calculation in here for debugging purposes
…lacement strategy logic for this test.
No description provided.