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

Fix matcher cache #8039

Merged
merged 2 commits into from
Jan 6, 2025
Merged

Fix matcher cache #8039

merged 2 commits into from
Jan 6, 2025

Conversation

alanprot
Copy link
Contributor

@alanprot alanprot commented Jan 3, 2025

Follow up of #7353

On the previous PR, we were using the reference of the matcher as a cache key -> This does not work as for every request, we create new references for those matchers.

This PR is just changing the cache key to be a string.

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

Changes

Change matchers cache key to be an string

Verification

Test was updated.

Signed-off-by: alanprot <alanprot@gmail.com>
@alanprot
Copy link
Contributor Author

alanprot commented Jan 3, 2025

cc @pedro-stanaka

Copy link
Contributor

@pedro-stanaka pedro-stanaka left a comment

Choose a reason for hiding this comment

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

LG

Did not like the need to coerce to string, but we needed it before as well because of the singleflight.

@@ -89,9 +89,9 @@ func NewMatchersCache(opts ...MatcherCacheOption) (*LruMatchersCache, error) {

func (c *LruMatchersCache) GetOrSet(key ConversionLabelMatcher, newItem NewItemFunc) (*labels.Matcher, error) {
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 still want ConversionLabelMatcher in the method itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking the same... maybe better to send the key as string... lemme do that

Copy link
Contributor Author

@alanprot alanprot Jan 3, 2025

Choose a reason for hiding this comment

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

Actually, we need as the key is also used as argument of the NewItemFunc. =/ We can change this function to not receive the key as well i think.. but is a bigger change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yeya24 PTAL? tried to simplify the code a bit.

Signed-off-by: alanprot <alanprot@gmail.com>
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

@alanprot alanprot requested a review from pedro-stanaka January 3, 2025 23:33
Copy link
Contributor

@pedro-stanaka pedro-stanaka left a comment

Choose a reason for hiding this comment

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

lg - thanks for the simplication

@yeya24 yeya24 merged commit bed76cf into thanos-io:main Jan 6, 2025
22 checks passed
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