Distributor validation #5250
Distributor validation #5250
Conversation
Signed-off-by: Doğukan Teber <dogukanteber1@hotmail.com>
Signed-off-by: Doğukan Teber <dogukanteber1@hotmail.com>
Signed-off-by: Doğukan Teber <dogukanteber1@hotmail.com>
| @@ -175,8 +174,10 @@ func (cfg *Config) Validate(limits validation.Limits) error { | |||
| return errInvalidShardingStrategy | |||
| } | |||
|
|
|||
| if cfg.ShardingStrategy == util.ShardingStrategyShuffle && limits.IngestionTenantShardSize <= 0 { | |||
| return errInvalidTenantShardSize | |||
| if limits.IngestionTenantShardSize <= 0 { | |||
There was a problem hiding this comment.
There could be an scenario where:
- Shuffle sharding is enabled for all tenants
- Ingestion tenant shard is zero for all tenants
- There is one specific tenant that has tenant shard bigger than zero
cortex/pkg/util/validation/limits.go
Line 56 in 2f436e8
- Shuffle sharding should be definitely enabled for that tenant.
Is that supported with this change?
There was a problem hiding this comment.
I guess it does not. Let me check and make necessary changes
There was a problem hiding this comment.
Let's make sure it is still supported
There was a problem hiding this comment.
I checked the case you mentioned @friedrichg and made sure it is still supported with this change.
There was a problem hiding this comment.
We make sure things are supported adding tests. integration or unit test
|
i think changing the validation to not throw error when shard size is specified to zero should be fine. Ring package takes care of the cases when shard size specified is <=0 or greater than number of instances. It returns the whole ring in such cases, which is desired in this case |
Signed-off-by: Doğukan Teber <dogukanteber1@hotmail.com>
Signed-off-by: Doğukan Teber <dogukanteber1@hotmail.com>
| @@ -15,11 +15,11 @@ | |||
| * [ENHANCEMENT] Log: Avoid expensive log.Valuer evaluation for disallowed levels. #5297 | |||
| * [ENHANCEMENT] Improving Performance on the API Gzip Handler. #5347 | |||
| * [BUGFIX] Ruler: Validate if rule group can be safely converted back to rule group yaml from protobuf message #5265 | |||
| * [BUGFIX] Querier: Convert gRPC `ResourceExhausted` status code from store gateway to 422 limit error. #5286 | |||
There was a problem hiding this comment.
we need a separate PR for this. don't fix stuff that is not related to the PR
| @@ -175,8 +174,10 @@ func (cfg *Config) Validate(limits validation.Limits) error { | |||
| return errInvalidShardingStrategy | |||
| } | |||
|
|
|||
| if cfg.ShardingStrategy == util.ShardingStrategyShuffle && limits.IngestionTenantShardSize <= 0 { | |||
| return errInvalidTenantShardSize | |||
| if limits.IngestionTenantShardSize <= 0 { | |||
There was a problem hiding this comment.
We make sure things are supported adding tests. integration or unit test
|
This issue has been automatically marked as stale because it has not had any activity in the past 60 days. It will be closed in 15 days if no further activity occurs. Thank you for your contributions. |
What this PR does:
According to the docs, shuffle sharding should be disabled when
-distributor.ingestion-tenant-shard-sizeis set to 0:However, doing that returns an error. This PR adjusts the code to what the docs say.
Which issue(s) this PR fixes:
Fixes #5189
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]