-
-
Notifications
You must be signed in to change notification settings - Fork 843
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
Add NewThrottle #427
base: master
Are you sure you want to change the base?
Add NewThrottle #427
Conversation
I think it has been implemented already: |
Throttle and debounce work slightly differently. |
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.
I agree with you. This helper seems useful.
retry.go
Outdated
|
||
if th.needInvoke || forcePurge { | ||
for _, f := range th.callbacks { | ||
go f() |
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.
by default, i would make it synchronous and let the developer trigger a goroutine
WDYT ?
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.
can you explain why you trigger the callbacks when calling purge() ?
IMO, to force a call, we would code something like this:
throttle()
throttle() // skipped
throttle() // skipped
reset()
throttle()
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.
by default, i would make it synchronous and let the developer trigger a goroutine
WDYT ?
Your way is more simple and configuable,
I'll fix it that way.
can you explain why you trigger the callbacks when calling purge() ?
IMO, to force a call, we would code something like this:
throttle() throttle() // skipped throttle() // skipped reset() throttle()
I wanted to offer a function to force triggering callbacks and reset current interval schedule.
The usage of it is exactly same as you mentioned.
I thought, name 'reset' might imply resetting interval schedule, but not including calling the functions
But, If 'reset' is more appropriate conventionally, it could be changed.
Just give me your opinion.
retry.go
Outdated
} | ||
} | ||
|
||
func (th *throttle) purge(forcePurge bool) { |
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.
sed /purge/reset/
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.
I change the function to 'reset'.
|
||
// NewThrottle creates a throttled instance that invokes given functions only once in every interval. | ||
// This returns 2 functions, First one is throttled function and Second one is purge function which invokes given functions immediately and reset interval timer. | ||
func NewThrottle(interval time.Duration, f ...func()) (func(), func()) { |
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.
I'm also think about a NewThrottle(100ms, 3, cb)
that would trigger the first 3 calls, then ignore others during 100ms. Like we do for a basic rate limiter.
Maybe it is a different helper.
WDYT?
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.
I'm also think about a
NewThrottle(100ms, 3, cb)
that would trigger the first 3 calls, then ignore others during 100ms. Like we do for a basic rate limiter.Maybe it is a different helper.
WDYT?
That would be a interesting option, but I am also afraid that might makes the function's signature complicated.
So I think, it's better to be implemented in other helper or function with different signature.
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.
2 helpers NewThrottle
and NewThrottleWithCount
?
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.
NewThrottleWithCount
looks nice,
I'll add it
a3ad285
to
819fbcc
Compare
819fbcc
to
1b95dfb
Compare
1b95dfb
to
b5654cf
Compare
Description
This PR adds lodash style 'throttle'.
NewThrottle function create a throttled instance that grants given functions are invoked only once in each interval.
It returns 2 functions, First one is throttled function and Second one is purge function which invokes given functions immediately and reset interval timer.
Related Issue
#396
Thank you.