fix(keda): sort Jobs list by completions, not parallelism - #1212
fix(keda): sort Jobs list by completions, not parallelism#1212magic-peach wants to merge 2 commits into
Conversation
sortByCompletions in the Jobs list actually sorted by parallelism first and only fell back to completions on a tie, despite the name saying otherwise. Also swapped in nullish coalescing so a job missing either field doesn't turn the comparator into NaN. Exported it so it's testable on its own. Signed-off-by: Akanksha Trehun <akankshatrehun@gmail.com>
Ralthos
left a comment
There was a problem hiding this comment.
Sorting by completions matches what the column shows, since getCompletions renders
completions/parallelism and the label is Completions. Ordering on the second number while
displaying the first was the bug, and the test that pins job(1, 5) before job(5, 1) is the one
that proves the direction actually changed.
Moving it out of the component to make it importable is the right call too.
One thing you are half way to fixing. The ?? 0 guards are new here, and the function directly
above still has the unguarded version of the same problem:
function getCompletions(job: Job) {
return `${job.spec.completions}/${job.spec.parallelism}`;
}completions and parallelism are both optional in batch/v1, so a Job with neither set now
sorts as 0, which is sensible, and displays as undefined/undefined, which is not. After this PR
the two behaviours disagree about the same missing field, and the sort order will look wrong to
anyone reading the column, because the rows say undefined and sit where 0 belongs.
Cheap to close while you are in here:
export function getCompletions(job: Job) {
return `${job.spec.completions ?? 0}/${job.spec.parallelism ?? 0}`;
}Or render a dash if unset should read as unknown instead of zero. Either is defensible. What is
hard to defend is the sort treating it as 0 and the cell treating it as a string that says
undefined.
…play too good catch on review — the sort treated missing fields as 0 but the cell still rendered them as literal "undefined", so the two disagreed about what a Job with neither field set should look like. same guard, applied to the render path this time. pulled getCompletions out to module scope so it's actually testable, same as sortByCompletions already was. Signed-off-by: Akanksha Trehun <akankshatrehun@gmail.com>
|
@Ralthos could you please check now? |
|
Checked. That was the part worth closing. A cell reading |
sortByCompletions in the keda Jobs list sorted by
parallelismfirst and only fell back tocompletionson a tie, which is backwards from what the name says it does. Two jobs with the same parallelism but very different completion counts would land in the right order by luck, but anything where the two disagreed sorted wrong.Also switched to nullish coalescing on both fields — a job missing
completionsorparallelism(both are optional on the Job spec) turned the comparison intoNaN, whichArray.sorttreats unpredictably.Exported the function out of
JobsListRendererso it can actually be unit tested instead of only exercised through the rendered table.