remove evil global state shared by all schedulers #5351
Conversation
|
After a ridiculous amount of effort, I finished running e2e and it appears to have passed. |
| @@ -46,20 +60,26 @@ type AlgorithmProviderConfig struct { | |||
| PriorityFunctionKeys util.StringSet | |||
| } | |||
|
|
|||
| // Registers a fit predicate with the algorithm registry. Returns the name, | |||
| // with which the predicate was registered. | |||
| // RegisterFitPredicate registers a fit predicate with the algorithm | |||
There was a problem hiding this comment.
Random question: Do we have a standard about whether you re-state the function name in the comment? Personally I don't like this style (I'd prefer "Registers a fit predicate..." as it was before) but I guess we don't have a standard practice.
There was a problem hiding this comment.
Its a godoc convention. From the blog, a godoc "comment is a complete sentence that begins with the name of the element it describes." govet issues a warning if you don't do this I believe.
There was a problem hiding this comment.
It's godoc style to repeat the function name verbatim, for whatever reason. :/
|
LGTM Personally I think factories are pretty evil too :-). I'm not sure whether this PR is a big net improvement. But if you think it is worth the somewhat extra complexity it's OK with me. |
|
Yeah I don't love this either but these Lister variables have to get made at scheduler creation time and not when the module in initialized. |
|
@davidopp it would be great to label things with satus-lgtm -- I use that to scan the PRs to work out which ones may be mergeable. Thanks. |
satnam6502
added
the
lgtm
remove evil global state shared by all schedulers
|
Satnam: The reason I didn't label it LGTM was that I was waiting for Daniel to confirm he thought adding this layer of indirection was worthwhile (sorry my comment with the LGTM wasn't clear). |
|
Thanks for merging, my #5446 builds on top of this one. |
Running e2e now. (needed for #5336)