RateLimiter rate type does not match TocketBucketGroup rate

[tarmath]

RateLimiter's rate is a Float, while that of TocketBucketGroup is a Double.

Since RateLimiter's rate is only used to create the TokenBucketGroup, it should likely be a Double as well...?

[tarmath]

If I may also ask at the same time, I am curious if you have a general recommendation / experience on the maximum number of buckets you would generally recommend this solution for?

Thanks for this great library!

[sief]

thanks @tarmath. The rate parameter in the constructor of TokenBucketGroup was changed to Double because it is later used to calculate the ratePerNano, it saves a call to toDouble. But with the require(rate >= 0.000001f) it is questionable if ratePerNano has to be a Double in the first place.

[sief]

I still think it's safer to keep it as is, somebody might change or remove the require without taking taking this into account.

[sief]

Regarding the number of buckets: the only limit I can think of is system resources. The bucket map only contains buckets that are not full, so under normal conditions it should be empty. I did some stress tests a while ago and the impact on throughput seemed to be negilible. I've been using this lib in production with multiple TokenBucketGroup instances for many years and have not encountered any memory or other resource consuption problems so far.

[tarmath]

It does seem a bit strange to keep it a mix of Float and Double rather than consolidate on one (no matter which), but happy to close this issue regardless as that's not something that would cause issues in this context anyhow...

[tarmath]

(assuming you were waiting for me to close it)

[tarmath]

just noticed, similarly: Bucket size is Int, but consume returns a Long...

[sief]

bucket size seems to be Long, too:

class TokenBucketGroup(size: Long, rate: Double, clock: Clock = CurrentTimeClock)

What am I missing here?

[tarmath]

Right, but when creating these through RateLimiter, size is Int and rate is Float:

https://github.com/sief/play-guard/blob/master/module/app/com/digitaltangible/playguard/RateLimitActionFilter.scala#L152-L156

yet the only thing these 2 arguments do, is be passed to the TockenBucketGroup...

[tarmath]

As a sidenote, I am currently returning in the response headers the limit and number of remaining tokens, somewhat mimicking github's v3 APIs as documented here:

https://developer.github.com/v3/rate_limit/#response

In order for me to return the limit (aka bucket size), I currently have to hold that value somewhere else since it is not exposed by the RateLimiter / TocketBucketGroup.

Simply declaring size as val would be enough to make this work. Perhaps something you might be interested in changing?

Thank you again!

[sief]

right, the RateLimiter should use Long for the size, there is no require limit on size anyway.

Declaring size as val is fine for me, I'd even make rate a val as well.

Are you interested in making a PR?

[tarmath]

I already would have, but my hands are tied unfortunately...

[sief]

ok, done, I changed RateLimiter.rate to Double as well ;)

[tarmath]

oh great! fantastic! much appreciated! 🙏

[tarmath]

can these changes be released?

[sief]

releasing to maven central is quite a hassle, I'll see what I can do next week.