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.

Sign up for GitHub

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

Jump to bottom

[rate-limit] Multi limits using the same type of limiter. #825

Merged
merged 3 commits into from
Aug 3, 2018
Merged

[rate-limit] Multi limits using the same type of limiter. #825

merged 3 commits into from
Aug 3, 2018

Conversation

y-tabata
Copy link
Contributor

When I define multipul limits using the same type of limiter,

"fixed_window_limiters" : [
  { "limit 1" },
  { "limit 2" }
]

The following error message is displayed.

"2018/07/30 17:52:07 [error] 18007\#18007: *1 lua entry thread aborted: runtime error: ...ast/gateway/src/apicast/policy/rate_limit/rate_limit.lua:168: bad argument \#2 to 'insert' (number expected, got table)"

I fixed this by removing table.unpack.

@y-tabata y-tabata requested a review from a team as a code owner July 30, 2018 09:07
davidor
davidor reviewed Jul 31, 2018
View reviewed changes
t/apicast-policy-rate-limit.t Outdated
@@ -1169,3 +1169,64 @@ so only the third call returns 429.
[200, 200, 200, 429]
--- no_error_log
[error]

=== TEST 19: Rejected (count).
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you mention in the title or the description that this is testing a config with multiple limiters of the same type?
Without that, it's difficult to know what this test is testing that all the rest are not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

davidor
davidor reviewed Jul 31, 2018
View reviewed changes
t/apicast-policy-rate-limit.t Outdated
local redis_key2 = redis:keys('*_fixed_window_/test19_1')[1]
local redis_key3 = redis:keys('*_fixed_window_/test19_2')[1]
local redis_key4 = redis:keys('*_fixed_window_/test19_3')[1]
redis:del(redis_key1, redis_key2, redis_key3, redis_key4)
Copy link
Contributor

Choose a reason for hiding this comment

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

have you considered using the flushdb command?
https://redis.io/commands/flushdb

It might help us simplify some code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

@davidor
Copy link
Contributor

davidor commented Jul 31, 2018

Thanks for your contribution @y-tabata ! 👍

The PR looks good. I added a couple of minor comments.

Can you add an entry in the changelog under the fixed section?

@y-tabata
Copy link
Contributor Author

y-tabata commented Aug 1, 2018

@davidor thank you for reviewing! 👍
I added an entry in the changelog.

@mikz mikz merged commit 32dc29d into 3scale:master Aug 3, 2018
@y-tabata y-tabata deleted the multi-limit-same-limiter branch August 6, 2018 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikz mikz mikz approved these changes

@davidor davidor davidor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@y-tabata @davidor @mikz