[chore] Refactor allocation strategies #2928
Conversation
|
yeah this is super close to what i tried to do a year or two ago, and am definitely in favor of this change. The trouble I ran into back then was how you give the right context to the strategy without duplicating memory. i.e. it's not great if the strategy has to keep its own collector map and the allocator struct does too. Overall, I think the approach makes sense. I'll give it a better read later today! |
Is that a realistic problem? The collector count is going to be less than 100 in almost all practical circumstances, and we're only storing pointers anyway, so the memory cost is small. None of my strategy implementations need to store collectors anyway, the In a follow-up, I'd like to restrict what data actually gets passed to the strategy in the |
|
I was worried about the labelsets cardinality, but yeah if we're just using some pointers for only the collectors we should be alright. Either way, I would love to see some benchmarks to ensure that's the case. It feels like we've had some brush ups with memory and the TA recently. |
Ran the So |
|
WOW! yeah that per-node allocation is 🔥 great stuff. |
There was a problem hiding this comment.
I'm definitely in favor of these, but I would love a second look from @kristinapathak or @matej-g
| TargetsPerCollector.WithLabelValues(item.CollectorName, a.strategy.GetName()).Set(float64(c.NumTargets)) | ||
| } | ||
| delete(a.targetItems, k) | ||
| delete(a.targetItemsPerJobPerCollector[item.CollectorName][item.JobName], item.Hash()) |
There was a problem hiding this comment.
If this is the last value in a.targetItemsPerJobPerCollector[item.CollectorName] for the job, we could probably also delete the job entry:
if len(a.targetItemsPerJobPerCollector[item.CollectorName][item.JobName]) == 0 {
delete(a.targetItemsPerJobPerCollector[item.CollectorName], item.JobName)
}
but I don't think this has to be done in this PR.
There was a problem hiding this comment.
I ended up refactoring the deletion and unassignment routines a bit and did this as well.
| for k, v := range allocator.collectors { | ||
| collectorsCopy[k] = v | ||
| func (s *leastWeightedStrategy) GetCollectorForTarget(collectors map[string]*Collector, item *target.Item) (*Collector, error) { | ||
| // if a collector is already assigned, do nothing |
There was a problem hiding this comment.
This is sneaky but perfect for maintaining this functionality 🙂
There was a problem hiding this comment.
In a follow up, I want to remove the CollectorName field from target.Item and instead track this in a separate map in the allocator. Then we can pass this map as an argument to this function and be explicit about the dependency.
|
@jaronoff97 @kristinapathak I ended up making a fairly substantial change while addressing your feedback, so I re-requested reviews from both of you. On a separate note, should I add a changelog entry for this change? It's technically a refactor, but I've changed enough that I think it'd be fair to alert users it happened. That would make it easier to attribute any bugs I might've introduced. |
|
@swiatekm-sumo i think it's worth adding a changelog given all that's going on here. I also think it's worth mentioning the performance win in there as that's been some feedback we've gotten in the past few weeks. (maybe this issue in particular #2916) |
swiatekm-sumo
force-pushed
the
refactor-strategies
branch
from
e9a4f27
to
eaa90bc
Compare
| err := a.addTargetToTargetItems(item) | ||
| if err != nil { | ||
| assignmentErrors = append(assignmentErrors, err) | ||
| item.CollectorName = "" |
There was a problem hiding this comment.
Why do we do this here?
There was a problem hiding this comment.
Getting rid of dubious test failures, mostly. One of the problems with having CollectorName be an attribute of a target is that it's not clear if targets supplied in SetTargets can have that attribute set, and what that means. The real target pipeline never sets this, but some tests sneakily do. I set it here instead of fixing those tests, because I really wanted my changes to pass existing tests unmodified.
This is one of the things I want to fix in a follow-up.
swiatekm-sumo
force-pushed
the
refactor-strategies
branch
from
677ece7
to
41fd94d
Compare
swiatekm-sumo
force-pushed
the
refactor-strategies
branch
from
41fd94d
to
a607911
Compare
Allocation strategies are currently messy and duplicate a lot of logic. This is an attempt at making them more DRY.
I'm not entirely happy with the end result, and I've left some TODOs in the code I'd like to address in a follow-up. I haven't included those changes in this PR in the interest of keeping it simpler.