Change the default of ignore_deletion_mark_delay to 1h #892
Conversation
| @@ -51,7 +51,7 @@ Because of this, we would suggest to avoid querying non compacted blocks. In ord | |||
| 3. Configure queriers `-querier.query-store-after` large enough to give compactor enough time to compact newly uploaded blocks (_see below_) | |||
| 4. Configure queriers `-querier.query-ingesters-within` equal to `-querier.query-store-after` plus 5m (5 minutes is just a delta to query the boundary both from ingesters and queriers) | |||
| 5. Configure ingesters `-blocks-storage.tsdb.retention-period` at least as `-querier.query-ingesters-within` | |||
| 6. Lower `-blocks-storage.bucket-store.ignore-deletion-marks-delay` to 1h, otherwise non compacted blocks could be queried anyway, even if their compacted replacement is available | |||
| 6. Ensure `-blocks-storage.bucket-store.ignore-deletion-marks-delay` is not too large (the default of 1h is suggested), otherwise non compacted blocks could be queried anyway, even if their compacted replacement is available | |||
There was a problem hiding this comment.
Repeating the default value here felt like introducing a maintenance burden, but without anything to reference the sentence seemed ambiguous.
There was a problem hiding this comment.
I agree with Andy, reason why we try to avoid mentioning default values in non-autogenerated doc (unless where mentioning the default is very valuable in the context).
There was a problem hiding this comment.
I swapped the language to (the default is suggested) to avoid referencing the actual value.
| # CLI flag: -blocks-storage.bucket-store.ignore-deletion-marks-delay | ||
| [ignore_deletion_mark_delay: <duration> | default = 6h] |
There was a problem hiding this comment.
this is unrelated to your change:
I noticed that there is a discrepancy between the cli flag and the yaml property:
-blocks-storage.bucket-store.ignore-deletion-marks-delay
ignore_deletion_mark_delay
note the mark/marks. I think that's something which could confuse a user, so I'm wondering if it would be worth it to fix that before the launch of Mimir. Now it's easier to make breaking changes than after the launch. WDYT? cc @09jvilla
There was a problem hiding this comment.
We have more fields like that, and now would be a great time to fix them all.
There was a problem hiding this comment.
If we'll fix all of them, then we could enforce it in CI (eg. fail CI if any CLI flag doesn't reflect its YAML counterpart).
There was a problem hiding this comment.
We have more fields like that, and now would be a great time to fix them all.
Do you remember any of them from the top of your head by chance?
I'll create an issue for it and then try to come up with a way to detect these discrepancies
There was a problem hiding this comment.
I think you could list of all them doing an hack in the doc generator code. There for each CLI flag we know the YAML config, so we you can compare them (with some substitutions, like _ and -) and print when they're different.
There was a problem hiding this comment.
Seems like a good idea to me to make this change here and for all flags and agreed now is the time to do it.
There was a problem hiding this comment.
@replay can you create an issue for this consistent naming convention between CLI and yaml flags for tracking and add it to https://github.com/grafana/mimir-squad/issues/367 under the 'low hanging fruit we really want to get done if possible'? I think that list in this point is mostly becoming "we want this done by launch", but I'd say this isn't a launch blocker if we can't get it complete.
There was a problem hiding this comment.
Created #901 and added it where you said
| @@ -51,7 +51,7 @@ Because of this, we would suggest to avoid querying non compacted blocks. In ord | |||
| 3. Configure queriers `-querier.query-store-after` large enough to give compactor enough time to compact newly uploaded blocks (_see below_) | |||
| 4. Configure queriers `-querier.query-ingesters-within` equal to `-querier.query-store-after` plus 5m (5 minutes is just a delta to query the boundary both from ingesters and queriers) | |||
| 5. Configure ingesters `-blocks-storage.tsdb.retention-period` at least as `-querier.query-ingesters-within` | |||
| 6. Lower `-blocks-storage.bucket-store.ignore-deletion-marks-delay` to 1h, otherwise non compacted blocks could be queried anyway, even if their compacted replacement is available | |||
| 6. Ensure `-blocks-storage.bucket-store.ignore-deletion-marks-delay` is not too large (the default of 1h is suggested), otherwise non compacted blocks could be queried anyway, even if their compacted replacement is available | |||
There was a problem hiding this comment.
I agree with Andy, reason why we try to avoid mentioning default values in non-autogenerated doc (unless where mentioning the default is very valuable in the context).
| # CLI flag: -blocks-storage.bucket-store.ignore-deletion-marks-delay | ||
| [ignore_deletion_mark_delay: <duration> | default = 6h] |
There was a problem hiding this comment.
If we'll fix all of them, then we could enforce it in CI (eg. fail CI if any CLI flag doesn't reflect its YAML counterpart).
andyasp
force-pushed
the
aasp/ignore-deletion-mark-delay
branch
2 times, most recently
from
0129a4c
to
64099e2
Compare
andyasp
force-pushed
the
aasp/ignore-deletion-mark-delay
branch
from
64099e2
to
4ba930a
Compare
What this PR does:
Updates the default of
-blocks_storage.bucket_store.ignore_deletion_marks_delayto1hto put the default in line with how it is commonly set.Which issue(s) this PR fixes:
Fixes https://github.com/grafana/mimir-squad/issues/485
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]