address comments

Signed-off-by: Roy Chiang <[email protected]>

Author: roystchiang

docs/guides/shuffle-sharding.md

@@ -160,10 +160,8 @@ Note that when using sharding strategy, each rule group is evaluated by single r
 Cortex compactor can run in three modes:
 
 1. **No sharding at all.** This is the most basic mode of the compactor. It is activated by using `-compactor.sharding-enabled=false` (default). In this mode every compactor will run every compaction.
-2. **Default sharding**, activated by using `-compactor.sharding-enabled=true` and `-compactor.sharding-strategy=default` (default). In this mode compactors register themselves into the ring. Each compactor will then select and evaluate only those users that it "owns".
-3. **Shuffle sharding**, activated by using `-compactor.sharding-enabled=true` and `-compactor.sharding-strategy=shuffle-sharding`. Similarly to default sharding, compactors use the ring to distribute workload, but compactions groups for each tenant can only be evaluated on limited number of compactors (`-compactor.tenant-shard-size`, can also be set per tenant as `compactor_tenant_shard_size` in overrides).
-
-The Cortex compactor by default shards by tenant ID when sharding is enabled.
+2. **Default sharding**, activated by using `-compactor.sharding-enabled=true` and `-compactor.sharding-strategy=default` (default). In this mode compactors register themselves into the ring. One single tenant will belong to only 1 compactor.
+3. **Shuffle sharding**, activated by using `-compactor.sharding-enabled=true` and `-compactor.sharding-strategy=shuffle-sharding`. Similarly to default sharding, but compactions for each tenant can be carried out on multiple compactors (`-compactor.tenant-shard-size`, can also be set per tenant as `compactor_tenant_shard_size` in overrides).
 
 With shuffle sharding selected as the sharding strategy, a subset of the compactors will be used to handle a user based on the shard size.
 

pkg/compactor/compactor.go

@@ -51,7 +51,6 @@ var (
 
 	supportedShardingStrategies = []string{util.ShardingStrategyDefault, util.ShardingStrategyShuffle}
 	errInvalidShardingStrategy  = errors.New("invalid sharding strategy")
-	errShardingRequired         = errors.New("sharding must be enabled to use shuffle-sharding sharding strategy")
 	errInvalidTenantShardSize   = errors.New("invalid tenant shard size, the value must be greater than 0")
 
 	DefaultBlocksGrouperFactory = func(ctx context.Context, cfg Config, bkt objstore.Bucket, logger log.Logger, reg prometheus.Registerer, blocksMarkedForDeletion , blocksMarkedForNoCompaction, garbageCollectedBlocks prometheus.Counter, _ prometheus.Gauge, _ *ring.Ring, _ *ring.Lifecycler, _ Limits, _ string) compact.Grouper {
@@ -231,10 +230,8 @@ func (cfg *Config) Validate(limits validation.Limits) error {
 		return errInvalidShardingStrategy
 	}
 
-	if cfg.ShardingStrategy == util.ShardingStrategyShuffle {
-		if !cfg.ShardingEnabled {
-			return errShardingRequired
-		} else if limits.CompactorTenantShardSize <= 0 {
+	if cfg.ShardingEnabled && cfg.ShardingStrategy == util.ShardingStrategyShuffle {
+		if limits.CompactorTenantShardSize <= 0 {
 			return errInvalidTenantShardSize
 		}
 	}
@@ -350,10 +347,10 @@ func newCompactor(
 	limits Limits,
 ) (*Compactor, error) {
 	var remainingPlannedCompactions prometheus.Gauge
-	if compactorCfg.ShardingStrategy == "shuffle-sharding" {
+	if compactorCfg.ShardingStrategy == util.ShardingStrategyShuffle {
 		remainingPlannedCompactions = promauto.With(registerer).NewGauge(prometheus.GaugeOpts{
 			Name: "cortex_compactor_remaining_planned_compactions",
-			Help: "Total number of plans that remain to be compacted.",
+			Help: "Total number of plans that remain to be compacted. Only available with shuffle-sharding strategy",
 		})
 	}
 	c := &Compactor{

pkg/compactor/compactor_test.go

@@ -125,14 +125,6 @@ func TestConfig_Validate(t *testing.T) {
 			},
 			expected: "",
 		},
-		"should fail with shuffle sharding strategy selected without sharding enabled": {
-			setup: func(cfg *Config) {
-				cfg.ShardingStrategy = util.ShardingStrategyShuffle
-				cfg.ShardingEnabled = false
-			},
-			initLimits: func(_ *validation.Limits) {},
-			expected:   errShardingRequired.Error(),
-		},
 		"should fail with bad compactor tenant shard size": {
 			setup: func(cfg *Config) {
 				cfg.ShardingStrategy = util.ShardingStrategyShuffle

pkg/compactor/shuffle_sharding_grouper.go

@@ -130,7 +130,7 @@ func (g *ShuffleShardingGrouper) Groups(blocks map[ulid.ULID]*metadata.Meta) (re
 		return nil, errors.Wrap(err, "unable to check sub-ring for compactor ownership")
 	}
 	if !onSubring {
-		level.Info(g.logger).Log("msg", "compactor is not on the current sub-ring skipping user", "user", g.userID)
+		level.Debug(g.logger).Log("msg", "compactor is not on the current sub-ring skipping user", "user", g.userID)
 		return outGroups, nil
 	}
 	// Metrics for the remaining planned compactions