streaming store-gateway: Polish batchSetsForBlocks #3651
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* rename to batchedSeriesSetForBlocks * remove unused cleanups * move to bucket.go * some formatting * also remove cleanups from synchronousSeriesSet Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
dimitarvdimitrov
force-pushed
the
dimitar/store-gateway-async-series-move-opening-function
branch
from
f03c20b to
f94360e
Compare
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
pracucci
deleted the
dimitar/store-gateway-async-series-move-opening-function
branch
dimitarvdimitrov
added a commit
that referenced
this pull request
Dec 6, 2022
This is a POC and is not meant to be merged yet. Overview Instead of loading all series (label sets and chunks) in memory before responding to a Series() RPC, we can batch them and load X at a time. This gives more predictability on the memory utilization of the store-gateway. The tradeoff is having to do one trip to the index cache and the bucket for each batch, which will affect overall latency of requests. How to use This change disables batch series loading by default and adds two flags to control this - whether it's enabled via `-blocks-storage.bucket-store.batched-series-loading=false` and how many series go into each batch via `-blocks-storage.bucket-store.batch-series-size=65536`. Limiting Ideally we want ot put a limit on the number of bytes that we want to load in each batch instead of the number of series. For now limiting the number of series should still give us some resilience against "big" requests, while still being vulnerable to a flurry of many requests. Testing I've changed all tests within pkg/storegateway to use this new loading strategy. This should give confidence that it is producing correct results. Further work should improve testing around resource utilization (i.e. batches are indeed freed one after the other) and should test both batched and non-batched strategies. This commit has TODOs, which should be addressed before merging this. Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> Signed-off-by: Marco Pracucci <marco@pracucci.com> Fix which context we use for openBlockSeriesChunkRefsSetsIterator Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> Streaming store-gateway: Split up blockSeriesChunkRefsSetIterator (#3641) Merge main into dimitar/store-gateway-async-series (#3642) Streaming store-gateway: Move seriesHasher (#3643) Move test limiter Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> streaming store-gateway: move and rename loadingBatchSet (#3650) Renames `loadingBatchSet` to `loadingSeriesChunksSetIterator` and moves it to `series_chunks.go` Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> streaming store-gateway: add preloading for series without chunks (#3649) * streaming store-gateway: add preloading for series without chunks This PR * adds preloading to the seriesSet without chunks * changes the implementation of `preloadingSeriesChunkSetIterator` to be a generic one, so we can reuse it for preloading `seriesChunkRefsSet`s too. * moves some tests of `preloadingSeriesChunkSetIterator` from `batch_series_test.go` to `series_chunks_test.go` * moves `newSeriesSetWithChunks` to `series_chunks.go` Question/note to reviewers: should this iterator be in a new file? I think putting it in preload.go and tests in preload_test.go sounds ok since it now applies to both series_chunks.go and series_refs.go. Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> streaming store-gateway: Polish batchSetsForBlocks (#3651) * Polish batchSetsForBlocks * rename to batchedSeriesSetForBlocks * remove unused cleanups * move to bucket.go * some formatting * also remove cleanups from synchronousSeriesSet Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> * Rename batchedSeriesSetForBlocks to streamingSeriesSetForBlocks Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> Co-authored-by: Marco Pracucci <marco@pracucci.com> Merge main into dimitar/store-gateway-async-series (#3655) Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cleanupsbucket.gocleanupsfrom synchronousSeriesSet