Skip to content

Conversation

@timebug
Copy link

@timebug timebug commented Nov 2, 2017

Just like resty.limit.req, but this module limit request rate using the "token bucket" method.

Interface design reference: https://github.com/juju/ratelimit

This module has been used in our internal production environment for limit request, avoiding the large number of requests from individual users affects the global system, like this:

local t0, _ = lim_global:take_available("__global__", 1)
local t1, _ = lim_single:take_available(userid, 1)

if t0 == 1 then
    return -- global bucket is not hungry
else -- exceed the maximum global design capacity
    if t1 == 1 then
        return -- single bucket is not hungry
    else
        return ngx.exit(503)
    end
end

@agentzh are you interested in this feature? or any other suggestions? thanks.

@agentzh
Copy link
Member

agentzh commented Nov 2, 2017

@timebug Thanks for your contribution. Some thoughts though:

  1. Because this library's name is lua-resty-limit-traffic, we'd better use the traffic level terminology in the API names instead of using the algorithm-level abstract terms like "tokens". IMHO.
  2. Better add to the docs some explanation on how this module is different from the existing resty.limit.req and resty.limit.count modules.
  3. Better make this module combinable with other kinds of limiters in the library through the resty.limit.traffic aggregator class.

@timebug
Copy link
Author

timebug commented Nov 3, 2017

@agentzh Thanks for the feedback!

What do you think of resty.limit.rate?

I surveyed some similar libraries, like ratelimit for golang, node-rate-limiter for node.js and ratelimit for rust, they all use token-bucket as the limit algorithm. And I think the word rate is also closer to the traffic level terminology. Of course there are some libraries, like uber-go/ratelimit and RateLimiters for .NET, use leaky-bucket or use both algorithm implementation, and in this repo, resty.limit.req design goal is a counterpart of the Nginx module limit_req, so I don't think it's a conflict here.

No problem, I will add to the docs some explanation on how this module is different from the existing modules later today. Thanks for your advice.

I'm sorry I didn't consider it when I submitted the Pull Request, I looked at the implementation of the resty.limit.traffic aggregator class just now, to make this module combinable with it, the current need to add two methods, they are incoming(self, key, commit) and uncommit(self, key). My initial idea is something like this:

function _M.incoming(self, key, commit)
    return _M.take(self, key, 1, self.max_wait, commit) -- max_wait may need to configure before this
end

function _M.take(self, key, count, max_wait, commit, fake_now)
    -- something else

    if is_num(max_wait) and wait_time > max_wait then
        if commit then
            update(self, key, avail, last)
        end

        return nil, "rejected"
    end

    return wait_time / 1000
end

Implement incoming method based on existing interface take, because the take interface has some additional parameters to configure, like count and max_wait. Is it appropriate to do so?

@agentzh
Copy link
Member

agentzh commented Nov 3, 2017

@timebug Seems good to me though I'll have to have a closer look at the code and the docs to be sure :)

@timebug
Copy link
Author

timebug commented Nov 5, 2017

@agentzh I created a new PR #27 use a new name called resty.limit.rate. And the following changes are completed:

  • replaced resty.limit.token with resty.limit.rate.
  • added explanation on how this module is different from the existing modules, see the module description section in README file.
  • new incoming and uncommit interfaces are added, make it combinable with other kinds of limiters through the resty.limit.traffic aggregator class.
  • new set_max_wait interface are added.
  • removed unnecessary interface, such as rate.
  • default disable resty.lock.
  • some interfaces have been slightly refactored, such as new and take.
  • fixed some related tests and docs.

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.

2 participants