RSDK-9371 - Reset obj-tracker counter at given time #14
Conversation
bhaney
left a comment
bhaney
left a comment
There was a problem hiding this comment.
Nice change, a few things to make it more configurable and to prevent race conditions
| func GetTimestamp() string { | ||
| currTime := time.Now() | ||
| return fmt.Sprintf(currTime.Format("20060102_150405")) | ||
| return string(currTime.Format("20060102_150405")) |
There was a problem hiding this comment.
This is a cool way of specifying what format you want something in! Didn't know about it
| h, m, s := start.Hour(), start.Minute(), start.Second() | ||
| if itsBeen >= 24*time.Hour || | ||
| (h == t.lastReset.Hour() && m == t.lastReset.Minute() && s == t.lastReset.Second()) { | ||
| t.classCounter = make(map[string]int) |
There was a problem hiding this comment.
Since this is running in a go routine, you want to make sure about race conditions that might appear. One thing to really prevent this would be to consider using a sync.Map object instead of a simple map.
There was a problem hiding this comment.
| // Reset if it's the exact time or it's been more than 24 hours since last reset | ||
| itsBeen := start.Sub(t.lastReset) | ||
| h, m, s := start.Hour(), start.Minute(), start.Second() | ||
| if itsBeen >= 24*time.Hour || |
There was a problem hiding this comment.
Make this a variable, not jsut 24 hours. Make it default to 24, but have it be settable by an optional config. And rather than hours, use the golang ParseDuration option to interpret time strings time.ParseDuration("5h3m").
so the new optional config should default to 24 hours, but be settable with a config like
"reset_duration": "2h5m"
There was a problem hiding this comment.
You need to add a test -- once you add the reset duration option, set the reset duration to something like 10 s and then see if it works like expected
This feature was requested mainly for customer reporting. Made it so the time was configurable. Successfully tested on Mac. This module still could use some standardization but that's another problem for another PR.