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

Hide TSDB block ranges period config from doc and mark it experimental #3518

Merged
merged 2 commits into from
Nov 28, 2022

Conversation

pracucci
Copy link
Collaborator

What this PR does

From time to time I see some people from OSS community changing TSDB block ranges period. I think we're not ready for that, given whenever we design features and default config we only think about the TSDB 2h block ranges period. This option was configurable only for testing purposes (e.g. we use it in integration tests) but in my opinion shouldn't be changed (at least I would love that we officially not support any value different than 2h).

For this reason, I'm proposing to hide it from the doc.

I know this PR is controversial, but I genuinely believe that doing so we do a step forward to help OSS community not shooting to their feet (cause if you change it you need to understand how things work internally and which other config options to change accordingly).

Thoughts?

Which issue(s) this PR fixes or relates to

N/A

Checklist

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

@pracucci pracucci requested review from a team as code owners November 24, 2022 13:46
@pstibrany
Copy link
Member

💯 Agree. If it wasn't a breaking change, I'd remove the option as well.

@replay
Copy link
Contributor

replay commented Nov 24, 2022

I think hiding it is one possible solution, although it could be interpreted as a bug in the docs... I just wanted to propose one potential alternative solution which doesn't require hiding it which I already mentioned when we met recently:

We could add an unsafe flag to the configuration, for a user to tune certain flags like the block ranges period they would have to also specify unsafe: true because otherwise Mimir would exit with an error. The down-side of this solution is that users who are upgrading with an existing config that already tunes the block ranges period would then end up with a Mimir that doesn't start anymore after the upgrade. Later we might want to put more "dangerous" flags behind the unsafe.

@krajorama
Copy link
Contributor

+1 I'm always in favor of less configuration to worry about.

To fix the high disk utilization use case, I'd rather we shipped blocks at 80% disk capacity (or two hours, whichever comes first) . And if we don't test something , then it doesn't exists, so we should make the current option unsafe or remove it.

@osg-grafana
Copy link
Contributor

osg-grafana commented Nov 24, 2022

I think hiding it is one possible solution, although it could be interpreted as a bug in the docs...

This is sometimes a strategy to force a happy path, and at other times it is indicative of a deeper issue. Documentation is a tool of discovery in this case. How close can we fix x at its core? I will not block this PR whatever you all decide, but encourage a more root-cause fix.

Copy link
Contributor

@osg-grafana osg-grafana left a comment

Choose a reason for hiding this comment

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

Unblocking and leaning on engineering to make a decision wrt ease of use

@osg-grafana
Copy link
Contributor

Do any of these references need to be updated or removed: https://github.com/grafana/mimir/search?l=Markdown&q=TSDB+block+ranges

Copy link
Contributor

@jhesketh jhesketh left a comment

Choose a reason for hiding this comment

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

Makes sense to me

@replay replay mentioned this pull request Nov 25, 2022
37 tasks
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci force-pushed the hide-tsdb-range-period-from-doc branch from 23d271d to 40a4dd4 Compare November 28, 2022 09:03
@pracucci
Copy link
Collaborator Author

Do any of these references need to be updated or removed: https://github.com/grafana/mimir/search?l=Markdown&q=TSDB+block+ranges

Good point @osg-grafana ! I spot this: 40a4dd4

@pracucci
Copy link
Collaborator Author

Merging until we have a better way to deal with it (see #3528).

@pracucci pracucci merged commit 56b6e35 into main Nov 28, 2022
@pracucci pracucci deleted the hide-tsdb-range-period-from-doc branch November 28, 2022 11:22
replay pushed a commit that referenced this pull request Nov 28, 2022
#3518)

* Hide TSDB block ranges period config from doc and mark it experimental

Signed-off-by: Marco Pracucci <marco@pracucci.com>

* Remove CLI flag mention from Helm migration doc

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Signed-off-by: Marco Pracucci <marco@pracucci.com>
replay added a commit that referenced this pull request Nov 29, 2022
grafanabot pushed a commit that referenced this pull request Nov 29, 2022
#3518)

* Hide TSDB block ranges period config from doc and mark it experimental

Signed-off-by: Marco Pracucci <marco@pracucci.com>

* Remove CLI flag mention from Helm migration doc

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Signed-off-by: Marco Pracucci <marco@pracucci.com>
(cherry picked from commit 56b6e35)
replay pushed a commit that referenced this pull request Nov 29, 2022
#3518) (#3575)

* Hide TSDB block ranges period config from doc and mark it experimental

Signed-off-by: Marco Pracucci <marco@pracucci.com>

* Remove CLI flag mention from Helm migration doc

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Signed-off-by: Marco Pracucci <marco@pracucci.com>
(cherry picked from commit 56b6e35)

Co-authored-by: Marco Pracucci <marco@pracucci.com>
masonmei pushed a commit to udmire/mimir that referenced this pull request Dec 16, 2022
grafana#3518)

* Hide TSDB block ranges period config from doc and mark it experimental

Signed-off-by: Marco Pracucci <marco@pracucci.com>

* Remove CLI flag mention from Helm migration doc

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Signed-off-by: Marco Pracucci <marco@pracucci.com>
masonmei pushed a commit to udmire/mimir that referenced this pull request Dec 16, 2022
masonmei pushed a commit to udmire/mimir that referenced this pull request Dec 16, 2022
grafana#3518) (grafana#3575)

* Hide TSDB block ranges period config from doc and mark it experimental

Signed-off-by: Marco Pracucci <marco@pracucci.com>

* Remove CLI flag mention from Helm migration doc

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Signed-off-by: Marco Pracucci <marco@pracucci.com>
(cherry picked from commit 56b6e35)

Co-authored-by: Marco Pracucci <marco@pracucci.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants