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

feat: Simple load balanced rate control #19

Merged
merged 1 commit into from
Jan 15, 2022
Merged

feat: Simple load balanced rate control #19

merged 1 commit into from
Jan 15, 2022

Conversation

LeipeLeon
Copy link
Contributor

By introducing the option balanced: true we interleave the initial ring w/ the interval divided by size so all calls will have some air between each call e.g. a simple load balancing.


By default all calls to the limit_method will be bursted, e.g. as quick as possible, until the rate is exceeded.
Then we wait for the remainder of the interval to continue. To even out the burst, an optional balanced parameter can be
provided to enable interleaving between the method calls, e.g: interleave = interval / size.

  ...
  limit_method :tick, rate: 60, balanced: true
  ...

For example: with an interval of 60 seconds and a rate of 60:

balanced: false As quickly as possible we call the method 60 times, then we wait for the remainder of the time.

balanced: true We interleave each call with 1 second so we call this method every second.


Also fixed in this PR:

  • chore(package): Use latest version of rubocop
  • use ruby version 2.4 and up in travis and rubocop
  • chore(package): Add rubocop-minitest and rubocop-rake as suggested
  • fix rubocop warnings

Considerations before merging this PR:

  • Use interleaved instead of balanced as an option name
  • Make the interleave configurable: e.g. instead of true/false provide
    an float/integer (which also favours renaming to interleave)
  • ditch this PR because when this is an issue your rate/interval is wrong.

@kvokka
Copy link
Contributor

kvokka commented Jun 20, 2021

This is a great feature and hope it will be merged!

@LeipeLeon
Copy link
Contributor Author

Just rebased branch and removed rubocop upgrades (b/c out of scope for the pull request)

Let me know if there is anything I need to do to get this accepted!

Cheers!

By introducing the option `balanced: true` we interleave the initial ring w/ the interval divided by size so all calls will have some air between each call e.g. a simple load balancing.

---

By default all calls to the `limit_method` will be bursted, e.g. as quick as possible, until the rate is exceeded.
Then we wait for the remainder of the interval to continue. To even out the burst, an optional `balanced` parameter can be
provided to enable interleaving between the method calls, e.g: `interleave = interval / size`.

``` ruby
  ...
  limit_method :tick, rate: 60, balanced: true
  ...
```

For example: with an interval of 60 seconds and a rate of 60:

`balanced: false` As quickly as possible we call the method 60 times, then we wait for the remainder of the time.

`balanced: true` We interleave each call with 1 second so we call this method every second.

---

Considerations before merging this PR:

- Use `interleaved` instead of `balanced` as an option name
- Make the interleave configurable: e.g. instead of `true/false` provide
  an `float/integer` (which also favours renaming to `interleave`)
- ditch this PR because when this is an issue your rate/interval is wrong.
@LeipeLeon
Copy link
Contributor Author

Rebased on master and forced pushed.

Would love to know if this has any chance of merging or otherwise I will close this PR

@LeipeLeon
Copy link
Contributor Author

@sbfaulkner Can you share a light on this please?

@sbfaulkner
Copy link
Contributor

seems reasonable... I'll try to get to full review later today

@sbfaulkner sbfaulkner merged commit d440f04 into Shopify:master Jan 15, 2022
@shopify-shipit shopify-shipit bot temporarily deployed to production January 15, 2022 03:31 Inactive
@sbfaulkner
Copy link
Contributor

thanks... should be published as v2.2.2

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.

3 participants