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

copyblocks: add option to bypass duration check for no-compact blocks #9439

Merged
merged 4 commits into from
Sep 27, 2024

Conversation

andyasp
Copy link
Contributor

@andyasp andyasp commented Sep 26, 2024

What this PR does

Adds a flag to bypass the block duration check in copyblocks for blocks marked as no-compact.

It's a common pattern to use min-block-duration with the intent of targeting blocks that are fully compacted. This can miss blocks that are marked as no-compact that have a shorter duration. This flag makes it easier to not miss those blocks.

Which issue(s) this PR fixes or relates to

Fixes N/A

Checklist

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

@andyasp andyasp requested a review from a team as a code owner September 26, 2024 19:19
@narqo
Copy link
Contributor

narqo commented Sep 27, 2024

The CHANGELOG has just been cut to prepare for the next release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the CHANGELOG.md document. Thanks!

@andyasp andyasp force-pushed the aasp/any-no-compact-duration branch from afb5414 to 9dd8f6b Compare September 27, 2024 15:13
Copy link
Contributor

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

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

Option seems good but the name is a bit confusing to me. Is there a way to rename it to be more obvious what it does? Maybe -disable-no-compact-validation or something? I don't feel strongly so if you are good with the current name feel free to merge as-is.

@andyasp
Copy link
Contributor Author

andyasp commented Sep 27, 2024

I struggled with the name as well. Open to ideas for it and I'll hold off while trying to think of something myself. -disable-no-compact-validation doesn't quite work because this is only for the duration check specifically.

@@ -59,6 +60,7 @@ func (c *config) registerFlags(f *flag.FlagSet) {
f.Var(&c.enabledUsers, "enabled-users", "If not empty, only blocks for these users are copied.")
f.Var(&c.disabledUsers, "disabled-users", "If not empty, blocks for these users are not copied.")
f.BoolVar(&c.dryRun, "dry-run", false, "Don't perform any copy; only log what would happen.")
f.BoolVar(&c.anyNoCompactBlockDuration, "any-no-compact-block-duration", false, "If set, blocks marked as no-compact are not checked against min-block-duration")
Copy link
Contributor

Choose a reason for hiding this comment

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

How about --skip-no-compact-block-duration-check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is way more clear. I'll change it to that, thanks!

tools/copyblocks/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@narqo narqo left a comment

Choose a reason for hiding this comment

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

🔥 Works for me

CHANGELOG.md Outdated Show resolved Hide resolved
@andyasp andyasp merged commit 10a3559 into main Sep 27, 2024
29 checks passed
@andyasp andyasp deleted the aasp/any-no-compact-duration branch September 27, 2024 18:21
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.

3 participants