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

feat: add redis and redis-cluster in limit-conn #10866

Merged

Conversation

theweakgod
Copy link
Contributor

@theweakgod theweakgod commented Jan 24, 2024

Description

Fixes #10501 (#10501)

Add options for redis and redis-cluster in the limit-conn plugin, and use redis as the counter for the leaky bucket algorithm.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@theweakgod theweakgod marked this pull request as draft January 24, 2024 12:06
@shreemaan-abhishek
Copy link
Contributor

Let's bring support for limit-conn first then you can raise another PR for limit-req it's easier to review.

@theweakgod
Copy link
Contributor Author

Let's bring support for limit-conn first then you can raise another PR for limit-req it's easier to review.

I'll split it into two pull requests. I encountered an issue during development related to the management of Redis keys. I noticed that APISIX generates a significant number of Redis keys. I'm considering consolidating these keys into a hash for better management. What are your thoughts on this?

@theweakgod
Copy link
Contributor Author

@shreemaan-abhishek @monkeyDluffy6017 I have a problem now. I need to write a test case to test the plugin, but what address should I fill in for the redis cluster? How to fill in the cluster with password and the cluster without password? How to fill in the address of the redis singleton with password and without password?

@theweakgod theweakgod changed the title feat: add redis and redis-cluster in limit-conn and limit-req feat: add redis and redis-cluster in limit-conn Jan 25, 2024
@theweakgod theweakgod marked this pull request as ready for review January 25, 2024 12:36
@theweakgod theweakgod marked this pull request as draft January 25, 2024 12:37
@shreemaan-abhishek
Copy link
Contributor

@theweakgod you will find redis servers (singleton and cluster) defined as docker compose applications in ci directory. You can use them or create new ones. You can also take inspiration from existing test cases for redis policy in limit count plugin.

@theweakgod
Copy link
Contributor Author

@theweakgod you will find redis servers (singleton and cluster) defined as docker compose applications in ci directory. You can use them or create new ones. You can also take inspiration from existing test cases for redis policy in limit count plugin.

Ok.I see, thank u.

@theweakgod theweakgod marked this pull request as ready for review January 26, 2024 09:54
@theweakgod
Copy link
Contributor Author

@shreemaan-abhishek @monkeyDluffy6017 I'm ready to review

apisix/plugins/limit-conn/util.lua Outdated Show resolved Hide resolved
apisix/plugins/limit-conn/util.lua Outdated Show resolved Hide resolved
@kayx23
Copy link
Member

kayx23 commented Feb 2, 2024

@shreemaan-abhishek I checked the doc and no more comments from me.

@shreemaan-abhishek shreemaan-abhishek merged commit 7e907a5 into apache:master Feb 7, 2024
47 checks passed
illidan33 pushed a commit to illidan33/apisix that referenced this pull request Feb 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

feat: centralized counter for limit conn and limit req
4 participants