Add support for caching based on compaction level and block age

[annanay25]

What this PR does:
Adds support for caching based on compaction level and max block age. Ideally, we want to cache bloom filters of blocks that will be around longer - so blocks which reach higher levels of compaction soon.

Which issue(s) this PR fixes:
Fixes #na!

Checklist

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

[annanay25]

There's one last thing left to implement: remove any caching while writing objects at the ingester. At the compactor we can have a similar shouldCache function as implemented for querier.

[joe-elliott]

Don't we need to merge #818 first before this?

remove any caching while writing objects at the ingester

Why wouldn't we cache objects written by the ingester?

[annanay25]

Don't we need to merge #818 first before this?

That's right. 818 is cleaned up and ready to review.

Why wouldn't we cache objects written by the ingester?

My bad, not remove caching, the write methods on ingester/compactor will use ShouldCache() to decide whether the objects should be cached.

[kvrhdn]

It would be nice if we could shield off direct access to r, w, uncachedReader and uncachedWriter. So you are always forced to call getReaderForBlock and getWriterForBlock.
Unfortunately we can't shield private fields because everything is in the same package.

I think we should either add a comment to warn you or somehow combine these fields into a new struct that knows what to cache.

When I search for usage of r and w I still find some code using them directly (so without getXxxForBlock). I don't know if this is okay.

r:

• compactor.go:154 (compact)
• compactor.go:160 (compact)
• tempodb.go:208 (CompleteBlock)

w:

• compactor.go:251 (appendBlock)
• tempodb.go:208 (CompleteBlock)

[mdisibio]

Some cases are unavoidable, such as compactor.go:154 to load the block meta from the backend, so the fields will have to remain accessible. compactor:160 and :251 are ok because they are iterating or flushing data, which does not involve the bloom filter. tempodb.go:208 actually looks like obsolete code only used by a querier test. The ingester used to call this when iterating the wal, but now it calls CompleteBlockWithBackend.

[kvrhdn]

Why is compactor.go:154 unavoidable? Using r directly means we read using the cache, what's the difference with using the reader we get from getReaderForBlock?

[mdisibio]

You're right. I was thinking that there will be cases where the meta is not available, i.e. a call to get the meta, but in this case the compactor does already have it.

[kvrhdn]

Overall changes look good and are easy to follow. It's fine for me to merge now and address the comments later.