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
Handle scale during deploy #1037
Conversation
Could you add a unit test for this? |
test added |
@@ -603,7 +603,7 @@ private SingularityDeployResult markStepFinished(SingularityPendingDeploy pendin | |||
int numTasksToShutDown = Math.max(otherActiveTasks.size() - (request.getInstancesSafe() - deployProgress.getTargetActiveInstances()), 0); | |||
List<SingularityTaskId> sortedOtherTasks = new ArrayList<>(otherActiveTasks); | |||
Collections.sort(sortedOtherTasks, SingularityTaskId.INSTANCE_NO_COMPARATOR); | |||
return sortedOtherTasks.subList(0, numTasksToShutDown); | |||
return sortedOtherTasks.subList(0, Math.min(numTasksToShutDown, sortedOtherTasks.size() - 1)); |
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.
i might be misunderstanding something obvious, but why subtract one here?
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.
e.g. size of the array is 2, so the max index we can use for the sublist is 1
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.
But if you look at that javadoc, toIndex
is exclusive:
* @param fromIndex low endpoint (inclusive) of the subList
* @param toIndex high endpoint (exclusive) of the subList
* @return a view of the specified range within this list
* @throws IndexOutOfBoundsException for an illegal endpoint index value
* (<tt>fromIndex < 0 || toIndex > size ||
* fromIndex > toIndex</tt>)
*/
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.
So, is it the case that the index out of bounds is coming from the from index then instead? Had just assumed it was on the high end when I made this PR. must have been that the list was empty, will fix, thanks
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.
Fxed to check both for toIndex errors(with the Math.min) and fromIndex (isEmpty check). First condition gets hit by the test I added, second condition will get hit by a number of other tests
Found one last hitch with scale during deploy. If the timing is right, you will end up with the pre-scale number of requests running after the deploy is finished. The deploy ends with the targetActiveInstances, but we currently don't enqueue another pending request on successful deploy. Going to extend the test to check for this case then figure out the best way to fix it |
LGTM |
@stevenschlansker should fix that edge case you ran into the other day when scaling + deploying at the same time