Skip to content
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

Merged
merged 6 commits into from
Aug 24, 2022
Merged

redis throttle done #156

merged 6 commits into from
Aug 24, 2022

Conversation

ansakharov
Copy link
Contributor

Fixes #82

@ansakharov ansakharov requested a review from vano144 August 9, 2022 12:47
@ansakharov ansakharov force-pushed the 82-redis-restored-pr branch 2 times, most recently from c8c7b42 to 8ddafff Compare August 9, 2022 15:16
@ansakharov ansakharov force-pushed the 82-redis-restored-pr branch from 8ddafff to 2e93f25 Compare August 9, 2022 15:28
plugin/action/throttle/README.md Outdated Show resolved Hide resolved
plugin/action/throttle/throttle.go Outdated Show resolved Hide resolved
@ansakharov ansakharov force-pushed the 82-redis-restored-pr branch 7 times, most recently from 92c7a86 to e004a87 Compare August 12, 2022 18:34
@ansakharov ansakharov force-pushed the 82-redis-restored-pr branch from e004a87 to d15efb5 Compare August 15, 2022 11:39
plugin/action/throttle/throttle.go Outdated Show resolved Hide resolved
@ansakharov ansakharov force-pushed the 82-redis-restored-pr branch from a817bc7 to 3d81555 Compare August 16, 2022 15:40
plugin/action/throttle/redis_limiter.go Outdated Show resolved Hide resolved
plugin/action/throttle/redis_limiter.go Outdated Show resolved Hide resolved
plugin/action/throttle/throttle.go Outdated Show resolved Hide resolved
plugin/action/throttle/throttle.go Outdated Show resolved Hide resolved
@ansakharov ansakharov force-pushed the 82-redis-restored-pr branch 6 times, most recently from ea09b00 to 270eb94 Compare August 17, 2022 20:12
@ansakharov ansakharov force-pushed the 82-redis-restored-pr branch 3 times, most recently from dc9dfe4 to 7b68338 Compare August 18, 2022 17:07
@ansakharov ansakharov force-pushed the 82-redis-restored-pr branch from 7b68338 to 0d095b1 Compare August 19, 2022 13:59
limitersMu.Lock()
wg := sync.WaitGroup{}
// run syncer at single throttle for one pipeline
if _, ok := limiterSyncMap[p.pipeline]; !ok {
Copy link
Collaborator

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:

  1. extract condition contents to separate function,
  2. don't use else
  3. you don't need sync.Map as it already under mutex
  4. use map[string]bool — we don't need to optimize memory here
  5. 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()

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

@ansakharov ansakharov Aug 22, 2022

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

  1. code separated is func
  2. else removed
  3. it's not sync.Map, renamed to avoid ambiguity
  4. leaved as is
  5. changed comment

plugin/action/throttle/redis_limiter.go Outdated Show resolved Hide resolved
plugin/action/throttle/redis_limiter.go Outdated Show resolved Hide resolved
@ansakharov ansakharov force-pushed the 82-redis-restored-pr branch 4 times, most recently from 86359c3 to e095ae9 Compare August 22, 2022 14:49
@ansakharov ansakharov force-pushed the 82-redis-restored-pr branch from e095ae9 to 8044ac2 Compare August 22, 2022 15:45
@ansakharov ansakharov merged commit 7adef62 into master Aug 24, 2022
@ansakharov ansakharov deleted the 82-redis-restored-pr branch August 24, 2022 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: redis throttle
3 participants