diff --git a/CHANGELOG.md b/CHANGELOG.md index 64fa3fd564..600a94d0c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ * [BUGFIX] Distributor: Do not use label with empty values for sharding #5717 * [BUGFIX] Query Frontend: queries with negative offset should check whether it is cacheable or not. #5719 * [BUGFIX] Redis Cache: pass `cache_size` config correctly. #5734 +* [BUGFIX] Distributor: Shuffle-Sharding with IngestionTenantShardSize == 0, default sharding strategy should be used #5189 ## 1.16.0 2023-11-20 diff --git a/integration/ingester_sharding_test.go b/integration/ingester_sharding_test.go index ab8e2ca06d..aeffc1f0cc 100644 --- a/integration/ingester_sharding_test.go +++ b/integration/ingester_sharding_test.go @@ -27,16 +27,28 @@ func TestIngesterSharding(t *testing.T) { tenantShardSize int expectedIngestersWithSeries int }{ - "default sharding strategy should spread series across all ingesters": { + //Default Sharding Strategy + "default sharding strategy should be ignored and spread across all ingesters": { shardingStrategy: "default", tenantShardSize: 2, // Ignored by default strategy. expectedIngestersWithSeries: 3, }, + "default sharding strategy should spread series across all ingesters": { + shardingStrategy: "default", + tenantShardSize: 0, // Ignored by default strategy. + expectedIngestersWithSeries: 3, + }, + //Shuffle Sharding Strategy "shuffle-sharding strategy should spread series across the configured shard size number of ingesters": { shardingStrategy: "shuffle-sharding", tenantShardSize: 2, expectedIngestersWithSeries: 2, }, + "Tenant Shard Size of 0 should leverage all ingesters": { + shardingStrategy: "shuffle-sharding", + tenantShardSize: 0, + expectedIngestersWithSeries: 3, + }, } for testName, testData := range tests { @@ -125,7 +137,7 @@ func TestIngesterSharding(t *testing.T) { // We expect that only ingesters belonging to tenant's shard have been queried if // shuffle sharding is enabled. expectedIngesters := ingesters.NumInstances() - if testData.shardingStrategy == "shuffle-sharding" { + if testData.shardingStrategy == "shuffle-sharding" && testData.tenantShardSize > 0 { expectedIngesters = testData.tenantShardSize } diff --git a/pkg/distributor/distributor.go b/pkg/distributor/distributor.go index 49d9446d68..4b4eea0bf9 100644 --- a/pkg/distributor/distributor.go +++ b/pkg/distributor/distributor.go @@ -49,7 +49,7 @@ var ( // Validation errors. errInvalidShardingStrategy = errors.New("invalid sharding strategy") - errInvalidTenantShardSize = errors.New("invalid tenant shard size, the value must be greater than 0") + errInvalidTenantShardSize = errors.New("invalid tenant shard size. The value must be greater than or equal to 0") // Distributor instance limits errors. errTooManyInflightPushRequests = errors.New("too many inflight push requests in distributor") @@ -178,7 +178,7 @@ func (cfg *Config) Validate(limits validation.Limits) error { return errInvalidShardingStrategy } - if cfg.ShardingStrategy == util.ShardingStrategyShuffle && limits.IngestionTenantShardSize <= 0 { + if cfg.ShardingStrategy == util.ShardingStrategyShuffle && limits.IngestionTenantShardSize < 0 { return errInvalidTenantShardSize } diff --git a/pkg/distributor/distributor_test.go b/pkg/distributor/distributor_test.go index fd7efa062b..74e9080da2 100644 --- a/pkg/distributor/distributor_test.go +++ b/pkg/distributor/distributor_test.go @@ -76,14 +76,14 @@ func TestConfig_Validate(t *testing.T) { initLimits: func(_ *validation.Limits) {}, expected: errInvalidShardingStrategy, }, - "should fail if the default shard size is 0 on when sharding strategy = shuffle-sharding": { + "should pass sharding strategy when IngestionTenantShardSize = 0": { initConfig: func(cfg *Config) { cfg.ShardingStrategy = "shuffle-sharding" }, initLimits: func(limits *validation.Limits) { limits.IngestionTenantShardSize = 0 }, - expected: errInvalidTenantShardSize, + expected: nil, }, "should pass if the default shard size > 0 on when sharding strategy = shuffle-sharding": { initConfig: func(cfg *Config) { @@ -94,6 +94,15 @@ func TestConfig_Validate(t *testing.T) { }, expected: nil, }, + "should fail because the ingestionTenantShardSize is a non-positive number": { + initConfig: func(cfg *Config) { + cfg.ShardingStrategy = "shuffle-sharding" + }, + initLimits: func(limits *validation.Limits) { + limits.IngestionTenantShardSize = -1 + }, + expected: errInvalidTenantShardSize, + }, } for testName, testData := range tests {