using standard atomic instead of uber atomic #16394
Conversation
Signed-off-by: Dave Canton <dvcanton7@gmail.com>
Signed-off-by: Dave Canton <dvcanton7@gmail.com>
|
@bwplotka can you take this? |
Signed-off-by: Dave Canton <dvcanton7@gmail.com>
Signed-off-by: Dave <dvcanton7@gmail.com>
|
I have noticed some methods -- such as Inc, Dec, Sub -- are not available anymore in the standard atomic. I will fix this later (possibly after Kubecon). |
standard atomic types Signed-off-by: Dave Canton <dvcanton7@gmail.com>
Signed-off-by: Dave Canton <dvcanton7@gmail.com>
Signed-off-by: Dave Canton <dvcanton7@gmail.com>
There was a problem hiding this comment.
Epic work, thanks!
It looks solid, but we could make it more DRY and maintainable for everyone.
What about creating a new utils/atomicutil package that:
- Contains generic atomic.Value[T] struct?
- Have
NewValue(obj T) *Value[T]that returns pointer.
This should make our life easier, what do you think?
For integer Inc, Dec, Sub I think it's ok to have a bit more complex use without wrapping.
| - pkg: "sync/atomic" | ||
| desc: "Use go.uber.org/atomic instead of sync/atomic" |
There was a problem hiding this comment.
Nice catch!
Can we actually NOT remove it but reverse the assertion?
Use sync/atomic instead of go.uber.org/atomic"
| @@ -152,10 +152,24 @@ type AlertingRule struct { | |||
| func NewAlertingRule( | |||
| name string, vec parser.Expr, hold, keepFiringFor time.Duration, | |||
| labels, annotations, externalLabels labels.Labels, externalURL string, | |||
| restored bool, logger *slog.Logger, | |||
| _restored bool, logger *slog.Logger, | |||
There was a problem hiding this comment.
It's not idiomatic to start variable names with underscore only to avoid collisions. Let's use this opportunity to make it clear what all variable controls e.g. by renaming var restored atomic.Bool to restoredAtomic or this variable to nonAtomic...
| _restored bool, logger *slog.Logger, | |
| nonAtomicRestored bool, logger *slog.Logger, |
| 8000 | ) *AlertingRule { | |
| el := externalLabels.Map() | ||
|
|
||
| var restored atomic.Bool |
There was a problem hiding this comment.
This is great.
- I suggest we group all vars in multiline
var ( ... )statement, then restore things. - If we see this pattern of taking pointers common, I think we could consider
util/atomicutilpackage creation for our helpers?
There was a problem hiding this comment.
Oh I definitely see this being pattern in other files 🙈
I would suggest doing (2).
| @@ -302,7 +321,7 @@ func (r *AlertingRule) SetEvaluationTimestamp(ts time.Time) { | |||
|
|
|||
| // GetEvaluationTimestamp returns the time the evaluation took place. | |||
| func (r *AlertingRule) GetEvaluationTimestamp() time.Time { | |||
| return r.evaluationTimestamp.Load() | |||
| return r.evaluationTimestamp.Load().(time.Time) | |||
There was a problem hiding this comment.
What about adding small helpers in util/atomicutil package for those type safe operations? Perhaps using generics?
This solves issue #14866
Solved with @khanhtc1202 as part of the Contribfest Workshop at Kubecon Europe 2025.
Thanks to @ArthurSens @bwplotka @vesari @beorn7.