Use a min heap to store top K scoring nodes for placement metrics #4564
Conversation
helper/lib/score_heap.go
Outdated
| @@ -0,0 +1,63 @@ | |||
| package lib | |||
There was a problem hiding this comment.
Need to figure out if there's a better package for this. Adding it to the lib package at the top level caused an import cycle because this heap is used in the structs package, and other code in the existing lib package uses the structs package.
There was a problem hiding this comment.
May want to repackage the other one to be lib/delayheap
command/monitor.go
Outdated
| if len(metrics.ScoreMetaData) > 0 { | ||
| for scorer, scoreMeta := range metrics.ScoreMetaData { | ||
| for _, nodeScore := range scoreMeta { | ||
| out += fmt.Sprintf("%s* Scorer %q, Node %q = %f\n", prefix, scorer, nodeScore.NodeID, nodeScore.Score) |
There was a problem hiding this comment.
From the demo it would be nice if this output was tabular. Sorting by the normalized score first then lexicographical by node id. Example:
node | binpack | node-affinity | normalized |
4ebde21b-daa9-f264-16dd-b6ba88efdd58 | 0.080626 | 1.000000 | 0.540313 |
5f2db925-69d4-e677-d3a9-b166155153c6 | 0.080626 | 1.000000 | 0.540313 |
14ebcc35-34d0-1812-14c9-b4397d603723 | 0.080626 | 0.333333 | 0.206980 |
api/allocations.go
Outdated
| } | ||
|
|
||
| // NodeScoreMeta is used to serialize node scoring metadata | ||
| // displayed in the CLI during verbose mode |
There was a problem hiding this comment.
Mark scores as deprecated here and in the website/api docs/changelog
There was a problem hiding this comment.
How do we mark fields in json responses as deprecated?
helper/lib/score_heap.go
Outdated
| @@ -0,0 +1,63 @@ | |||
| package lib | |||
helper/lib/score_heap.go
Outdated
| @@ -0,0 +1,63 @@ | |||
| package lib | |||
There was a problem hiding this comment.
May want to repackage the other one to be lib/delayheap
nomad/structs/structs.go
Outdated
| if a.topScores == nil { | ||
| a.topScores = make(map[string]*lib.ScoreHeap) | ||
| } | ||
| scorerHeap, ok := a.topScores[name] |
There was a problem hiding this comment.
I do not believe this is the API we want. It is possible to not have all the sub scores for a the top k normalized scores.
nomad/structs/structs.go
Outdated
| // scoring nodes in descending order | ||
| for scoreHeap.Len() > 0 { | ||
| item := heap.Pop(scoreHeap).(*lib.HeapItem) | ||
| scoreMeta[i] = &NodeScoreMeta{ |
There was a problem hiding this comment.
Is it worth just adding a GetItems() and GetItemsReverse() method to the library?
Scoring metadata is now aggregated by scorer type to make it easier to parse when reading it in the CLI.
preetapan
force-pushed
the
f-score-heap
branch
from
9120fab to
f1a949a
Compare
nomad/structs/structs.go
Outdated
| func (a *AllocMetric) ScoreNode(node *Node, name string, score float64) { | ||
| if a.Scores == nil { | ||
| a.Scores = make(map[string]float64) | ||
| if a.nodeScoreMeta == nil { |
There was a problem hiding this comment.
Doesn't this have to be a.nodeScoreMeta == nil || a.nodeScoreMeta.NodeID != node.ID?
There was a problem hiding this comment.
I reset it to nil once I have the final score from the normalized scorer. But I went ahead and made this change because its safer to do it this way (since it works even if the norm scorer doesn't get invoked).
nomad/structs/structs.go
Outdated
| // The map is populated by popping elements from a heap of top K scores | ||
| // maintained per scorer | ||
| func (a *AllocMetric) PopulateScoreMetaData() { | ||
| if a.topScores != nil { |
There was a problem hiding this comment.
if a.topScores == nil { return }
Less nesting is easier to read
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR changes the placement metrics map which used to store all scored nodes, to only storing the top K nodes per scorer.
Also includes changes to the CLI, score metrics are shown in the following format