refactor(monitor): replace RWMutex with atomic.Pointer for snapshot management #2022
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## reboot #2022 +/- ##
==========================================
+ Coverage 89.78% 93.15% +3.36%
==========================================
Files 17 17
Lines 999 1008 +9
==========================================
+ Hits 897 939 +42
+ Misses 81 54 -27
+ Partials 21 15 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sthaha
force-pushed
the
refactor-monitor-snapshot
branch
2 times, most recently
from
005cd50 to
11a01a6
Compare
sthaha
force-pushed
the
refactor-monitor-snapshot
branch
2 times, most recently
from
e835739 to
582546b
Compare
| if prevSnapshot == nil { | ||
| prevSnapshot = NewSnapshot() | ||
| } |
There was a problem hiding this comment.
cant we simplify it to
if prevSnapshot == nil {
snapshot := NewSnapshot()
snapshot.Timestamp = time.Now()
pm.snapshot.Store(snapshot)
return err
}
otherwise both prevSnapshot and newSnapshot are same
There was a problem hiding this comment.
sorry, I am not getting it :(
When monitor starts, we don't allocate any memory for the snapshot. In the computePower we ensure that previousSnapshot which is going to be referred from now on has a blank (0 ed out) snapshot. We specifically don't want to set the timestamp of the previous snapshot and it must be 0. This information is useful to compute the first - collection.
internal/monitor/node.go
Outdated
| pm.snapshotMu.RUnlock() | ||
|
|
||
| if prevReadTime.IsZero() { | ||
| func (pm *PowerMonitor) calculateNodePower(snapshot *Snapshot, prevNode *Node) error { |
There was a problem hiding this comment.
shouldnt computeNodePower have prev and current *Node as parameters?
There was a problem hiding this comment.
+1 why not .. we can change it back if other snapshot members need to be accessed.
internal/monitor/node.go
Outdated
|
|
||
| if prevReadTime.IsZero() { | ||
| func (pm *PowerMonitor) calculateNodePower(snapshot *Snapshot, prevNode *Node) error { | ||
| if prevNode == nil || prevNode.Timestamp.IsZero() { | ||
| // No previous data, nothing to do | ||
| return pm.firstNodeRead(snapshot) |
There was a problem hiding this comment.
this can still trigger pm.signalNewData()
There was a problem hiding this comment.
yes, it can. Something wrong with that?
…anagement This commit replaces sync.RWMutex with atomic.Pointer for thread-safe snapshot access in PowerMonitor. * Move zone initialization to initZones method and defer snapshot creation. * Update calculateNodePower to accept previous node data, decoupling it from snapshot locking. * update tests to reflect new logic. Signed-off-by: Sunil Thaha <[email protected]>
Signed-off-by: Sunil Thaha <[email protected]>
sthaha
force-pushed
the
refactor-monitor-snapshot
branch
from
582546b to
80f91e4
Compare
| if prevSnapshot == nil { | ||
| prevSnapshot = NewSnapshot() | ||
| } |
This commit replaces sync.RWMutex with atomic.Pointer for thread-safe snapshot
access in PowerMonitor.