Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new GetChangedStrategies API to scheduler plugins so callers can retrieve scheduling-strategy deltas, and centralizes the strategy payload types used for API unmarshalling.
Changes:
- Introduces shared
SchedulingStrategy/SchedulingStrategiesResponsetypes inplugin/util. - Extends the
CustomSchedulerinterface withGetChangedStrategies()and updates implementations/mocks to compile. - Updates the Gthulhu plugin to track strategy changes across
UpdateStrategyMapcalls.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| plugin/util/util.go | Adds shared strategy DTOs for JSON/API usage. |
| plugin/internal/registry/registry.go | Extends CustomScheduler with GetChangedStrategies(). |
| plugin/gthulhu/strategy.go | Switches to using shared util strategy DTOs when fetching/unmarshalling API data. |
| plugin/gthulhu/gthulhu.go | Tracks strategy diffs and adds GetChangedStrategies() implementation. |
| plugin/gthulhu/gthulhu_test.go | Updates tests to use util strategy DTOs. |
| plugin/simple/simple.go | Adds stub GetChangedStrategies() to satisfy interface. |
| plugin/plugin_test.go | Updates test mock scheduler to satisfy interface. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // copy g.newStrategy to changed and clear g.newStrategy | ||
| changed = append(changed, g.newStrategy...) | ||
| g.newStrategy = []util.SchedulingStrategy{} | ||
| return changed, nil |
There was a problem hiding this comment.
GetChangedStrategies takes an RLock but then mutates shared state by clearing g.newStrategy. Because RLock allows concurrent readers, multiple callers can race on g.newStrategy, and this also violates the RWMutex contract (writes under read lock). Use g.strategyMu.Lock() while copying+clearing the slices (or use an atomic swap pattern) to make this thread-safe.
| g.oldStrategyMap = g.strategyMap | ||
| g.strategyMap = newMap | ||
| changed, removed := g.caculateChangedStrategies() | ||
| g.newStrategy = append(g.newStrategy, changed...) | ||
| g.removedStrategy = append(g.removedStrategy, removed...) |
There was a problem hiding this comment.
UpdateStrategyMap accumulates both g.newStrategy and g.removedStrategy, but GetChangedStrategies currently returns only the changed slice and always returns nil for the removed slice, and never clears g.removedStrategy. This will both break the intended API and can cause unbounded memory growth. Return (changed, removed) and clear both buffers once they’ve been read.
| // Campare g.oldStrategyMap and g.strategyMap and return the list of SchedulingStrategy that have changed strategies | ||
| func (g *GthulhuPlugin) caculateChangedStrategies() ([]util.SchedulingStrategy, []util.SchedulingStrategy) { |
There was a problem hiding this comment.
Typo/unclear naming: caculateChangedStrategies and the comments above it (“Campare…”) are misspelled and misleading. Rename to calculateChangedStrategies (or similar) and update the comments to accurately describe what the method returns (e.g., added/changed vs removed).
| GetPoolCount() uint64 | ||
| // SendMetrics sends custom metrics to the monitoring system | ||
| SendMetrics(interface{}) | ||
| // GetChangedStrategies returns the list of scheduling strategies that have changed since the last call |
There was a problem hiding this comment.
The interface comment doesn’t explain what each of the two returned slices from GetChangedStrategies represents. Please document the contract (e.g., first slice = added/updated strategies, second slice = removed strategies) so implementers and callers interpret the results consistently.
| // GetChangedStrategies returns the list of scheduling strategies that have changed since the last call | |
| // GetChangedStrategies returns the scheduling strategies that have changed since the last call. | |
| // The first slice contains strategies that were added or updated. | |
| // The second slice contains strategies that were removed. |
| func (g *GthulhuPlugin) GetChangedStrategies() ([]util.SchedulingStrategy, []util.SchedulingStrategy) { | ||
| changed := []util.SchedulingStrategy{} |
There was a problem hiding this comment.
GetChangedStrategies introduces new externally-consumed behavior (returning strategy deltas and clearing internal buffers), but there are no tests covering its semantics (e.g., returns changes after UpdateStrategyMap, returns removals, and returns empty on subsequent calls). Please add unit tests to lock down the API contract and prevent regressions.
1 participant
No description provided.