-
Notifications
You must be signed in to change notification settings - Fork 804
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
Remove -store.fullsize-chunks option #2656
Conversation
@@ -15,15 +15,13 @@ type Config struct{} | |||
|
|||
var ( | |||
// DefaultEncoding exported for use in unit tests elsewhere | |||
DefaultEncoding = Bigchunk | |||
alwaysMarshalFullsizeChunks = true |
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.
Why are you saying it was unused? Being the default true
doesn't it mean it's always used unless explicitly disabled via CLI flag?
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, it was written this way to avoid changing things for existing users.
I mean the option to disable was never used.
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.
However I got the change wrong.
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, then we need to remove the flag while always keeping it enabled ;)
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.
Try now.
c23ce13
to
4a00949
Compare
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!
CHANGELOG.md
Outdated
@@ -4,6 +4,7 @@ | |||
|
|||
* [CHANGE] Query Frontend now uses Round Robin to choose a tenant queue to service next. #2553 | |||
* [CHANGE] `-promql.lookback-delta` is now deprecated and has been replaced by `-querier.lookback-delta` along with `lookback_delta` entry under `querier` in the config file. `-promql.lookback-delta` will be removed in v1.4.0. #2604 | |||
* [CHANGE] Removed `-store.fullsize-chunks` option which was undocumented and unused (it broke ingester hand-overs). |
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.
Would you mind adding the PR number at the end of the line? The release script we use look for PRs missing in the CHANGELOG (for manual check) and does the check based on the PR number here.
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 but needs rebasing and DCO.
f1f926b
to
856112c
Compare
It broke ingester hand-overs, and is not needed now we use BigChunk. I left in the code which allows non-full-size chunks to be read in: maybe someone has some in a database. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
856112c
to
77f5a1d
Compare
It broke ingester hand-overs, and is not needed now we use BigChunk.
Fixes #1163
Checklist
CHANGELOG.md
updated