SGrondin / bottleneck

Job scheduler and rate limiter, supports Clustering
MIT License
1.81k stars 74 forks source link

reservoirRefreshTime #125

Open PuKoren opened 5 years ago

PuKoren commented 5 years ago

Feature request

Have a parameter reservoirRefreshTime where we can specify a time of the day when the reservoir should be replenished.

Why

The goal is to be able to use the library against API that have daily quotas, resetting at a time of day (ie: An API free tier that allows 2500 request/day and resets everyday at 00:00 Pacific Time).

This way we can use the lib to ensure we request the API in the free tier as much as possible for a day, and continue automatically the jobs on the next day / fallback to another provider for the rest of the day

Format

The reservoirRefreshTime could accept an UTC hour and minutes, for example 00:00

What do you think about this ?

SGrondin commented 5 years ago

I think this is a great idea!

This option would have to be mutually exclusive with reservoirRefreshInterval. I'll do it this weekend if I have a few hours free

SGrondin commented 5 years ago

Hmm... 🤔

What if your quota resets every hour?

  1. 00:12 app started, reservoir initial value = 2500
  2. 01:00 reservoir refreshed to 2500
  3. 02:00 reservoir refreshed to 2500 etc

So it seems like what we need something like reservoirRefreshStart. In your case, reservoirRefreshStart would be the UTC date+minutes (or maybe even a Date object?) and you'd set reservoirRefreshInterval to 1000*60*60*24 (1 day).

PuKoren commented 5 years ago

Hum, I just realize with this we will loose the feature of the burst control and requests rate limiting. The thing is that kind of API can have daily quota AND rate limit (like 100 req / sec max). It would be nice if this could work side-by-side.

I'm trying to think about it but for now I only have ideas that would bloat the client with more options (like globalLimit and globalLimitRefreshInterval globalLimitRefreshStart). I will think a bit more

SGrondin commented 5 years ago

Hum, I just realize with this we will loose the feature of the burst control and requests rate limiting.

I don't think so. I think with reservoirRefreshStart everything would still work the same way, but if reservoirRefreshStart is set, it will only start doing the Refresh Interval after an initial delay.

It would be similar to this:

const limiter = new Bottleneck({
  maxConcurrent,
  minTime
})

setTimeout(() => {
  limiter.updateSettings({ reservoir, reservoirRefreshInterval, reservoirRefreshAmount })
}, calculateDelay(reservoirRefreshStart))
PuKoren commented 5 years ago

It indeed works well with maxConcurrent and minTime but this does not control burst

For example, have this config now:

new Bottleneck({
        reservoir: 50,
        reservoirRefreshAmount: 50,
        reservoirRefreshInterval: 1000,
        maxConcurrent: 5,
        minTime: 20
    });

If I have an heavy burst, it can overflow my reservoir but it is now controlled the first requests will respond very fast, while others will get delayed until the bucket is replenished. I think it is a cool feature of bottleneck.

If I want to use the reservoirRefreshStart with the daily quota, I have to give up on my burst control (50 req over one sec) to put the daily quota instead, so my minTime will increase and maxConcurrent will decrease to make sure I fit the req/sec limit

It also conflicts with the leaked buckets control I think

Not sure if I'm clear, let me know :+1:

SGrondin commented 5 years ago

Ah! I understand now.

I think 95% of people use minTime/maxConcurrent to do burst control and use reservoir for quotas.

You could use 2 limiters, one for burst control and one for quotas.

const quotaLimiter = new Bottleneck({ ... })
const burstLimiter = new Bottleneck({ ... })

const noop = () => {}
const scheduleJob = async (...args) => {
  await quotaLimiter.schedule(noop)
  return burstLimiter.schedule(...args)
}

This double limiter + noop trick lets you combine 2 limiters. The quota limiter would have reservoirRefreshStart.

What do you think?

PuKoren commented 5 years ago

Ha indeed it is a good idea, it makes sense