-
Notifications
You must be signed in to change notification settings - Fork 339
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(cache) Don't cache failed calls to resourceStore #1894
Merged
jakubdyszkiewicz
merged 6 commits into
kumahq:master
from
lahabana:fix/cacheCachingErrors
Apr 30, 2021
+343
−95
Merged
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
18baef9
fix(cache) Don't cache failed calls to resourceStore
7309523
Move everything common between cla and mesh cache in a common cache i…
3b740e3
Improve api of once.Cache
a0c2919
Merge branch 'master' of github.com:kumahq/kuma into fix/cacheCaching…
58ac623
Merge branch 'master' into fix/cacheCachingErrors
mergify[bot] 9204da8
Correct typo and bug in which we might call get twice
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| package once | ||
|
|
||
| import ( | ||
| "context" | ||
| "time" | ||
|
|
||
| "github.com/patrickmn/go-cache" | ||
| "github.com/prometheus/client_golang/prometheus" | ||
|
|
||
| "github.com/kumahq/kuma/pkg/metrics" | ||
| ) | ||
|
|
||
| type Cache struct { | ||
| cache *cache.Cache | ||
| onceMap *omap | ||
| metrics *prometheus.GaugeVec | ||
| } | ||
|
|
||
| func New(expirationTime time.Duration, name string, metrics metrics.Metrics) (*Cache, error) { | ||
| metric := prometheus.NewGaugeVec(prometheus.GaugeOpts{ | ||
| Name: name, | ||
| Help: "Summary of " + name, | ||
| }, []string{"operation", "result"}) | ||
| if err := metrics.Register(metric); err != nil { | ||
| return nil, err | ||
| } | ||
| return &Cache{ | ||
| cache: cache.New(expirationTime, time.Duration(int64(float64(expirationTime)*0.9))), | ||
| onceMap: newMap(), | ||
| metrics: metric, | ||
| }, nil | ||
| } | ||
|
|
||
| func (c *Cache) Get(ctx context.Context, key string, fn func(context.Context, string) (interface{}, error)) (interface{}, error) { | ||
| v, found := c.cache.Get(key) | ||
| if found { | ||
| c.metrics.WithLabelValues("get", "hit").Inc() | ||
| return v, nil | ||
| } | ||
| o := c.onceMap.Get(key) | ||
| c.metrics.WithLabelValues("get", "hit-wait").Inc() | ||
| o.Do(func() (interface{}, error) { | ||
| defer c.onceMap.Delete(key) | ||
| c.metrics.WithLabelValues("get", "hit-wait").Dec() | ||
| c.metrics.WithLabelValues("get", "miss").Inc() | ||
| res, err := fn(ctx, key) | ||
| if err != nil { | ||
| c.metrics.WithLabelValues("get", "error").Inc() | ||
| return nil, err | ||
| } | ||
| c.cache.SetDefault(key, res) | ||
| return res, nil | ||
| }) | ||
| if o.Err != nil { | ||
| return "", o.Err | ||
| } | ||
| return o.Value, nil | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Yeah, there was duplication indeed, but it's quite problematic to introduce a well-understandable and reusable abstraction here. I don't mind going with this
once.Cacheabstraction, but we have to do something with the interface because it's really unclear whatGetdoes.Maybe we can introduce new types:
and rename
GettoGetOrRefreshso it will look like:and document this method with a comment something like:
WDYT?
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.
Sounds good!
How about
GetOrCompute()as Refresh sounds like it will only work when the item has been in the cache?For the
Keywhat's the motivation for adding the subtype?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.
The main reason for questioning the introduction of subtypes is that the underlying lib
go-cacheusesstringas key andinterface{}as value.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.
In the end went for
GetOrRetrieveand didn't add subtypes forkeyandvalue. Let me know what you think.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 like
GetOrRetrieve👍Subtypes help to make the signature more readable. If you define it like:
then it's not clear that
FetcherFunctakeskeyas a parameter and returnsvalue.Also,
interface{}is used twice - as a return value ofGetOrRetrieveand as a return value ofFetcherFunc. I think it makes sense to show that thoseinterface{}are the same type. For example, if in the future we would like to introduce a new type forValue.But I don't mind going with just
stringandinterface{}