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

perf(*-rate-limiting) optimize redis pipelining #2956

Merged
merged 2 commits into from
Oct 26, 2017
Merged

perf(*-rate-limiting) optimize redis pipelining #2956

merged 2 commits into from
Oct 26, 2017

Conversation

hbagdi
Copy link
Member

@hbagdi hbagdi commented Oct 14, 2017 •
edited
Loading

Summary

Redis policy for rate-limiting and response rate-limiting plugins uses redis pipelining feature but start and commit multiple pipelines instead of using a single one.

I messed up in a force push on my forked repo, this is same as #2932

Sorry for the confusion.

Full changelog

  • Optimize redis storage policy for rate limiting plugin by using a single pipeline
  • Optimize redis storage policy for response rate limiting plugin by using a single pipeline

@subnetmarco
Copy link
Member

subnetmarco commented Oct 16, 2017 •
edited
Loading

I am manually restarting the tests to make them pass - the failure is unrelated to this PR.

@hbagdi
Copy link
Member Author

hbagdi commented Oct 18, 2017

@thefosk @thibaultcha are there any changes required here?

@thibaultcha
Copy link
Member

@hbagdi Sorry, we've just been busy on our side with various company events and the Lua Workshop we hosted at our offices. We'll get another look at this soon!

hishamhm
hishamhm approved these changes Oct 23, 2017
View reviewed changes
Copy link
Contributor

@hishamhm hishamhm left a comment

Choose a reason for hiding this comment

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

The changes suggested by the team in the previous PR were all applied, so this is good to merge!

thibaultcha
thibaultcha approved these changes Oct 23, 2017
View reviewed changes
Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

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

Agreed. Proper commit scope will be perf(*-rate-limiting) though. Let's hold until 0.11.1 gets released.

@thibaultcha thibaultcha added the pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) label Oct 23, 2017
@hishamhm hishamhm changed the title feat(plugins) optimize redis pipelining perf(*-rate-limiting) optimize redis pipelining Oct 23, 2017
@thibaultcha thibaultcha merged commit 777b667 into Kong:master Oct 26, 2017
@thibaultcha
Copy link
Member

@hbagdi Merged - thank you for your contribution!

@hbagdi hbagdi deleted the redis-opt branch October 26, 2017 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thibaultcha thibaultcha thibaultcha approved these changes

@hishamhm hishamhm hishamhm approved these changes

Assignees
No one assigned
Labels
pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hbagdi @subnetmarco @thibaultcha @hishamhm