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

store gateway: fix merge fetched postings with lazy postings #7979

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Dec 10, 2024

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

When fetching postings at Store Gateway, we sort posting groups by their cardinality and then fetch postings for each groups. And when a posting group is marked as lazy, then all posting groups have higher cardinality are also marked as lazy.

But since #7961, we start to mark posting groups to lazy if they fetch a lot of keys, which means we can have non consecutive lazy posting groups.

This introduces a bug in fetchAndExpandPostingGroups where we stop expanding postings when encoutering the first lazy posting group. We lack testing for this code so I extract that part of logic to mergeFetchedPostings and create a test TestMergeFetchedPostings to cover it.

Verification

@@ -313,7 +325,7 @@ func fetchAndExpandPostingGroups(ctx context.Context, r *bucketIndexReader, post
var groupAdds, groupRemovals []index.Postings
for _, g := range postingGroups {
if g.lazy {
break
continue
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the crux...
We break the for loop before when group is lazy but now we continue

},
expectedSeriesRefs: []storage.SeriesRef{1, 2, 3, 4, 5},
},
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test case will fail on main

@alanprot
Copy link
Contributor

Good catch! thanks

@yeya24
Copy link
Contributor Author

yeya24 commented Dec 10, 2024

Given the approvals from @harry671003 and @alanprot, I will go ahead and merge this fix

@yeya24 yeya24 merged commit 0ea6bac into thanos-io:main Dec 10, 2024
21 of 22 checks passed
@yeya24 yeya24 deleted the fix-merge-fetchedpostings-lazy branch December 11, 2024 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants