agent/core: Allow setting goal CU in tests #1129
Conversation
No changes to the coverage.
HTML Report |
| } | ||
|
|
||
| // one level of indirection below State so that the fields can be public, and JSON-serializable | ||
| type state struct { | ||
| type state[A AlgorithmState] struct { |
There was a problem hiding this comment.
Is there a reason why this has to be a type parameter, compared with just having an interface-typed field?
There was a problem hiding this comment.
Yeah, otherwise callers of (*core.State).UpdateAlgorithmState() would have to downcast to the specific type in order to do the updates — possible, but I figured why bother downcasting when we can just use the exact type
There was a problem hiding this comment.
Can you take a quick look at #1140? I wanted to try to implement the same change, but without introducing generics, so that the impact is minimal.
There was a problem hiding this comment.
Ah, I see, the idea there is that the algorithm doesn't store the metrics? So, the "manual CU" algorithm would have shared state that's updated externally?
There was a problem hiding this comment.
Ah, I see, the idea there is that the algorithm doesn't store the metrics? So, the "manual CU" algorithm would have shared state that's updated externally?
Yes and yes. These are the consequences of doing the minimal possible changes.
There was a problem hiding this comment.
Ok, I see. My two cents: I think the changes here aren't too bad (it's mostly in tests), and having shared mutable state makes me nervous (specifically: changing things out from underneath another object) — but maybe that's just a holdover from my Rust programming 😅
rename:
* core.state.{Metrics => Algorithm}
* (*core.State).{UpdateMetrics => UpdateAlgorithmState}
* core.StdAlgorithm.{System => SystemMetrics}
* core.StdAlgorithm.{LFC => LFCMetrics}
This is kind of a second take on #737, and a pre-req to #729 so that we can freely change how metrics are interpreted without needing to rewrite our unit tests in 'pkg/agent/core/state_test.go'.