Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Streaming store-gateway: add iterators for loading series from index #3645

Merged
merged 3 commits into from
Dec 6, 2022

Conversation

dimitarvdimitrov
Copy link
Contributor

We are adding three iterators:

  • loadingSeriesChunkRefsSetIterator is responsible for loading chunks
    from the index
  • postingsSetsIterator is responsible for batching postings so loading
    series from the index doesn't load all labels and chunk refs from the
    index in one go
  • limitingSeriesChunkRefsSetIterator is responsible for limiting
    series and chunks within a request

We are also adding a function to set up these three iterators for an
incoming Series() request for a block (openBlockSeriesChunkRefsSetsIterator).

Note to reviewers: this PR uses the changes in #3644, so it's the base branch of this PR. After merging #3644, the base branch of this PR should change to main automatically

Co-authored-by: Marco Pracucci marco@pracucci.com
Signed-off-by: Dimitar Dimitrov dimitar.dimitrov@grafana.com

@dimitarvdimitrov dimitarvdimitrov requested a review from a team as a code owner December 5, 2022 14:13
Base automatically changed from dimitar/store-gateway-e2e-test-changes to main December 5, 2022 14:42
@dimitarvdimitrov dimitarvdimitrov force-pushed the dimitar/upstream-index-loading-iterators branch from e0dd8c9 to e361abf Compare December 5, 2022 14:44
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

maxTime,
)

limitingIterator := newLimitingSeriesChunkRefsSetIterator(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[non blocking] openBlockSeriesChunkRefsSetsIterator() is per block. This means that we apply the series limiter for every series and block. If the same series is queried from multiple blocks, then the series is double counted. This is the same behaviour we currently have in main so this is not a regression, but with the streaming store-gateway we have the opportunity the limiter up the stack and to actually apply on the merged series from all blocks. We can further discuss it and eventually do it in the working branch.

@dimitarvdimitrov dimitarvdimitrov force-pushed the dimitar/upstream-index-loading-iterators branch from 9a2eb66 to dda7f44 Compare December 5, 2022 15:25
@pracucci pracucci enabled auto-merge (squash) December 6, 2022 09:19
@dimitarvdimitrov dimitarvdimitrov force-pushed the dimitar/upstream-index-loading-iterators branch from dda7f44 to 5d313cd Compare December 6, 2022 09:37
dimitarvdimitrov and others added 2 commits December 6, 2022 10:48
We are adding three iterators:
* `loadingSeriesChunkRefsSetIterator` is responsible for loading chunks
from the index
* `postingsSetsIterator` is responsible for batching postings so loading
 series from the index doesn't load all labels and chunk refs from the
 index in one go
* `limitingSeriesChunkRefsSetIterator` is responsible for limiting
 series and chunks within a request

We are also adding a function to set up these three iterators for an
incoming Series() request for a block.

Co-authored-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
@dimitarvdimitrov dimitarvdimitrov force-pushed the dimitar/upstream-index-loading-iterators branch from 5d313cd to 808b3d8 Compare December 6, 2022 09:50
@dimitarvdimitrov dimitarvdimitrov enabled auto-merge (squash) December 6, 2022 10:29
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
@dimitarvdimitrov dimitarvdimitrov force-pushed the dimitar/upstream-index-loading-iterators branch from 808b3d8 to 6f69220 Compare December 6, 2022 10:33
@dimitarvdimitrov dimitarvdimitrov merged commit 04f9181 into main Dec 6, 2022
@dimitarvdimitrov dimitarvdimitrov deleted the dimitar/upstream-index-loading-iterators branch December 6, 2022 10:46
masonmei pushed a commit to udmire/mimir that referenced this pull request Dec 16, 2022
…rafana#3645)

* Streaming store-gateway: add iterators for loading series from index

We are adding three iterators:
* `loadingSeriesChunkRefsSetIterator` is responsible for loading chunks
from the index
* `postingsSetsIterator` is responsible for batching postings so loading
 series from the index doesn't load all labels and chunk refs from the
 index in one go
* `limitingSeriesChunkRefsSetIterator` is responsible for limiting
 series and chunks within a request

We are also adding a function to set up these three iterators for an
incoming Series() request for a block.

Co-authored-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

* Update nolint directives

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

* Fix e2e testing interface used

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants