failsafe-lib / failsafe

Fault tolerance and resilience patterns for the JVM
https://failsafe.dev
Apache License 2.0
4.17k stars 296 forks source link

Create rate limiter policy #308

Closed jhalterman closed 2 years ago

jhalterman commented 2 years ago

See #78 for some previous notes.

The rate limiter implementation will allow a max number of executions per time period (ex: 10 / second). There are two options for how this could work:

Other things

jhalterman commented 2 years ago

Open question:

Any preference as to whether methods that block while trying to acquire a rate limiter permit, ex: RateLimiter.acquirePermit(), should throw InterruptedException when the interrupted or just set the Thread's interrupt flag? @Tembrel I'm of course curious what you think about this one.

https://github.com/failsafe-lib/failsafe/blob/ratelimiter/src/main/java/dev/failsafe/RateLimiter.java#L81

Tembrel commented 2 years ago

Better to have blocking methods throw IE as the default position, and maybe add -Uninterruptibly variants if there's demand for that. The longer name for the uninterruptible version forces users to state explicitly that they are giving up on interruptibility, which means that standard task cancellation via interruption won't work. (The throws InterruptibleException, similarly, forces them to write a catch block and make a decision about how to respond, if only by logging the interruption and setting the interrupt flag.)

You could do it the other way, with uninterruptibility being the default, and adding -Interruptibly variants, but that would saddle the latter with the double whammy of an awkward name along with having to handle the exception. Users would end up using the former whether or not it was appropriate.

jhalterman commented 2 years ago

Thanks. I agree, I'm happy to wait and see if there's a demand for an uninterruptble variant.

The throws InterruptibleException, similarly, forces them to write a catch block and make a decision about how to respond

That's what I was leaning towards as well since users may not otherwise check Thread.isInterrupted(), even if it's mentioned in the Javadoc.

I was somewhat surprised that the Guava RateLimiter sets an interrupt flag when interrupted, rather than throwing InterruptedException, but perhaps that's because they intend for all of their blocking RateLimiter methods to be uninterruptable.

Tembrel commented 2 years ago

Hmm, maybe we should ask them?

jhalterman commented 2 years ago

Yea mostly it's just a curiosity. My guess was it aligns with some internal/google API design preference, but just I messaged one of the maintainers to see if he knows.

sunng87 commented 2 years ago

A max-wait option can be handy to avoid rate limit errors, just like in object pool scenario.

jhalterman commented 2 years ago

@sunng87 You mean a max-wait to acquire a permit? Here's the RateLimiter policy in progress:

https://github.com/failsafe-lib/failsafe/blob/ratelimiter/src/main/java/dev/failsafe/RateLimiter.java#L201

Tembrel commented 2 years ago

Chris Povirk's response to @jhalterman's query reinforces my sense that interruptible is the best default, with -Uninterruptibly variants added as needed.

jhalterman commented 2 years ago

Chris Povirk's response to @jhalterman's query reinforces my sense that interruptible is the best default, with -Uninterruptibly variants added as needed.

Same for me.

jhalterman commented 2 years ago

A PR is up for the RateLimiter if anyone wants to weigh in there: #313. Hopefully there's nothing too surprising.

jhalterman commented 2 years ago

An open question for the RateLimiter API in case anyone wants to weigh in (/cc @whiskeysierra since you were on the original issue):

With two types of rate limiters, we could configure them in the same way or different ways. For example, a bursty rate limiter can accept the max executions per time period:

burystBuilder(10, Duration.ofSeconds(1)) // max 10 per second

This makes sense since it uses a fixed window approach to track executions per period.

A smooth rate limiter could also accept the max executions per time period:

smoothBuilder(10, Duration.ofSeconds(1)) // max 10 per second

But in reality, it’s less interested in the max per period than the rate of individual executions, which it could derive from the max per period, or allow it could be provided explicitly:

smoothBuilder(Duration.ofMillis(100)) // 1 execution every 100 millis

This more clearly reflects what the smooth rate limiter does: it spaces out executions every 100 millis, for example. But this somewhat inverts your thinking as a user, since I imagine most rate limiting is thought of as executions per period rather than interval between executions.

Which of the two smoothBuilder options look good to you? Thoughts?