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 tests for index cache with query sharding #3728

Merged
merged 6 commits into from
Dec 15, 2022

Conversation

dimitarvdimitrov
Copy link
Contributor

Adds tests for when running the streaming implementation with query
sharding and index cache. This is a follow-up of #3687

This PR also includes some changes to interfaces so that it's easier to mock the hash of a series.

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

Adds tests for when running the streaming implementation wth query
sharding and index cache. This is a followup of PR 3687

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
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.

The test you've added looks great!

type cachedSeriesHasher struct {
cache *hashcache.BlockSeriesHashCache
}

func (b cachedSeriesHasher) Fetch(seriesID storage.SeriesRef) (uint64, bool) {
return b.cache.Fetch(seriesID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just realised we were/are not tracking stats.seriesHashCacheRequests and stats.seriesHashCacheHits in filterPostingsByCachedShardHash(), but I think we should.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we are. here

stats.seriesHashCacheHits++

are we talking about the same place 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and now i've changed it in 7453753 so that the stats are passed as a pointer to filterPostingsByCachedShardHash and it passes it down to cachedSeriesHasher, which in turn updates the stats

Comment on lines 1132 to 1133
blockSeriesHashCache,
cachedSeriesHasher{blockSeriesHashCache},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why we need both? Can we only pass one of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

filterPostingsByCachedShardHash can only look at the cache. I kept them as two so that I don't change the default implementation.

This commit makes them one 7453753 and changes the default implementaiton

…iesCaching

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
@dimitarvdimitrov dimitarvdimitrov force-pushed the dimitar/streaming-index-cache-tests branch from 16f75cf to 7453753 Compare December 15, 2022 11:04
@dimitarvdimitrov dimitarvdimitrov marked this pull request as ready for review December 15, 2022 11:05
@dimitarvdimitrov dimitarvdimitrov requested a review from a team as a code owner December 15, 2022 11:05
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.

Great job! Nothing to say ;) The series hasher design looks cleaner to me now. 👏

@pracucci pracucci merged commit bada69c into main Dec 15, 2022
@pracucci pracucci deleted the dimitar/streaming-index-cache-tests branch December 15, 2022 14:40
masonmei pushed a commit to udmire/mimir that referenced this pull request Dec 16, 2022
grafana#3728)

* streaming store-gateway: add tests for index cache with query sharding

Adds tests for when running the streaming implementation wth query
sharding and index cache. This is a followup of PR 3687

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

* Remove unnecessary short circuit

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

* Remove unnecessary seriesHashCache interface

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

* Make a note for scope of TestOpenBlockSeriesChunkRefsSetsIterator_SeriesCaching

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

* Revert "Remove unnecessary seriesHashCache interface"

This reverts commit fd30b11.

* Pass a single struct to calculate and cache hashes of series

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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants