Make the task termination order deterministic#2265
Conversation
Codecov Report
@@ 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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thank you for your feedback. According to your feedback, this PR is updated to use slot number as a tie-breaker in slotByRunningState.
sungwonh
force-pushed
the
deterministic-scale-down
branch
2 times, most recently
from
77391de to
ad203db
Compare
aaronlehmann
left a comment
aaronlehmann
left a comment
There was a problem hiding this 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] |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
force-pushed
the
deterministic-scale-down
branch
from
ad203db to
986a1aa
Compare
|
@aaronlehmann Thank you for your feedback. PR is updated! |
|
LGTM Thank you for making the changes! |
|
What's the current state of this PR? Would love to see this feature implemented. |
|
@nishanttotla |
|
@sungwonh I think first of all you need to rebase as there are conflicting files. |
sungwonh
force-pushed
the
deterministic-scale-down
branch
5 times, most recently
from
e70aeee to
0237a28
Compare
|
Rebased! |
anshulpundir
left a comment
anshulpundir
left a comment
There was a problem hiding this 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 |
There was a problem hiding this comment.
Please add a comment on why doing this in increasing slot ids is better.
| } | ||
|
|
||
| return iRunning && !jRunning | ||
| if iRunning && !jRunning { |
There was a problem hiding this comment.
Can I also request you to add a high level comment for the Less() function ? thx!
sungwonh
force-pushed
the
deterministic-scale-down
branch
from
0237a28 to
1aecc00
Compare
Use Slot number as a tie-breaker Signed-off-by: Sungwon Han <sungwon.han@navercorp.com>
sungwonh
force-pushed
the
deterministic-scale-down
branch
from
1aecc00 to
dfee0c4
Compare
|
@anshulpundir updated according to your comments. |
5 participants
This PR adds support for making the task termination order deterministic when scaling down.
Please refer to #2264 for more details. (closes #2264)