Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cache: new package #144
Conversation
davecheney
reviewed
Jul 17, 2015
| +// Key represents a cache key. It must be a comparable type. | ||
| +type Key interface{} | ||
| + | ||
| +// Cache holds a time-limited cache of values for arbitrary keys. |
davecheney
reviewed
Jul 17, 2015
| + "gopkg.in/errgo.v1" | ||
| +) | ||
| + | ||
| +type entry struct { |
davecheney
Jul 17, 2015
Contributor
even though this is a private symbol, commenting the type and it's fields wound be nice.
davecheney
reviewed
Jul 17, 2015
| +} | ||
| + | ||
| +// New returns a new Cache that will cache items for | ||
| +// at most maxAge. |
rogpeppe
Jul 20, 2015
Owner
There are no lower bounds. It's rounded up to 2ns but that should affect the caller in practice.
|
LGTM with some documentation nits |
mhilton
reviewed
Jul 20, 2015
| + // actual expiry time will be maxAge - a random value in the | ||
| + // interval [0. maxAge/2). If maxAge is < 2ns then this requires | ||
| + // a random interval in [0, 0) which causes a panic. | ||
| + if maxAge < 2*time.Nanosecond { |
mhilton
Jul 20, 2015
Member
This is a bit clunky for a general purpose package I feel (I know I wrote it). I have thought previously about something like:
// New returns a new cache that will cache items for at most maxAge.
// If f is non nil it will be called before adding a new item into the cache,
// the item's expiry time will be derived from the return value.
func New(maxAge time.Duration, f func(time.Duration) time.Duration) {
...
}what do you think?
mhilton
reviewed
Jul 20, 2015
| +} | ||
| + | ||
| +// Evict removes the entry with the given key from the cache if present. | ||
| +func (c *Cache) Evict(key Key) { |
mhilton
Jul 20, 2015
Member
I'm not sure that this is sufficient. the entry might still be in old and not expired, at which point Evict doesn't evict anything.
rogpeppe
Jul 20, 2015
Owner
excellent point. I've added a test, checked that it failed, then fixed the logic.
|
LGTM, with some API thoughts. |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-utils |
added a commit
that referenced
this pull request
Jul 20, 2015
rogpeppe commentedJul 17, 2015
This provides a simple caching mechanism. We're moving this
from the charmstore package internal packages so that
it can be used elsewhere too.
One change from the original: we allow arbitrary keys
rather than restricting to just strings.
(Review request: http://reviews.vapour.ws/r/2198/)