[RateLimiter] Add support for long intervals (months and years) #43060
Conversation
alexandre-daubois
force-pushed
the
feat/rate-limiter-long-interval
branch
2 times, most recently
from
bed13d3
to
fcfde37
Compare
|
Tests please. 🙂 |
|
@derrabus indeed! Wasn't sure which tests to add though. But I'll definitely try to find some! 😄 |
|
Well, you should be able to come up with a test that fails on 5.4 and passes after your changes. After all, you're fixing a problem here. 😎 |
|
Oh yeah of course you're right! Just added them. If you see anything else, please tell me! Speaking of that, should this be rebased to 5.3 as a bug fix or is it considered as a feature and 5.4 is fine? |
alexandre-daubois
force-pushed
the
feat/rate-limiter-long-interval
branch
from
fcfde37
to
dd44a83
Compare
|
That’s a new feature |
alexandre-daubois
force-pushed
the
feat/rate-limiter-long-interval
branch
3 times, most recently
from
a3bcb2b
to
a1d2d61
Compare
|
|
||
| yield ['PT2H4M', 2 * 3600 + 4 * 60]; | ||
|
|
||
| yield ['P1Y', 24 * 3600 * 365]; |
There was a problem hiding this comment.
I would expect that this test will start to fail on March 1st, 2023. 🤓
There was a problem hiding this comment.
I'm struggling a bit with these tests, as months and years, of course, may have different number of days. Any advice welcome about this one!
There was a problem hiding this comment.
Is the variable length of the limit desired? So far, the rate limiter worked with fixed lengths, IIRC. We would change that now.
There was a problem hiding this comment.
I'm wondering if it's worth testing this method doing a simple timestamp operation. What I mean is if it changes, FixedWindowLimiter tests and similar will break, as I updated FixedWindowLimiterTest with long intervals too.
The lack of tests on TimeUtil currently makes me wonder even more.
| @@ -5,6 +5,7 @@ CHANGELOG | |||
| --- | |||
|
|
|||
| * The component is not experimental anymore | |||
| * Add support of long intervals (months and years) | |||
There was a problem hiding this comment.
| * Add support of long intervals (months and years) | |
| * Add support for long intervals (months and years) |
There was a problem hiding this comment.
Of course, fixed. Thanks, Oskar!
alexandre-daubois
changed the title
alexandre-daubois
force-pushed
the
feat/rate-limiter-long-interval
branch
from
a1d2d61
to
342164e
Compare
|
Thank you @alexandre-daubois. |
As mentioned in the issue and as found on Stackoverflow, we may use timestamps instead of converting
DateIntervalproperties to seconds.