Skip to content

update too many req cooldown handling #176

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

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

yatharthranjan
Copy link
Member

According to this, the rate limit is reset at the top of the hour, unlike the current implementation - using a rolling window from when the error is received.

Returned if the application has reached the rate limit for a specific user. The rate limit will be reset at the top of the hour.

This PR adds a strategy to calculate cooldown/backoff time based on top of the current hour. But the legacy code is till present and is the default. Configuration can be dones as follows-

# Use top-of-hour strategy (recommended for Fitbit)
fitbit.cooldown.strategy=TOP_OF_HOUR

# Use rolling window strategy (legacy behavior)
fitbit.cooldown.strategy=ROLLING_WINDOW
fitbit.too.many.requests.cooldown.s=3600

This PR also makes the backoff configurable. This was hardcoded to 1hr by default. Though i think we don't need to update this dynamically since rate limit is by the hour (150 req per hour), but would be good for testing and debugging.

public static final String FITBIT_COOLDOWN_STRATEGY_CONFIG = "fitbit.cooldown.strategy";
private static final String FITBIT_COOLDOWN_STRATEGY_DOC = "Strategy for handling too many requests cooldown. Options: ROLLING_WINDOW, TOP_OF_HOUR";
private static final String FITBIT_COOLDOWN_STRATEGY_DISPLAY = "Cooldown strategy";
private static final String FITBIT_COOLDOWN_STRATEGY_DEFAULT = "ROLLING_WINDOW";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the TOP_OF_HOUR approach should be the default, right. It conforms to the cooldown strategy of Fitbit.

@@ -849,7 +878,7 @@ public Duration getPollIntervalPerUser() {
}

public Duration getTooManyRequestsCooldownInterval() {
return Duration.ofHours(1);
return Duration.ofSeconds(getInt(FITBIT_TOO_MANY_REQUESTS_COOLDOWN_CONFIG));
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we have an issue of problems with backwards compatibility I think. We should take care to add some migration note to RADAR-Kubernetes.

Copy link
Member

@mpgxvii mpgxvii left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

3 participants