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

[issue #752] replace go-rate rate limiter with a buffered channel implementation #799

Merged
merged 4 commits into from
Jul 13, 2022

Conversation

zzzming
Copy link
Contributor

@zzzming zzzming commented Jun 30, 2022

Fixes #752

Motivation

The go rate limiter library, beefsack/go-rate, is an MIT license. We need update the license. The PR removes this dependency.

Modifications

In Go, it is rather simple to implement a rate limiter directly. A buffered channel is used to store a request timestamp as message. The channel receiver check each timestamp (in the message) to sleep on the remaining time duration.

The per second rate is the size of channel buffer. If the buffer is full, the main producing go routine blocks on producing Pulsar message, until the sleep expires and move to on the next message in the channel thus free up a slot in the channel.

It resembles leaky bucket rate limiting implementation. Because of leveraging go channel, it uses less than 10 lines of code and remove the dependency entirely. Since the implementation only uses one buffered channel, avoid using expensive mutex lock/unlocking and a list by the original library implementation.

Verifying this change

Since the change only affects the perf utility, I built the tool and verify the producing rate is accurate.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes)
    Removes beefsak/go-rate dependency
  • The public API: ( no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable / docs / GoDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

Copy link
Contributor

@pgier pgier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of removing the dependency, just left a few in-line comments.

var rateLimiter *rate.RateLimiter
if produceArgs.Rate > 0 {
rateLimiter = rate.New(produceArgs.Rate, time.Second)
rateLimitCh := make(chan time.Time, produceArgs.Rate)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the buffer size here is proportional to the rate, should there be a limit on the max size? Is there a way to do this with a fixed buffer size?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The buffer size is the rate per second. It is a leaky bucket rate limiter implementation. So the channel buffer should hold the same number of rate allowed.

perf/perf-producer.go Outdated Show resolved Hide resolved
perf/perf-producer.go Outdated Show resolved Hide resolved
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MIT license is perfectly fine to be used here: https://www.apache.org/legal/resolved.html#category-a

The problem in that issue was to upgrade to a version with MIT license (old license was GPL).

@pgier
Copy link
Contributor

pgier commented Jul 1, 2022

@merlimat I think the idea @zzzming had was that removing the dependency also solves the license issue, and it's about the same amount of code anyway.

@merlimat merlimat merged commit 6a8e7f3 into apache:master Jul 13, 2022
@merlimat merlimat added this to the v0.9.0 milestone Jul 13, 2022
tisonkun added a commit to tisonkun/pulsar-client-go that referenced this pull request Mar 2, 2023
go-rate is licensed under GPLv3, which is not compatible with ALv2.

It has been already removed in
apache#799 but we don't update
all references.

Thank @artursouza for spotting this.

Signed-off-by: tison <[email protected]>
tisonkun added a commit that referenced this pull request Mar 2, 2023
go-rate is licensed under GPLv3, which is not compatible with ALv2.

It has been already removed in #799 but we don't update
all references.

Thank @artursouza for spotting this.

Signed-off-by: tison <[email protected]>
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.

Update license txt files to reflect updated dependency (go-rate)
3 participants