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

RateLimiter - consider whole operation execution time #251

Merged
merged 11 commits into from
Dec 17, 2024

Conversation

Kamil-Lontkowski
Copy link
Contributor

Closes #246

@Kamil-Lontkowski Kamil-Lontkowski changed the title Draft: RateLimiter - consider whole operation execution time RateLimiter - consider whole operation execution time Dec 10, 2024
@Kamil-Lontkowski Kamil-Lontkowski marked this pull request as ready for review December 10, 2024 14:21
@Kamil-Lontkowski
Copy link
Contributor Author

Kamil-Lontkowski commented Dec 16, 2024

Considering comment.

DurationRateLimiterAlgorithm.FixedWindow tracks number of running operations and doesn't replenish number of permits that running operations are "blocking".

DurationRateLimiterAlgorithm.SlidingWindow adds entry to log only after the operation is finished. This means that we consider end of operation as a point where we can replenish permits after per passes.

These changes mean that fixed window behaves in example as specified in the comment.

For sliding window consider this example:
We have three types of operations - instant, short(300 millis), long (3 seconds)

val rateLimiter = RateLimiter.slidingWindow(3, FiniteDuration(1, "second"), OperationDuration)

forkUserDiscard:
  result1 = rateLimiter.runOrDrop(short)
forkUserDiscard:
  result2 = rateLimiter.runOrDrop(short)
forkUserDiscard:
  result3 = rateLimiter.runOrDrop(long)
forkUserDiscard:
  sleep(1500.millis)
  result4 = rateLimiter.runOrDrop(instant)
  result5 = rateLimiter.runOrDrop(instant)
  result6 = rateLimiter.runOrDrop(instant)

After 1.5 second, two of three operations ended(at 0.3 seconds, so one full second passed since then), so we should have 2 available permits. Since at this point long operation is still running, we drop last operation and result6 is None.

Regarding leakyBucket. I don't know if it makes sense in this mode. It can work like fixed window, where we don't replenish tokens if operation is running, but I can't really wrap my head around if it makes sense.

@adamw
Copy link
Member

adamw commented Dec 16, 2024

@Kamil-Lontkowski thanks, this looks much better. I left some comments - I think it's especially worthwhile describing how the modified algorithms work (for our future selves :) ), and how they differ from the other mode counterparts.

@adamw
Copy link
Member

adamw commented Dec 16, 2024

And yes, I think the token bucket only makes sense in the original mode

emil-bar
emil-bar previously approved these changes Dec 16, 2024
@emil-bar emil-bar self-requested a review December 16, 2024 11:31
@emil-bar emil-bar dismissed their stale review December 16, 2024 11:32

Missclick

@Kamil-Lontkowski Kamil-Lontkowski force-pushed the consider-whole-operation-execution-time-RateLimite branch from 1078464 to b48dd23 Compare December 16, 2024 19:13
@adamw adamw merged commit d94cbe2 into master Dec 17, 2024
5 checks passed
@adamw adamw deleted the consider-whole-operation-execution-time-RateLimite branch December 17, 2024 16:52
@adamw
Copy link
Member

adamw commented Dec 17, 2024

Thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend rate limiter to support whole operation execution time, not just the start
3 participants