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

Improve rate limit policy #679

Closed
wants to merge 7 commits into from

Conversation

y-tabata
Copy link
Contributor

This PR improves the rate limit policy according to the comments of PR #665 and addresses to Issue #667.

@@ -10,6 +10,10 @@ local limit_traffic = require "resty.limit.traffic"
local ts = require ('apicast.threescale_utils')
local tonumber = tonumber
local next = next
local format = string.format
local insert = table.insert
local os_date = os.date
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to use ngx.time or ngx.now as they are way more performant in scope of OpenResty.

@@ -234,6 +234,7 @@ describe('Rate limit policy', function()
assert.equal('10', window)

os.execute("sleep 1")
Copy link
Contributor

Choose a reason for hiding this comment

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

Haven't noticed this earlier. There is ngx.sleep() that is better than spawning a subprocess.

@y-tabata y-tabata force-pushed the improve-rate-limit-policy branch from 57a3255 to d2ee92a Compare April 25, 2018 00:57
@y-tabata y-tabata force-pushed the improve-rate-limit-policy branch from 9c49ac2 to feae81d Compare April 27, 2018 13:24
@y-tabata
Copy link
Contributor Author

y-tabata commented May 3, 2018

@mikz The build error and s2i-builder error have nothing to do with this PR, right?

@davidor
Copy link
Contributor

davidor commented May 3, 2018

@y-tabata The error has nothing to do with your PR, sorry.

@y-tabata
Copy link
Contributor Author

y-tabata commented May 8, 2018

@mikz @davidor Is there anything I can do to help move this PR forward?

@mikz
Copy link
Contributor

mikz commented May 11, 2018

@y-tabata I'm sorry, but the recent changes introduced in #696 conflict with this PR.
Could you resolve those conflicts?

@y-tabata
Copy link
Contributor Author

@mikz Sure, no problem.

@y-tabata
Copy link
Contributor Author

move to #703

@y-tabata y-tabata closed this May 14, 2018
@y-tabata y-tabata deleted the improve-rate-limit-policy branch May 14, 2018 06:29
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