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

Implement token-bucket algorithm #55

Closed
marcosbarbero opened this issue Feb 13, 2018 · 9 comments
Closed

Implement token-bucket algorithm #55

marcosbarbero opened this issue Feb 13, 2018 · 9 comments

Comments

@marcosbarbero
Copy link
Owner

@lchayoun I'm thinking about to change our main implementation to use a token-bucket instead.
Here's a link to take a look https://github.com/bbeck/token-bucket.

WDYT?

@lchayoun
Copy link
Collaborator

I think it is a good direction, not sure about this implementation.
bucket4j has good integration with spring and is out there for quite a while

@marcosbarbero
Copy link
Owner Author

Good to know.
I’ll take a look ASAP.

@marcosbarbero
Copy link
Owner Author

@lchayoun did you see those examples?

@marcosbarbero marcosbarbero added this to To Do in Zuul RateLimit via automation Feb 16, 2018
@lchayoun
Copy link
Collaborator

Saw them, I think the main issue is the configuration required because it is not zuul specific.
I think we should support it as backend, we'll gain the support for all the backends supported by bucket4j

@marcosbarbero
Copy link
Owner Author

Don't you think it would be better to entirely move the ratelimit algorithm to a bucket based one? Doesn't necessarily needs to be bucket4j.

@lchayoun
Copy link
Collaborator

Well, since buckets doesn't work natively with time limitation, the filters are still required.
Also redis is a very good backend and I think it need to remain.
Adding bucket4j as RateLimiter implementation will give us the logic of it with the backends supported without losing any capability.

@marcosbarbero
Copy link
Owner Author

Actually that's the main idea, I'm not planning to remove any supported backend just to move the logic towards a bucked based algorithm.

@lchayoun
Copy link
Collaborator

not sure I understood, maybe we are saying the same.
I think that it should be added first as another backend without modifying the existing ones and later on change merge them.

@marcosbarbero
Copy link
Owner Author

I think we are actually saying the same thing :D

@lchayoun lchayoun self-assigned this Apr 4, 2018
@lchayoun lchayoun closed this as completed Apr 7, 2018
Zuul RateLimit automation moved this from To Do to Done Apr 7, 2018
lchayoun pushed a commit that referenced this issue Apr 7, 2018
lchayoun pushed a commit that referenced this issue Apr 7, 2018
lchayoun pushed a commit that referenced this issue Apr 7, 2018
lchayoun pushed a commit that referenced this issue Apr 7, 2018
lchayoun pushed a commit that referenced this issue Apr 7, 2018
lchayoun added a commit that referenced this issue Apr 8, 2018
* #55 Implement token-bucket algorithm

* #55 Implement token-bucket algorithm

* #55 Implement token-bucket algorithm

* #55 Implement token-bucket algorithm

* #55 Implement token-bucket algorithm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

2 participants