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

[bitnami/thanos] Add concurrency value to compactor #30722

Merged
merged 4 commits into from
Dec 12, 2024

Conversation

ChrisMcD1
Copy link
Contributor

Description of the change

Adds a field to set the concurrency of the Thanos Compactor

Benefits

This is a key field that controls resource consumption and processing speed of the compactor. Having this available as a core option instead of needing to set extraFlags would be helpful.

Possible drawbacks

None that I am aware of. The default value for this field is already 1. Docs link

--compact.concurrency=1   Number of goroutines to use when compacting groups.

Applicable issues

I did not create an issue.

Additional information

According to the Thanos Compactor documentation:

It’s recommended to give --compact.concurrency amount of CPU cores.

and

Memory usage depends on block sizes in the object storage and compaction concurrency.

I am no expert at writing Helm Charts, but the idea crossed my mind of allowing the chart to use the allocated CPU amount to dynamically set the concurrency level of the compactor. This could be especially useful when using resourcesPreset. Currently, the compactor won't really speed up at all when going from a small to a 2xlarge because it will still be constrained by the default value of this field. I was unable to find examples of this pattern being implemented online successfully, so perhaps it is just an anti-pattern to have dynamic chart behavior in that way.

Checklist

  • Chart version bumped in Chart.yaml according to semver. This is not necessary when the changes only affect README.md files.
  • Variables are documented in the values.yaml and added to the README.md using readme-generator-for-helm
  • Title of the pull request follows this pattern [bitnami/<name_of_the_chart>] Descriptive title
  • All commits signed off and in agreement of Developer Certificate of Origin (DCO)

@github-actions github-actions bot added thanos triage Triage is needed labels Dec 3, 2024
@github-actions github-actions bot requested a review from javsalgar December 3, 2024 02:50
Signed-off-by: Bitnami Containers <[email protected]>
@javsalgar javsalgar added verify Execute verification workflow for these changes in-progress labels Dec 3, 2024
@github-actions github-actions bot removed the triage Triage is needed label Dec 3, 2024
@github-actions github-actions bot removed the request for review from javsalgar December 3, 2024 09:27
@github-actions github-actions bot requested a review from alvneiayu December 3, 2024 09:27
@ChrisMcD1
Copy link
Contributor Author

While experimenting with a high concurrency setup today I realized that there were actually several other concurrency flags that one would also want to set to process a large backlog, like I am.

--compact.concurrency
--compact.blocks-fetch-concurrency
--downsample.concurrency
--block-files-concurrency
--block-meta-fetch-concurrency

So having a field that just claims the concurrency key on the compactor root element is probably not ideal.
Is it worth adding more descriptive field names for all 5, or maybe this should just be left as an extraFlags, and this PR can be closed.

alvneiayu
alvneiayu previously approved these changes Dec 12, 2024
Signed-off-by: Alvaro Neira Ayuso <[email protected]>
Signed-off-by: Bitnami Containers <[email protected]>
@alvneiayu alvneiayu merged commit 12b9c84 into bitnami:main Dec 12, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solved thanos verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants