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 support for retries on 429 rate limited requests #193

Merged
merged 1 commit into from
Nov 30, 2023
Merged

Conversation

acm19
Copy link
Contributor

@acm19 acm19 commented Nov 23, 2023

Adds an option to enable/disable retries on rate limit errors: dropRateLimitedBatches. It's enabled by default to keep consistency with Promtail options. Also because Loki scales less aggressively and relies on retries when the load is spiky.

Resolves: #192

@acm19 acm19 force-pushed the main branch 3 times, most recently from d851c91 to 6396c9d Compare November 24, 2023 12:16
Copy link
Contributor

@nehaev nehaev left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Overall the PR looks good. There are only a few minor comments to address.

@acm19
Copy link
Contributor Author

acm19 commented Nov 27, 2023

@nehaev thanks for the review, I've replied to your comments, let me know if you want any other changes.

@acm19 acm19 requested a review from nehaev November 27, 2023 14:28
Adds an option to enable/disable retries on rate limit errors:
`dropRateLimitedBatches`. It's enabled by default to keep consistency
with Promtail options. Also because Loki scales less aggressively and
relies on retries when the load is spiky.

Resolves: loki4j#192
Copy link
Contributor

@nehaev nehaev left a comment

Choose a reason for hiding this comment

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

Everything looks good! Grafana Cloud tests are failing, but it's unrelated to this PR.

I'll try to fix tests and then will merge this.

@acm19
Copy link
Contributor Author

acm19 commented Nov 28, 2023

Great, thanks @nehaev.

@nehaev nehaev merged commit b66ec83 into loki4j:main Nov 30, 2023
7 of 9 checks passed
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.

Support retries on rate limited batches
2 participants