-
Notifications
You must be signed in to change notification settings - Fork 4k
util,log: make util.EveryN generic #157210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This changes util.EveryN to a generic function and then - modifies log.EveryN to allow use a crtime.Mono instead of a time.Time. - updates most callers that were easy to update to use util.EveryMono We might consider removing the time.Time version of EveryN, but it is still useful in a couple of places. Epic: none Release note: None
yuzefovich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yuzefovich reviewed 27 of 27 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @Abhinav1299, @arjunmahishi, @herkolategan, and @kev-cao)
pkg/util/every_n.go line 55 at r1 (raw file):
var zero T e.Lock() if e.lastProcessed == zero || now.Sub(e.lastProcessed) >= e.N {
The change to always return true on the first call to ShouldProcess is a behavior change, and although it seems reasonable to me, it'd probably be worth extracting it into a separate commit with explanation for why.
yuzefovich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @Abhinav1299, @arjunmahishi, @herkolategan, @kev-cao, and @stevendanna)
pkg/util/every_n.go line 55 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
The change to always return
trueon the first call toShouldProcessis a behavior change, and although it seems reasonable to me, it'd probably be worth extracting it into a separate commit with explanation for why.
Or, actually, it's not a behavior change, right? nvm then
stevendanna
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @Abhinav1299, @arjunmahishi, @herkolategan, @kev-cao, and @yuzefovich)
pkg/util/every_n.go line 55 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Or, actually, it's not a behavior change, right? nvm then
Right, shouldn't be a behaviour change, at least for all real callers. The code change is a result of the fact that when using a time.Time, now.Sub(e.lastProcessed) would be >= e.N since now was a relatively large value. When using a crtime.Mono, now represents the times since the crtime package's initialisation, which is a very small value.
|
TFRT! bors r=yuzefovich |
This changes util.EveryN to a generic function and then
We might consider removing the time.Time version of EveryN, but it is still useful in a couple of places.
Epic: none
Release note: None