-
Notifications
You must be signed in to change notification settings - Fork 84
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
redis throttle done #156
redis throttle done #156
Conversation
c8c7b42
to
8ddafff
Compare
8ddafff
to
2e93f25
Compare
92c7a86
to
e004a87
Compare
e004a87
to
d15efb5
Compare
a817bc7
to
3d81555
Compare
ea09b00
to
270eb94
Compare
dc9dfe4
to
7b68338
Compare
7b68338
to
0d095b1
Compare
plugin/action/throttle/throttle.go
Outdated
limitersMu.Lock() | ||
wg := sync.WaitGroup{} | ||
// run syncer at single throttle for one pipeline | ||
if _, ok := limiterSyncMap[p.pipeline]; !ok { |
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.
make this code more readable:
- extract condition contents to separate function,
- don't use
else
- you don't need sync.Map as it already under mutex
- use map[string]bool — we don't need to optimize memory here
- what does
run syncer at single throttle for one pipeline
mean?run sync only once per pipeline
?
e.g
limitersMu.Lock()
if !limiterSyncMap[p.pipeline] {
limiterSyncMap[p.pipeline] = true
runSync(p.pipeline)
}
limitersMu.Unlock()
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.
About point num 3: it's not sync map, just named sync to show it's purpose. I will rename it to avoid ambiguity
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.
Point 5: Correct, I meant run sync only once per pipeline
, I renamed with this suggestion.
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.
All done except point 4: map[string]struct{} suits better then map[string]bool
- code separated is func
else
removed- it's not sync.Map, renamed to avoid ambiguity
- leaved as is
- changed comment
86359c3
to
e095ae9
Compare
e095ae9
to
8044ac2
Compare
Fixes #82