Skip to content
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

Make the task termination order deterministic #2265

Merged
merged 1 commit into from Dec 7, 2017

Conversation

@sungwonh
Copy link
Contributor

commented Jun 16, 2017

This PR adds support for making the task termination order deterministic when scaling down.
Please refer to #2264 for more details. (closes #2264)

@codecov

This comment has been minimized.

Copy link

commented Jun 16, 2017

Codecov Report

Merging #2265 into master will decrease coverage by 0.41%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2265      +/-   ##
==========================================
- Coverage   62.08%   61.66%   -0.42%     
==========================================
  Files          49      128      +79     
  Lines        6842    21080   +14238     
==========================================
+ Hits         4248    13000    +8752     
- Misses       2163     6682    +4519     
- Partials      431     1398     +967
// First sort tasks in an ascending order of slot number. This will
// make the task termination order deterministic.

sort.Sort(slotsBySlotNumber(slotsSlice))

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann Jun 16, 2017

Collaborator

golang's Sort function is not a stable sort, so I don't think this accomplishes anything. The same slice is immediately re-sorted afterwards using slotsByRunningState. I think the right way to do this is to change slotsByRunningState so that slot number is used as a tie-breaker.

This comment has been minimized.

Copy link
@sungwonh

sungwonh Jun 19, 2017

Author Contributor

Thank you for your feedback. According to your feedback, this PR is updated to use slot number as a tie-breaker in slotByRunningState.

@sungwonh sungwonh force-pushed the sungwonh:deterministic-scale-down branch 2 times, most recently from 77391de to ad203db Jun 19, 2017

@aaronlehmann
Copy link
Collaborator

left a comment

This looks much better, thanks. I've added some comments. Also, can you please add a unit test for this comparator, now that it is becoming a little more complicated?

// Use Slot number as a tie-breaker. This will make the order
// of task termination deterministic when scaling down.
iSlot := is[i]
jSlot := is[j]

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann Jun 19, 2017

Collaborator

We use is[i] and is[j] above, so it seems strange to assign shorthands to them here. For consistency, we should either define these at the beginning of the function, or use is[i] and is[j] directly. My preference would be the latter, since is[i] and is[j] are short and assigning them to a variable does not make the code clearer.

// of task termination deterministic when scaling down.
iSlot := is[i]
jSlot := is[j]
if (iRunning && jRunning) || (!iRunning && !jRunning) {

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann Jun 19, 2017

Collaborator

As a small cosmetic tweak, could you move this check above as:

if !iRunning && jRunning {
        return false
}

This feels a little more symmetric with the if iRunning && !jRunning check.

Then as the tie-breaker, we can have return is[i][0].Slot < is[j][0].Slot.

@sungwonh sungwonh force-pushed the sungwonh:deterministic-scale-down branch from ad203db to 986a1aa Jun 20, 2017

@sungwonh

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2017

@aaronlehmann Thank you for your feedback. PR is updated!

@aaronlehmann

This comment has been minimized.

Copy link
Collaborator

commented Jun 20, 2017

LGTM

Thank you for making the changes!

@sebastianbinder

This comment has been minimized.

Copy link

commented Dec 1, 2017

What's the current state of this PR? Would love to see this feature implemented.

@sungwonh

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2017

@nishanttotla
Who else do I need approval from / what is the process left to get this PR merged?

@sebastianbinder

This comment has been minimized.

Copy link

commented Dec 1, 2017

@sungwonh I think first of all you need to rebase as there are conflicting files.

@sungwonh sungwonh force-pushed the sungwonh:deterministic-scale-down branch 5 times, most recently from e70aeee to 0237a28 Dec 1, 2017

@sungwonh

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2017

Rebased!

@anshulpundir
Copy link
Contributor

left a comment

Minor comments. I'll merge the change once they are addressed @sungwonh

return false
}

// Use Slot number as a tie-breaker. This will make the order

This comment has been minimized.

Copy link
@anshulpundir

anshulpundir Dec 4, 2017

Contributor

Please add a comment on why doing this in increasing slot ids is better.

@@ -29,7 +29,17 @@ func (is slotsByRunningState) Less(i, j int) bool {
}
}

return iRunning && !jRunning
if iRunning && !jRunning {

This comment has been minimized.

Copy link
@anshulpundir

anshulpundir Dec 4, 2017

Contributor

Can I also request you to add a high level comment for the Less() function ? thx!

@sungwonh sungwonh force-pushed the sungwonh:deterministic-scale-down branch from 0237a28 to 1aecc00 Dec 5, 2017

Make the task termination order deterministic
Use Slot number as a tie-breaker

Signed-off-by: Sungwon Han <sungwon.han@navercorp.com>

@sungwonh sungwonh force-pushed the sungwonh:deterministic-scale-down branch from 1aecc00 to dfee0c4 Dec 5, 2017

@sungwonh

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2017

@anshulpundir updated according to your comments.

@anshulpundir anshulpundir merged commit 6fc872d into docker:master Dec 7, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/project 61.66% (target 0%)
Details
dco-signed All commits are signed

@sungwonh sungwonh deleted the sungwonh:deterministic-scale-down branch Dec 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.