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

Add startedTime configuration on RateLimiter #2100

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

injae-kim
Copy link

Fixes gh-752, gh-1668

Motivation

The API I need to apply a rate limiter for, is using the "Fixed window counter" algorithm.
Let's say it allows 10 requests each minute.
So, within the 2000-01-01T10:00:00 - 2000-01-01T10:01:00 time frame, I can do 10 requests.

This is necessary if someone wants to user RateLimiter both on his server and client 
... could make limit period overlap on the server and client.
  • We found that many users want to set some startedTime on RateLimiter
    • Some user wants to start rate limiter on every minute's start
    • Some user uses rate limiter on both server & client, and wants to sync them to overlap time window

Modification

  • Add startedTime configuration on RateLimiter

Result

@injae-kim injae-kim force-pushed the add-rate-limiter-started-time branch from b441ce2 to 09d9d1f Compare January 24, 2024 10:45
Comment on lines +129 to +133
if (startedTime.isAfter(Instant.now())) {
throw new IllegalArgumentException("StartedTime should be earlier or equal than now");
}
return startedTime;
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI) if we allow to set startTime that later than now, we need to fix lots of codes on AtomicRateLimiter.


/**
* Calculates time elapsed from the class loading.
*/
private long currentNanoTime() {
return nanoTime() - nanoTimeStart;
}

Cause AtomicRateLimiter assumes that startTime is the time when class is loaded and always now() > startTime

long currentNanos = currentNanoTime();
long currentCycle = currentNanos / cyclePeriodInNanos;

And it calculate currentCycle based on currentNanoTime() (elapsed time after class loaded), so if we set future as start time here then currentCycle is negative and limiter not works well.

Also, I think it's unclear what limiter should do when now() is before startTime.

So I named it as startedTime and want to set instant that earlier or equal than now~!

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.

startTime property in RateLimiter
1 participant