-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fix matcher cache #8039
Conversation
Signed-off-by: alanprot <[email protected]>
There was a problem hiding this 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.
pkg/store/cache/matchers_cache.go
Outdated
@@ -89,9 +89,9 @@ func NewMatchersCache(opts ...MatcherCacheOption) (*LruMatchersCache, error) { | |||
|
|||
func (c *LruMatchersCache) GetOrSet(key ConversionLabelMatcher, newItem NewItemFunc) (*labels.Matcher, error) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
5653cd5
to
a5634c8
Compare
Signed-off-by: alanprot <[email protected]>
a5634c8
to
6083b3d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
There was a problem hiding this 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
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.
Changes
Change matchers cache key to be an string
Verification
Test was updated.