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

Compact more than 2 blocks at a time #348

Merged
merged 4 commits into from
Nov 19, 2020
Merged

Conversation

mdisibio
Copy link
Contributor

What this PR does:
Enhance TimeWindowBlockSelector to allow choosing more than 2 blocks in the active window to compact at a time, controllable by parameters. Hard-coded default values are min=2 (same behavior as before) and max=8. Blocks outside the active window are compacted using the minimum block count always (same behavior as before). The min/max parameters are not exposed as configuration values but could be if it makes sense.

Existing tests were updated to specify max=2 to preserve prior behavior as needed. New test was added for choosing more than 2 blocks.

Which issue(s) this PR fixes:
n/a

Checklist

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

@joe-elliott
Copy link
Member

The min/max parameters are not exposed as configuration values but could be if it makes sense.

For now let's leave them unexposed. I would like to keep the number of tunables down and only expose things when we have reason.

So I'm generally fine with this PR, but I do have one concern. There is a weird interaction between this change and MaxCompactionObjects:

https://github.com/grafana/tempo/blob/master/tempodb/config.go#L33

If a time window + compaction level has 8 blocks, but those blocks are not compactable due to this value then the compactor will never attempt to compact them even though it may still be able to reduce the blocklist by doing 4 => 1 twice. Thoughts?

@mdisibio
Copy link
Contributor Author

So I'm generally fine with this PR, but I do have one concern. There is a weird interaction between this change and MaxCompactionObjects:

https://github.com/grafana/tempo/blob/master/tempodb/config.go#L33

If a time window + compaction level has 8 blocks, but those blocks are not compactable due to this value then the compactor will never attempt to compact them even though it may still be able to reduce the blocklist by doing 4 => 1 twice. Thoughts?

Good catch. Fixed and covered with a test.

Copy link
Contributor

@annanay25 annanay25 left a comment

Choose a reason for hiding this comment

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

LGTM!

@joe-elliott joe-elliott merged commit 143bd10 into master Nov 19, 2020
@joe-elliott joe-elliott deleted the compact-many-blocks branch November 19, 2020 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants