-
Notifications
You must be signed in to change notification settings - Fork 543
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
feat(ruler): add flags for the query frontend retries #10259
base: main
Are you sure you want to change the base?
Conversation
493f5d9
to
e89f6db
Compare
Currently, Mimir is hardcoded at 3 retries and 100ms as a minimum retry time. Could be useful to configure this
e89f6db
to
fb55d8e
Compare
Should the config flags be experimental for now? |
The new flags expose the existing retry config which is already enabled for everyone using the query frontend, which isn't an experimental feature from what I can see. It doesn't seem like they should be experimental, unless I'm misunderstanding 🤔. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you're adding those flags so you can experiment with different retry strategies. I'm also leaning towards making these experimental and removing the once we know what's the better setting.
@@ -2987,6 +2987,12 @@ Usage of ./cmd/mimir/mimir: | |||
Override the default minimum TLS version. Allowed values: VersionTLS10, VersionTLS11, VersionTLS12, VersionTLS13 | |||
-ruler.query-frontend.grpc-client-config.tls-server-name string | |||
Override the expected name on the server certificate. | |||
-ruler.query-frontend.max-retries int | |||
[experimental] Maximum number of retries for a single request. (default 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is actually max attempts, right? So it includes the first try. That's my understanding from reading the dskit code. I think we should clarify the CLI flag then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a suggest about the wording on the max retries, but otherwise LGTM, I don't need to see it again
Currently, Mimir is hardcoded at 3 retries and 100ms as a minimum retry time. Could be useful to configure this
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
.about-versioning.md
updated with experimental features.