Don't load series multiple times when streaming chunks from store-gateways and only one batch is needed

[charleskorn]

What this PR does

Acknowledgement: this has been a joint effort between myself and @zenador

This PR improves the performance of store-gateways when streaming chunks to queriers.

Specifically, it improves the performance of the case where a Select() call selects no more than than -blocks-storage.bucket-store.batch-series-size series (default value is 5000).

Currently this case behaves as follows (assuming the index-cache is empty initially):

• When a Select() call arrives, the store-gateway needs to first send all selected series' labels to the querier:
  • It calls BucketStore.streamingSeriesForBlocks(), which creates an iterator for the seriesChunkRefsSets for the query. Each seriesChunkRefsSet contains at most -blocks-storage.bucket-store.batch-series-size series.
  • As this iterator is consumed, the series in each seriesChunkRefsSet are loaded and cached as SeriesForRef items in the index-cache. These cache entries are written asynchronously.
• Once all labels have been sent, it then begins streaming the chunks for those series:
  • This calls BucketStore.streamingChunksSetForBlocks(), which creates the same iterator again (albeit this time asking for chunk refs to be loaded as well)
  • As this iterator is consumed, it will try to fetch the cached series, and if a cache miss occurs, falls back to loading these directly as before.

This last part is the opportunity for improvement: it's very likely that the asynchronous cache write is yet to complete at this point, so the store-gateway will end up loading the series again. However, in the case where the query only needs one batch, recreating the batch is unnecessary: we can just keep the batch in memory and reuse it for both sending labels and sending chunks. (We can't do this if multiple batches are required: that would require keeping all batches in memory, which we are trying to avoid.)

⚠️ It has taken me quite a while for me to get my head around this part of the store-gateway - please review this change carefully, don't assume I know what I'm doing.

Which issue(s) this PR fixes or relates to

Fixes the issue discussed in #6646 (comment)

Checklist

• Tests updated.
• [n/a] Documentation added.
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
• [n/a] about-versioning.md updated with experimental features.

[dimitarvdimitrov]

Note to reviewers: I don't love how invasive this change was, open to ideas and feedback on how to better isolate this. Perhaps we could wrap loadingSeriesChunkRefsSetIterator in another iterator type that either caches the single set or recreates the iterator when we start streaming chunks?

I tend to agree. The logic for what is being reset (in streamingChunksSetForBlocks) and how the strategy and "releasability" change is selected (in loadingSeriesChunkRefsSetIterator.Reset()) are very far away in the call chain.

Maybe we can have something like an iterator factory: instead of letting getSeriesIteratorFromBlocks call directly openBlockSeriesChunkRefsSetsIterator we can pass a factory to getSeriesIteratorFromBlocks.

1. The first time getSeriesIteratorFromBlocks invokes the factory the factory creates empty iterators.
2. The factory can make the decision whether there will be only a single batch for the block or multiple (which due to sharding and labels filtering won't be accurate, but it might work most of the time).
3. If there is only a single batch, then the factory returns an iterator which returns unreleasable batch and caches the result.
4. In the second invocation from getSeriesIteratorFromBlocks the factory reuses the cached result and returns a releasable batch.
5. Done?

this way the factory and the caching iterator it returns will be the major place which decide short-term caching, releasability, and the seriesIteratorStrategy

[dimitarvdimitrov]

nice one! I also like this approach better. it looks good for the most part, the only major thing is that the postings are no longer copied before reusing them. Can you double-check if that's causing bugs?

[charleskorn]

I have tested this in a test cluster at Grafana Labs and it seems to work fine, so I think this is ready for a final review before merging 🎉

[dimitarvdimitrov]

LGTM, awesome work @charleskorn and @zenador!