Skip to content

Allow shard sizes to be percent of instances #5393

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

Merged
merged 1 commit into from
Jun 13, 2023

Conversation

harry671003
Copy link
Contributor

@harry671003 harry671003 commented Jun 9, 2023

What this PR does:
This PR allows querier and store-gateway tenant shard sizes to be configured as a percentage of a total

For backwards compatibility, the data type was changed to float64.

  • If the configured value is < 1, it'll be treated as a percentage.
  • If the value is > 1, it'll be treated as a constant number

Which issue(s) this PR fixes:
Fixes #5374

Checklist

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

Copy link
Member

@friedrichg friedrichg left a comment

Choose a reason for hiding this comment

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

I like that we are not adding another flag.

The operator in me says this flag needs a percentage sign like kubernetes does it. For humans to understand it better.
The developer in me says this number comes another system, this should be machine readable 😄
I can see value in both.

@@ -43,3 +43,12 @@ func ShuffleShardExpectedInstancesPerZone(shardSize, numZones int) int {
func ShuffleShardExpectedInstances(shardSize, numZones int) int {
return ShuffleShardExpectedInstancesPerZone(shardSize, numZones) * numZones
}

// DynamicShardSize returns the shard size as a percentage of numInstances if the value is < 1. If the value is > 1, the value is rounded and returned.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// DynamicShardSize returns the shard size as a percentage of numInstances if the value is < 1. If the value is > 1, the value is rounded and returned.
// DynamicShardSize returns the shard size as a percentage of numInstances if the value is < 1. If the value is >= 1, the value is rounded and returned.

Signed-off-by: 🌲 Harry 🌊 John 🏔 <[email protected]>
@harry671003
Copy link
Contributor Author

I like that we are not adding another flag.

The operator in me says this flag needs a percentage sign like kubernetes does it. For humans to understand it better. The developer in me says this number comes another system, this should be machine readable 😄 I can see value in both.

I agree. :)
We could clean this up (and simplify other configurations) in a future major release.

Copy link
Member

@friedrichg friedrichg left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks! Nice implementation and great work

@yeya24 yeya24 merged commit 2fbd2dd into cortexproject:master Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic tenant shard sizes
3 participants