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

WIP Introduction of RateLimitSemaphoreBackPressureHandler #1316

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jeroenvandevelde
Copy link

@jeroenvandevelde jeroenvandevelde commented Jan 1, 2025

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 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

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated reference documentation to reflect the change
  • All tests passing
  • No breaking changes

🔮 Next steps

  • Need to add documentation
  • Need to verify the open TODOs
  • Polish the code

@github-actions github-actions bot added the component: sqs SQS integration related issue label Jan 1, 2025
@jeroenvandevelde jeroenvandevelde changed the title Introduction of RateLimitSemaphoreBackPressureHandler WIP Introduction of RateLimitSemaphoreBackPressureHandler Jan 1, 2025
@jeroenvandevelde jeroenvandevelde marked this pull request as draft January 1, 2025 20:41
@tomazfernandes
Copy link
Contributor

Hey @jeroenvandevelde, thanks for the PR!

One question - how is the TimedSemaphore different from Guava's RateLimiter and why did you choose this one?

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 BackPressureHandler logic where appropriate rather than duplicating it, perhaps extracting some of the logic to a third class, or any other approach that makes sense.

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.

@jeroenvandevelde
Copy link
Author

jeroenvandevelde commented Jan 2, 2025

Hi @tomazfernandes

One question - how is the TimedSemaphore different from Guava's RateLimiter and why did you choose this one?

I picked the TimedSemaphore to stick as close as possible to the current implementation with Semaphores.
I haven't done an in-depth check which one makes the most sense for this use-case.

I made this PR purely to show the concept, but i will take a look at both and compare.
Any preference for either the TimedSemaphore from apache or Guava's RateLimiter from your end?

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 BackPressureHandler logic where appropriate rather than duplicating it, perhaps extracting some of the logic to a third class, or any other approach that makes sense.

Completely agree on the fact that this code needs some love, by either extracting another class and share some logic or ...
I just made this PR to showcase the idea, so i will definitely improve the design once we agree on a way forward.

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.

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.
I need to think this through a bit more in the coming period.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: sqs SQS integration related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants