-
-
Notifications
You must be signed in to change notification settings - Fork 311
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
WIP Introduction of RateLimitSemaphoreBackPressureHandler #1316
base: main
Are you sure you want to change the base?
WIP Introduction of RateLimitSemaphoreBackPressureHandler #1316
Conversation
df842ba
to
e1ee829
Compare
Hey @jeroenvandevelde, thanks for the PR! One question - how is the I think a design goal we should keep in mind both for this PR and for #1308 is that this is an inherently complex area of the solution, so we should make an effort to keep complexity at check. For this PR, this might mean trying to reuse some of the original Also, I'm not sure if in your discussion with @loicrouchon you came to a conclusion whether rate limiting might be a special case of his proposal, and in that case if we should try to come up with a single abstraction to enhance back pressure capabilities. Please let me know your thoughts, thanks. |
I picked the I made this PR purely to show the concept, but i will take a look at both and compare.
Completely agree on the fact that this code needs some love, by either extracting another class and share some logic or ...
In the discussion with @loicrouchon, i pointed out that we are trying to solve separate challenges of rate limiting (dynamic, static, time-based and non-time-based). Static - this one - is where the rate limit stays the same, the dynamic one - #1308 - where the rate limit is dynamically changed based on events that happen outside of the library and time-based - also this one - where the rate limit is depending on time like 5 messages per second. In time-based rate limiting there is a need to keep track of how many messages have been requested in a certain timeframe. This is necessary to know if you go over the rate limit with requesting 5 more messages or not. #1308 is for example a non-time-based dynamic rate limiter, the rate limiter is dynamically changed based on input from outside of the library. Non-time-based because there is no time involved but the rate limiter decides if the library retrieves 0 x the normal amount of messages, 0,5 x the normal amount of messages or ... I would like to keep the complexity of how many messages have been requested in a certain timeframe in the library so that users don't need to implement this every time they need a time-based rate limiter. How we can best align both cases of rate limiting is currently unclear to me. |
📢 Type of change
📜 Description
Related to issue #481 and discussion #479, this is purely to show the concept.
Still need to add tests, polish it, ...
💡 Motivation and Context
This enables the ability to read messages from a queue with a max amount of messages per second.
This is done for rate limiting purposes to for example protect a target resource.
This is further described in discussion #479 and in the comments of the discusion. #479 (reply in thread).
💚 How did you test it?
📝 Checklist
🔮 Next steps