Implement TTL for cached datasource instances #1230
Conversation
iwysiu
force-pushed
the
iwysiu/ds/329
branch
from
0c13d1d to
5b8ea72
Compare
iwysiu
changed the title
iwysiu
requested review from
andresmgot,
oshirohugo and
briangann
and removed request for
a team
| strKey := fmt.Sprintf("%v", cacheKey) | ||
| im.locker.RLock(strKey) | ||
| item, ok := im.cache.Get(strKey) | ||
| im.locker.RUnlock(strKey) |
There was a problem hiding this comment.
the shared read locking here looks unnecessary. even though your PR didn't introduce it, I'd be in favour of removing it.
| } | ||
|
|
||
| instance, err := im.provider.NewInstance(ctx, pluginContext) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| im.cache.Store(cacheKey, CachedInstance{ | ||
| im.cache.Set(strKey, CachedInstance{ |
There was a problem hiding this comment.
looks like there's a cache.SetDefault() method for the 'set this and use the default expiration' case.
| } else { | ||
| activeInstances.Dec() | ||
| } | ||
| im.cache.Delete(strKey) |
There was a problem hiding this comment.
It may be a little faster to avoid this delete, make a function for the disposer code, and call that function from here and in the OnEvicted handler.
There was a problem hiding this comment.
I think the added complexity probably isn't worth it here - Delete here does acquire a lock, but we'll only hit this code path when the datasource config or Grafana config are updated, so quite rarely in the scheme of things.
There was a problem hiding this comment.
Looks good overall, but I do think it would be better if we can implement this as "expire after idle time" rather than "expire after creation". This would let us set a much shorter expiration time, maybe an hour or fifteen minutes even, and make the whole thing much more effective (and a tiiiny bit less disruptive).
Fortunately that looks pretty straightforward! I think all we need to do to is to add
im.cache.SetDefault(strKey, ci)before the returns on lines 127 and 141. This would reset the expiration for the item every time it's retrieved. There's no harm here if multiple threads do this within a short period of time, as they'll all be accomplishing essentially the same thing, so there's no need for extra locking.
| } | ||
|
|
||
| tip := &testInstanceProvider{} | ||
| im := NewWithOptions(tip, time.Millisecond, 2*time.Millisecond, defaultDisposeTTL) |
There was a problem hiding this comment.
If I'm reading this correctly, this test will take 5 seconds - should we use a smaller dispose TTL here?
| } else { | ||
| activeInstances.Dec() | ||
| } | ||
| im.cache.Delete(strKey) |
There was a problem hiding this comment.
I think the added complexity probably isn't worth it here - Delete here does acquire a lock, but we'll only hit this code path when the datasource config or Grafana config are updated, so quite rarely in the scheme of things.
What this PR does / why we need it:
Investigation for implementing caching for datasource instances. It's pretty simple and straightforward. I did this using a popular caching library that we already use in core grafana.
There are a couple minor implementation notes/questions:
InstanceManagerWithExpirationtype if we wanted to avoid thesprintfWhich issue(s) this PR fixes:
Fixes https://github.com/grafana/data-sources/issues/329
Special notes for your reviewer:
Opening this pr for review to get it in front of eyes that may have opinions about the implementation questions