Addressed review comments

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Author: pracucci

CHANGELOG.md

@@ -3,11 +3,11 @@
 ## master / unreleased
 
 * [CHANGE] Blocks storage: compactor is now required when running a Cortex cluster with the blocks storage, because it also keeps the bucket index updated. #3583
-* [CHANGE] Blocks storage: block deletion marks are now stored in a per-tenant global markers/ location too, other than within the block location. The compactor, at startup, will copy deletion marks from the block location to the global location. This migration is required only once, so you can safely disable it via `-compactor.block-deletion-marks-migration-enabled=false` once the new compact has successfully started once in your cluster. #3583
+* [CHANGE] Blocks storage: block deletion marks are now stored in a per-tenant global markers/ location too, other than within the block location. The compactor, at startup, will copy deletion marks from the block location to the global location. This migration is required only once, so you can safely disable it via `-compactor.block-deletion-marks-migration-enabled=false` once new compactor has successfully started once in your cluster. #3583
 * [ENHANCEMENT] Memberlist: add status page (`/memberlist`) with available details about memberlist-based KV store and memberlist cluster. It's also possible to view KV values in Go struct or JSON format, or download for inspection. #3575
 * [ENHANCEMENT] Memberlist: client can now keep a size-bounded buffer with sent and received messages and display them in the admin UI (/memberlist) for troubleshooting. #3581
 * [ENHANCEMENT] Compactor: exported the following metrics.
-* [ENHANCEMENT] Blocks storage: introduced a per-tenant bucket index, periodically updated by the compactor, used to avoid full bucket scanning done by queriers and store-gateways. The bucket index is updated by the compactor every `-compactor.cleanup-interval`. #3553 #3555 #3561 #3583
+* [ENHANCEMENT] Blocks storage: introduced a per-tenant bucket index, periodically updated by the compactor, used to avoid full bucket scanning done by queriers and store-gateways. The bucket index is updated by the compactor during blocks cleanup, on every `-compactor.cleanup-interval`. #3553 #3555 #3561 #3583
 * [ENHANCEMENT] Compactor: exported the following metrics. #3583
   * `cortex_bucket_blocks_count`: Total number of blocks per tenant in the bucket. Includes blocks marked for deletion.
   * `cortex_bucket_blocks_marked_for_deletion_count`: Total number of blocks per tenant marked for deletion in the bucket.

docs/blocks-storage/compactor.md

@@ -139,8 +139,7 @@ compactor:
   # blocks will be marked for deletion and compactor component will permanently
   # delete blocks marked for deletion from the bucket. If 0, blocks will be
   # deleted straight away. Note that deleting blocks immediately can cause query
-  # failures, if store gateway still has the block loaded, or compactor is
-  # ignoring the deletion because it's compacting the block at the same time.
+  # failures.
   # CLI flag: -compactor.deletion-delay
   [deletion_delay: <duration> | default = 12h]
 

docs/configuration/config-file-reference.md

@@ -4009,8 +4009,7 @@ The `compactor_config` configures the compactor for the blocks storage.
 # blocks will be marked for deletion and compactor component will permanently
 # delete blocks marked for deletion from the bucket. If 0, blocks will be
 # deleted straight away. Note that deleting blocks immediately can cause query
-# failures, if store gateway still has the block loaded, or compactor is
-# ignoring the deletion because it's compacting the block at the same time.
+# failures.
 # CLI flag: -compactor.deletion-delay
 [deletion_delay: <duration> | default = 12h]
 

pkg/compactor/blocks_cleaner.go

@@ -26,7 +26,7 @@ type BlocksCleanerConfig struct {
 	DeletionDelay                      time.Duration
 	CleanupInterval                    time.Duration
 	CleanupConcurrency                 int
-	BlockDeletionMarksMigrationEnabled bool // TODO Remove in Cortex 1.8.0 and document that upgrading to 1.7.0 before 1.8.0 is required.
+	BlockDeletionMarksMigrationEnabled bool // TODO Discuss whether we should remove it in Cortex 1.8.0 and document that upgrading to 1.7.0 before 1.8.0 is required.
 }
 
 type BlocksCleaner struct {

pkg/compactor/compactor.go

@@ -87,8 +87,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	f.BoolVar(&cfg.ShardingEnabled, "compactor.sharding-enabled", false, "Shard tenants across multiple compactor instances. Sharding is required if you run multiple compactor instances, in order to coordinate compactions and avoid race conditions leading to the same tenant blocks simultaneously compacted by different instances.")
 	f.DurationVar(&cfg.DeletionDelay, "compactor.deletion-delay", 12*time.Hour, "Time before a block marked for deletion is deleted from bucket. "+
 		"If not 0, blocks will be marked for deletion and compactor component will permanently delete blocks marked for deletion from the bucket. "+
-		"If 0, blocks will be deleted straight away. Note that deleting blocks immediately can cause query failures, "+
-		"if store gateway still has the block loaded, or compactor is ignoring the deletion because it's compacting the block at the same time.")
+		"If 0, blocks will be deleted straight away. Note that deleting blocks immediately can cause query failures.")
 	f.BoolVar(&cfg.BlockDeletionMarksMigrationEnabled, "compactor.block-deletion-marks-migration-enabled", true, "When enabled, at compactor startup the bucket will be scanned and all found deletion marks inside the block location will be copied to the markers global location too. This option can (and should) be safely disabled as soon as the compactor has successfully run at least once.")
 
 	f.Var(&cfg.EnabledTenants, "compactor.enabled-tenants", "Comma separated list of tenants that can be compacted. If specified, only these tenants will be compacted by compactor, otherwise all tenants can be compacted. Subject to sharding.")

pkg/storage/tsdb/bucketindex/index.go

@@ -43,7 +43,7 @@ type Index struct {
 	UpdatedAt int64 `json:"updated_at"`
 }
 
-// RemoveBlock from the index.
+// RemoveBlock removes block and its deletion mark (if any) from index.
 func (idx *Index) RemoveBlock(id ulid.ULID) {
 	for i := 0; i < len(idx.Blocks); i++ {
 		if idx.Blocks[i].ID == id {

pkg/storage/tsdb/bucketindex/markers.go

@@ -78,7 +78,7 @@ func MigrateBlockDeletionMarksToGlobalLocation(ctx context.Context, bkt objstore
 		// Upload it to the global markers location.
 		uploadErr := userBucket.Upload(ctx, BlockDeletionMarkFilepath(blockID), reader)
 		if closeErr := reader.Close(); closeErr != nil {
-			errs.Add(uploadErr)
+			errs.Add(closeErr)
 		}
 		if uploadErr != nil {
 			errs.Add(uploadErr)

pkg/storage/tsdb/bucketindex/storage.go

@@ -50,8 +50,7 @@ func ReadIndex(ctx context.Context, bkt objstore.Bucket, userID string, logger l
 	return index, nil
 }
 
-// WriteIndex generates the bucket index and writes it to the storage. If the old index is not
-// passed in input, then the bucket index will be generated from scratch.
+// WriteIndex uploads the provided index to the storage.
 func WriteIndex(ctx context.Context, bkt objstore.Bucket, userID string, idx *Index) error {
 	bkt = bucket.NewUserBucketClient(userID, bkt)