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: Implement metadata API limit in stores #7652

Merged
merged 2 commits into from
Sep 3, 2024

Conversation

harry671003
Copy link
Contributor

@harry671003 harry671003 commented Aug 20, 2024

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

Changes

This PR is part of #7575 and implements the metadata API limit in stores.

  • The querier will now pass the limit parameter to the stores.
  • The stores will return limited results back to querier.

Verification

  • Added Unit tests and updated e2e tests.

@harry671003 harry671003 force-pushed the implement_limit_in_stores branch 2 times, most recently from fadedb9 to d31ed17 Compare August 20, 2024 18:21
@harry671003 harry671003 changed the title Implement metadata API limit in stores store: Implement metadata API limit in stores Aug 20, 2024
@harry671003 harry671003 force-pushed the implement_limit_in_stores branch 2 times, most recently from 56f8311 to d4cea97 Compare August 21, 2024 17:57
@harry671003 harry671003 marked this pull request as ready for review August 21, 2024 22:40
@@ -1945,8 +1958,13 @@ func (s *BucketStore) LabelNames(ctx context.Context, req *storepb.LabelNamesReq
return nil, status.Error(codes.Unknown, errors.Wrap(err, "marshal label names response hints").Error())
}

names := strutil.MergeSlices(sets...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we think it is worth it to pass limit into strutil.MergeSlices and truncate there?

func MergeSlices(a ...[]string) []string {
	if len(a) == 0 {
		return nil
	}
	if len(a) == 1 {
		return a[0]
	}
	l := len(a) / 2
	return mergeTwoStringSlices(MergeSlices(a[:l]...), MergeSlices(a[l:]...))
}

For example, if the limit is < len(a) / 2, we can skip merging the second half of slice.

Copy link
Contributor

Choose a reason for hiding this comment

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

But this is not a blocker, we can do it in further prs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Ben. I can look into it. Since this PR has gotten too big, I prefer to do in a follow up PR. :)

pkg/store/bucket.go Outdated Show resolved Hide resolved
Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>
Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>
Copy link
Contributor

@MichaHoffmann MichaHoffmann left a comment

Choose a reason for hiding this comment

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

Lgtm!

@MichaHoffmann
Copy link
Contributor

Can we add acceptance tests in a followup PR? Just to make sure that all store implementations support this

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good. Agree with @MichaHoffmann that we need to update acceptance test in next pr.

@yeya24 yeya24 merged commit 2c488dc into thanos-io:main Sep 3, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants