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.

Sign up for GitHub

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

Jump to bottom

Enable ingester to querier chunks streaming and ingester query request minimisation by default #6174

Merged
merged 6 commits into from
Nov 13, 2023
Merged

Enable ingester to querier chunks streaming and ingester query request minimisation by default #6174

merged 6 commits into from
Nov 13, 2023

Conversation

charleskorn
Copy link
Contributor

@charleskorn charleskorn commented Oct 2, 2023 •
edited
Loading

What this PR does

This PR enables two recently-introduced features by default, and marks them as no longer being experimental:

  • ingester to querier chunks streaming (store-gateway to querier chunks streaming will remain experimental for now)
  • ingester query request minimisation

Both of these features have been enabled in production at Grafana Labs for quite some time now, and we've had no issues with either of them.

Which issue(s) this PR fixes or relates to

(none)

Checklist

  • [n/a] Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@charleskorn charleskorn marked this pull request as ready for review October 2, 2023 05:18
@charleskorn charleskorn requested review from a team as code owners October 2, 2023 05:18
@charleskorn charleskorn marked this pull request as draft October 2, 2023 06:29
@charleskorn
Copy link
Contributor Author

Putting this back to draft while I investigate an issue I've just noticed on an internal Grafana Labs cluster.

pracucci
pracucci approved these changes Oct 4, 2023
View reviewed changes
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

I left you investigate the issue you've mentioned. If turns out it was a false positive, then I'm happy to mark it as stable and enable it by default. Good job!

CHANGELOG.md Outdated Show resolved Hide resolved
dimitarvdimitrov
dimitarvdimitrov reviewed Oct 6, 2023
View reviewed changes
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

are there cases where this feature has to be disabled? I'm wondering if it's not a bad idea to enable them by default but then remove the flag after some time and always have the feature enabled? That will help keep the configuration surface of mimir smaller

@charleskorn @pracucci
Update CHANGELOG.md
33fcc1d 
Co-authored-by: Marco Pracucci <[email protected]>
@charleskorn
Copy link
Contributor Author

are there cases where this feature has to be disabled? I'm wondering if it's not a bad idea to enable them by default but then remove the flag after some time and always have the feature enabled? That will help keep the configuration surface of mimir smaller

Agree. There are no cases where you'd want to disable this based on our testing. I'd like to leave the flag there for a little while just in case there's an issue we haven't already run into, but I'd absolutely like to make streaming the default and remove the flag altogether if things go smoothly.

@dimitarvdimitrov
Copy link
Contributor

are there cases where this feature has to be disabled? I'm wondering if it's not a bad idea to enable them by default but then remove the flag after some time and always have the feature enabled? That will help keep the configuration surface of mimir smaller

Agree. There are no cases where you'd want to disable this based on our testing. I'd like to leave the flag there for a little while just in case there's an issue we haven't already run into, but I'd absolutely like to make streaming the default and remove the flag altogether if things go smoothly.

my comment was mostly about keeping the flag as experimental in this PR so that we can remove it without going through the deprecation process

@charleskorn
Copy link
Contributor Author

my comment was mostly about keeping the flag as experimental in this PR so that we can remove it without going through the deprecation process

Right, makes sense. Sounds good to me, I'll switch it back to experimental.

charleskorn and others added 2 commits October 10, 2023 14:35
@charleskorn
Mark prefer_streaming_chunks_from_ingesters and minimize_ingester_req…
5582e50 
…uests as experimental so we can remove them without deprecation in a later release
@charleskorn
Update about-versioning.md
c073ada
@charleskorn charleskorn marked this pull request as ready for review October 19, 2023 02:53
@charleskorn
Copy link
Contributor Author

#6346 fixed the issue I discovered and we've had no instances of the issue since rolling out that fix, so I believe this is good to go again.

dimitarvdimitrov
dimitarvdimitrov approved these changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

LGTM

@charleskorn charleskorn merged commit df905ca into main Nov 13, 2023
28 checks passed
@charleskorn charleskorn deleted the charleskorn/ingester-defaults branch November 13, 2023 23:55
@charleskorn charleskorn mentioned this pull request Nov 14, 2023
Merged
2 tasks
@dimitarvdimitrov dimitarvdimitrov mentioned this pull request Jan 22, 2024
Open
This was referenced Mar 16, 2024
Closed
Merged
Merged
@eyenx eyenx mentioned this pull request May 28, 2024
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dimitarvdimitrov dimitarvdimitrov dimitarvdimitrov approved these changes

@pracucci pracucci pracucci approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@charleskorn @dimitarvdimitrov @pracucci