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

perf(rate-limiting) only increment configured limits #2488

Merged
merged 1 commit into from
May 16, 2017

Conversation

subnetmarco
Copy link
Member

@subnetmarco subnetmarco commented May 2, 2017

  • Rate-Limiting: performance improvements to only increment the configured limits, and not all of them.

Notes: the same patch will have to be applied to the Response Rate Limiting plugin in a separate PR.

@subnetmarco subnetmarco changed the title fix(rate-limiting) only increment configured limits perf(rate-limiting) only increment configured limits May 2, 2017
Copy link
Contributor

@p0pr0ck5 p0pr0ck5 left a comment

Choose a reason for hiding this comment

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

Looks like we need to address failing tests here?

@p0pr0ck5 p0pr0ck5 added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels May 3, 2017
@p0pr0ck5 p0pr0ck5 added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels May 11, 2017
@p0pr0ck5
Copy link
Contributor

LGTM, but this is missing context (the commit message explains what is happening, but now why- is this a perf improvement? Bug fix? Behavior change?). Would like to have a second pair of eyes as well, but seems clean.

@subnetmarco
Copy link
Member Author

subnetmarco commented May 12, 2017

@p0pr0ck5 perf improvement, squashed and updated message

@subnetmarco subnetmarco force-pushed the fix/rate-limiting-increments branch from c4b11c0 to 156131b Compare May 12, 2017 01:08
@subnetmarco subnetmarco force-pushed the fix/rate-limiting-increments branch from 156131b to 705196c Compare May 12, 2017 01:10
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.

We should apply the same changes to the response-ratelimiting plugin as well.

@thibaultcha thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels May 12, 2017
@subnetmarco
Copy link
Member Author

@thibaultcha as noted in the original PR summary, that deserves a different PR, also because response rate-limiting also has deeper problems in its test suite (currently not working).

@subnetmarco subnetmarco merged commit 56b005e into master May 16, 2017
@subnetmarco subnetmarco deleted the fix/rate-limiting-increments branch May 16, 2017 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants