-
Notifications
You must be signed in to change notification settings - Fork 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
Add new flags and memcached support to query frontend #166
Conversation
Signed-off-by: Ben Ye <yb532204897@gmail.com>
Signed-off-by: Ben Ye <yb532204897@gmail.com>
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.
One comment, otherwise looks really good 👍
all.jsonnet
Outdated
splitInterval: '24h', | ||
maxRetries: 5, | ||
logQueriesLongerThan: '5s', |
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.
These were changed on purposed to different values than the defaults, to show that overwriting the values actually works, which is kind of the entire point of the all.jsonnet
in total 😉 😊
Happy to add this as a comment to the top of the file. Wdyt?
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.
Oh, got it. Thanks for the clarification!
I was wondering why this changed yesterday 🤣 I can update these values back.
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.
Yes, that would be great and maybe even change values in all.jsonnet
to different ones that the defaults, haha. :) Doesn't need to be too crazy though.
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.
LGTM 🥇
Could you update Thanos version that supports these changes? These aren't in v0.16.0, are they?
They are part of the new release so v0.17.0. Can I use the current master image? |
Then let's wait for a couple of hours / days with merging this and update to at least use the first rc I'll cut soon. |
Signed-off-by: Ben Ye <yb532204897@gmail.com>
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.
Nice,
This does not mean we cannot use it in our prod/stage conf code, right? (: |
Hm, good point about using these new features downstream. I guess for that reason we could already go ahead and merge it, since I usually see this master branch match mostly whatever Thanos master is doing. |
Sorry, I quickly want to release v0.16.0 branch without this. Happy to merge after that (next 30min). Just making sure we don't merge it before that, thus converting to draft 😇 |
I guess it is ready to go. Mark this ready for review. |
Signed-off-by: Ben Ye yb532204897@gmail.com
Changes
This pr supersedes #163.
Added new flag configurations for the labels and series middleware of the query frontend.
Verification