Write block deletion marks in the global location too

[pracucci]

What this PR does:
This is another preliminary step required to implement the bucket index, as designed in #3554. In this PR:

• Introduce bucket index reader and writer (but not plugged in yet)
• Introduce a bucket client wrapper used to store block deletion marks in the per-tenant global location too
• Plug the bucket client wrapper in the compactor (which is the component deleting blocks)

Which issue(s) this PR fixes:
N/A

Checklist

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

[pstibrany]

Nice job! I have mostly minor nits and comments, nothing blocking.

Unrelated to this PR: have you considered naming bucketindex.Block to BlockMeta.

[pstibrany]

Suggested change

	return block.IsBlockDir(parts[len(parts)-2])
	return block.IsBlockDir(path.Dir(name))

and avoid split.

[pracucci]

It's not the same thing. The object prefix could be /user-1/<block-id>/deletion-mark.json. We need to pick the 2nd last segment, not the whole path.Dir().

[pstibrany]

We need to pick the 2nd last segment, not the whole path.Dir().

block.IsBlockDir already covers that.

[pracucci]

I missed that, sorry. Fixed!

[pstibrany]

We can replace marker reading code in this method with:

	m := metadata.DeletionMark{}
	err := metadata.ReadMarker(ctx, w.logger, w.bkt, id.String(), &m)

(UserBucketClient already implements required InstrumentedBucket)

[pracucci]

Very good point. Done, thanks!

[pracucci]

have you considered naming bucketindex.Block to BlockMeta

Yes. I would prefer to not call it "BlockMeta" to avoid misunderstanding. We don't index the BlockMeta, but some information about the Block and I would prefer not couple it with the meta.json (eg. we may also index block information which is not in the meta.json).

[pracucci]

Thanks a lot @pstibrany for your valuable feedback! I addressed most comments and replied to few other ones. Could you take another look, please?

[pstibrany]

LGTM, nice work.

[lzh-lab]

In bucket_index.json aleady have block ids which is marked delete,
Why save a delete file for every block separately?

[pracucci]

In bucket_index.json aleady have block ids which is marked delete,
Why save a delete file for every block separately?

@AllenzhLi Without a per-tenant global markers/ location we wouldn't have an easy way to find out which blocks have been marked for deletion. Without that, we would have to check the deletion mark for every single block with 1 GET object operation per block (which is what we do right now). With the global location, we can just issue 1 LIST objects API call to find all blocks marked for deletion.

[lzh-lab]

Thank you for your reply.

I have read the design doc, in the doc

In the bucket_index.json have the list of block deletion marks, why not use it?

[pstibrany]

In the bucket_index.json have the list of block deletion marks, why not use it?

Bucket index JSON is created based on found blocks and deletion marks. Querier and store-gateway only use index (and ignore blocks marked for deletion). During compaction, compactor will only create deletion marks, but not update the index. Index update is a separate loop, that looks for deletion marks.

[lzh-lab]

Understood, thanks.