Repository navigation
feat(cache): implement per-table tagging and raw SQL bypass #119
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
Changes from 4 commits
5d0ec08
0e95298
87d578e
d0c2880
a38fa69
41efec7
8960bb2
1347c02
8fc97a6
79f4258
9afeefa
f73c424
1148a02
a208da9
4e50cab
86e85eb
976f793
2898cb2
f5269d8
e0c9bbb
0f606a3
63e3b70
47aff71
3604b56
735d060
fe97e13
d8f7b5c
755985a
9b522c8
348b5ea
593304a
2652fd3
d1d18a0
eff476d
28622d4
9c1bb4f
57d4e19
24bcf54
246a1fa
9052b8b
381d3d3
c577ab8
ac2ed2d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would like to talk about this file and help understanding it in-person review later, lot's of questions/confusion on it and could use a walkthrough |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,8 +10,9 @@ import ( | |
|
|
||
| // LocalCache is an L1 in-process cache backed by Ristretto. | ||
| type LocalCache struct { | ||
| cache *ristretto.Cache[string, []byte] | ||
| ttls sync.Map // key -> expiry time.Time | ||
| cache *ristretto.Cache[string, []byte] | ||
| ttls sync.Map // key -> expiry time.Time | ||
| tagsMap sync.Map // tag (string) -> *sync.Map (key -> struct{}) | ||
| } | ||
|
|
||
| // NewLocal creates a new Ristretto-backed local cache. | ||
|
|
@@ -34,27 +35,53 @@ func (l *LocalCache) Get(_ context.Context, key string) ([]byte, time.Duration, | |
| } | ||
|
|
||
| var remaining time.Duration | ||
| if exp, ok := l.ttls.Load(key); ok { | ||
| remaining = time.Until(exp.(time.Time)) | ||
| if remaining <= 0 { | ||
| l.cache.Del(key) | ||
| l.ttls.Delete(key) | ||
| return nil, 0, nil | ||
| if expVal, ok := l.ttls.Load(key); ok { | ||
| exp := expVal.(time.Time) | ||
| if !exp.IsZero() { | ||
| remaining = time.Until(exp) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @EricAndrechek I've made Get do a expired key cleanup, but I'm not sure if having a routine do this be a better option. In terms of overall efficiency, a background routine would ensure we don't leave 'stale links' in our tag maps for data that is never asked for again. However, I’m worried that a background worker might slow down the system by locking the cache frequently, whereas the current approach only does the work when someone actually needs the data. |
||
| if remaining <= 0 { | ||
| l.cache.Del(key) | ||
| l.ttls.Delete(key) | ||
|
taitelee marked this conversation as resolved.
Resolved
|
||
| return nil, 0, nil | ||
| } | ||
|
taitelee marked this conversation as resolved.
Resolved
taitelee marked this conversation as resolved.
Resolved
|
||
| } | ||
| } | ||
| return val, remaining, nil | ||
| } | ||
|
|
||
| func (l *LocalCache) Set(_ context.Context, key string, value []byte, ttl time.Duration) error { | ||
| func (l *LocalCache) Set(_ context.Context, key string, value []byte, ttl time.Duration, tags []string) error { | ||
| l.cache.SetWithTTL(key, value, int64(len(value)), ttl) | ||
| var exp time.Time | ||
| if ttl > 0 { | ||
| l.ttls.Store(key, time.Now().Add(ttl)) | ||
| exp = time.Now().Add(ttl) | ||
| } | ||
| l.ttls.Store(key, exp) | ||
|
taitelee marked this conversation as resolved.
Resolved
|
||
|
|
||
| // Register key under each tag | ||
| for _, tag := range tags { | ||
| m, _ := l.tagsMap.LoadOrStore(tag, &sync.Map{}) | ||
| m.(*sync.Map).Store(key, struct{}{}) | ||
|
taitelee marked this conversation as resolved.
Resolved
|
||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (l *LocalCache) InvalidateByTags(_ context.Context, tags []string) error { | ||
|
taitelee marked this conversation as resolved.
Resolved
|
||
| for _, tag := range tags { | ||
| if keysMap, ok := l.tagsMap.Load(tag); ok { | ||
| // Iterate through all keys associated with this tag | ||
| keysMap.(*sync.Map).Range(func(key, _ any) bool { | ||
| k := key.(string) | ||
| l.cache.Del(k) | ||
|
taitelee marked this conversation as resolved.
Resolved
|
||
| l.ttls.Delete(k) | ||
| return true | ||
| }) | ||
| // Clear the tag index entirely | ||
| l.tagsMap.Delete(tag) | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
taitelee marked this conversation as resolved.
Resolved
taitelee marked this conversation as resolved.
Resolved
|
||
|
|
||
| // Wait blocks until all buffered writes have been applied. | ||
| // Exposed for testing; production callers rarely need this. | ||
| func (l *LocalCache) Wait() { | ||
| l.cache.Wait() | ||
| } | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See comments in file, but this needs revisions, should build it as if fully implemented/supported |
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 really don't like all the regex in this file... it scares me... also worries me that is may go out of sync with regex we have in the internal
querypackage. How much of this is needed here vs should/could be shared and may be useful more generally, and/or what of this do you think will be removed anyway when we work on #167?