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 new flags and memcached support to query frontend #166

Merged
merged 3 commits into from
Nov 4, 2020

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Nov 3, 2020

Signed-off-by: Ben Ye yb532204897@gmail.com

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

This pr supersedes #163.

Added new flag configurations for the labels and series middleware of the query frontend.

Verification

Signed-off-by: Ben Ye <yb532204897@gmail.com>
Signed-off-by: Ben Ye <yb532204897@gmail.com>
Copy link
Contributor

@metalmatze metalmatze left a 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
Comment on lines 113 to 115
splitInterval: '24h',
maxRetries: 5,
logQueriesLongerThan: '5s',
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Member

@kakkoyun kakkoyun left a 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?

@yeya24
Copy link
Contributor Author

yeya24 commented Nov 4, 2020

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?

@metalmatze
Copy link
Contributor

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>
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Nice,

@bwplotka
Copy link
Member

bwplotka commented Nov 4, 2020

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.

This does not mean we cannot use it in our prod/stage conf code, right? (:

@metalmatze
Copy link
Contributor

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.
The overhead of adding this to our downstream for 2 days is probably not justified.
Feel free to merge in this case!

@metalmatze metalmatze marked this pull request as draft November 4, 2020 14:49
@metalmatze
Copy link
Contributor

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 😇

@yeya24
Copy link
Contributor Author

yeya24 commented Nov 4, 2020

I guess it is ready to go. Mark this ready for review.

@yeya24 yeya24 marked this pull request as ready for review November 4, 2020 15:38
@bwplotka bwplotka merged commit a878b46 into thanos-io:master Nov 4, 2020
@yeya24 yeya24 deleted the qf-new-flags branch November 4, 2020 16:01
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.

4 participants