From 2fd2fba6254cd23fd61670db7859c5c7747a3832 Mon Sep 17 00:00:00 2001 From: Dmitry Tyzhnenko Date: Mon, 14 Oct 2019 00:17:12 +0300 Subject: [PATCH 1/6] Add RedisCache Issue #3 --- go.mod | 1 + go.sum | 2 + lru_cache.go | 2 +- options.go | 3 ++ redis_cache.go | 119 +++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 126 insertions(+), 1 deletion(-) create mode 100644 redis_cache.go diff --git a/go.mod b/go.mod index 21bd98b..4872ee2 100644 --- a/go.mod +++ b/go.mod @@ -2,6 +2,7 @@ module github.com/go-pkgz/lcw require ( github.com/davecgh/go-spew v1.1.1 // indirect + github.com/go-redis/redis v6.15.6+incompatible github.com/hashicorp/golang-lru v0.5.0 github.com/patrickmn/go-cache v2.1.0+incompatible github.com/pkg/errors v0.8.1 diff --git a/go.sum b/go.sum index 57d795f..40d1d23 100644 --- a/go.sum +++ b/go.sum @@ -2,6 +2,8 @@ github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/go-redis/redis v6.15.6+incompatible h1:H9evprGPLI8+ci7fxQx6WNZHJSb7be8FqJQRhdQZ5Sg= +github.com/go-redis/redis v6.15.6+incompatible/go.mod h1:NAIEuMOZ/fxfXJIrKDQDz8wamY7mA7PouImQ2Jvg6kA= github.com/hashicorp/golang-lru v0.5.0 h1:CL2msUPvZTLb5O648aiLNJw3hnBxN2+1Jq8rCOH9wdo= github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8= github.com/patrickmn/go-cache v2.1.0+incompatible h1:HRMgzkcYKYpi3C8ajMPV8OFXaaRUnok+kx1WdO15EQc= diff --git a/lru_cache.go b/lru_cache.go index 6887550..d5b6b8e 100644 --- a/lru_cache.go +++ b/lru_cache.go @@ -7,7 +7,7 @@ import ( "github.com/pkg/errors" ) -// LruCache wraps lru.LruCache with laoding cache Get and size limits +// LruCache wraps lru.LruCache with loading cache Get and size limits type LruCache struct { options CacheStat diff --git a/options.go b/options.go index 4ee964a..93cb129 100644 --- a/options.go +++ b/options.go @@ -3,6 +3,8 @@ package lcw import ( "errors" "time" + + "github.com/go-redis/redis" ) type options struct { @@ -12,6 +14,7 @@ type options struct { maxCacheSize int64 ttl time.Duration onEvicted func(key string, value Value) + redis *redis.Options } // Option func type diff --git a/redis_cache.go b/redis_cache.go new file mode 100644 index 0000000..063d1dc --- /dev/null +++ b/redis_cache.go @@ -0,0 +1,119 @@ +package lcw + +import ( + "sync/atomic" + "time" + + "github.com/go-redis/redis" + "github.com/pkg/errors" +) + +// RedisCache implements LoadingCache for Redis. +type RedisCache struct { + options + CacheStat + backend *redis.Client + currentSize int64 +} + +// NewRedisCache makes Redis LoadingCache implementation. +func NewRedisCache(opts ...Option) (*RedisCache, error) { + + res := RedisCache{ + options: options{ + ttl: 5 * time.Minute, + }, + } + for _, opt := range opts { + if err := opt(&res.options); err != nil { + return nil, errors.Wrap(err, "failed to set cache option") + } + } + + res.backend = redis.NewClient(res.redis) + + return &res, nil +} + +// Get gets value by key or load with fn if not found in cache +func (c *RedisCache) Get(key string, fn func() (Value, error)) (data Value, err error) { + + v, ok := c.backend.Get(key).Result() + if ok == nil { + atomic.AddInt64(&c.Hits, 1) + return v, nil + } + if ok == redis.Nil { + if data, err = fn(); err != nil { + atomic.AddInt64(&c.Errors, 1) + return data, err + } + } else if ok != nil { + atomic.AddInt64(&c.Errors, 1) + return v, ok + } + atomic.AddInt64(&c.Misses, 1) + + if c.allowed(key, data) { + c.backend.Set(key, data, c.ttl) + } + return data, nil +} + +// Invalidate removes keys with passed predicate fn, i.e. fn(key) should be true to get evicted +func (c *RedisCache) Invalidate(fn func(key string) bool) { + for _, key := range c.backend.Keys("*").Val() { // Keys() returns copy of cache's key, safe to remove directly + if fn(key) { + c.backend.Del(key) + } + } +} + +// Peek returns the key value (or undefined if not found) without updating the "recently used"-ness of the key. +func (c *RedisCache) Peek(key string) (Value, bool) { + ret, err := c.backend.Get(key).Result() + if err != nil { + return nil, false + } + return ret, true +} + +// Purge clears the cache completely. +func (c *RedisCache) Purge() { + c.backend.FlushDB() + +} + +// Delete cache item by key +func (c *RedisCache) Delete(key string) { + c.backend.Del(key) +} + +// Stat returns cache statistics +func (c *RedisCache) Stat() CacheStat { + return CacheStat{ + Hits: c.Hits, + Misses: c.Misses, + Size: c.size(), + Keys: c.keys(), + Errors: c.Errors, + } +} + +func (c *RedisCache) size() int64 { + return 0 +} + +func (c *RedisCache) keys() int { + return int(c.backend.DBSize().Val()) +} + +func (c *RedisCache) allowed(key string, data Value) bool { + if c.backend.DBSize().Val() >= int64(c.maxKeys) { + return false + } + if c.maxKeySize > 0 && len(key) > c.maxKeySize { + return false + } + return true +} From f368c74d0d7bed7c66dbff62c2f3f4a4e067e311 Mon Sep 17 00:00:00 2001 From: Dmitry Tyzhnenko Date: Mon, 14 Oct 2019 00:37:15 +0300 Subject: [PATCH 2/6] Fix CI errors --- options.go | 13 ++++++++++++- redis_cache.go | 10 +++++++--- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/options.go b/options.go index 93cb129..35c1028 100644 --- a/options.go +++ b/options.go @@ -14,7 +14,7 @@ type options struct { maxCacheSize int64 ttl time.Duration onEvicted func(key string, value Value) - redis *redis.Options + redisOptions *redis.Options } // Option func type @@ -87,3 +87,14 @@ func OnEvicted(fn func(key string, value Value)) Option { return nil } } + +// RedisOptions functional option defines duration. +// Works for RedisCache only +func RedisOptions(Addr string, Password string, DB int) Option { + return func(o *options) error { + o.redisOptions.Addr = Addr + o.redisOptions.Password = Password + o.redisOptions.DB = DB + return nil + } +} diff --git a/redis_cache.go b/redis_cache.go index 063d1dc..bb3cceb 100644 --- a/redis_cache.go +++ b/redis_cache.go @@ -3,6 +3,7 @@ package lcw import ( "sync/atomic" "time" + "unsafe" "github.com/go-redis/redis" "github.com/pkg/errors" @@ -12,8 +13,7 @@ import ( type RedisCache struct { options CacheStat - backend *redis.Client - currentSize int64 + backend *redis.Client } // NewRedisCache makes Redis LoadingCache implementation. @@ -30,7 +30,7 @@ func NewRedisCache(opts ...Option) (*RedisCache, error) { } } - res.backend = redis.NewClient(res.redis) + res.backend = redis.NewClient(res.redisOptions) return &res, nil } @@ -115,5 +115,9 @@ func (c *RedisCache) allowed(key string, data Value) bool { if c.maxKeySize > 0 && len(key) > c.maxKeySize { return false } + if unsafe.Sizeof(data) > (512 * 1024 * 1024) { + return false + } + return true } From fb6f7da6222f366200182f2d65e3a7e17e44423c Mon Sep 17 00:00:00 2001 From: Dmitry Tyzhnenko Date: Mon, 14 Oct 2019 00:47:56 +0300 Subject: [PATCH 3/6] Fix CI --- redis_cache.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/redis_cache.go b/redis_cache.go index bb3cceb..8fc6069 100644 --- a/redis_cache.go +++ b/redis_cache.go @@ -3,7 +3,6 @@ package lcw import ( "sync/atomic" "time" - "unsafe" "github.com/go-redis/redis" "github.com/pkg/errors" @@ -115,9 +114,11 @@ func (c *RedisCache) allowed(key string, data Value) bool { if c.maxKeySize > 0 && len(key) > c.maxKeySize { return false } - if unsafe.Sizeof(data) > (512 * 1024 * 1024) { - return false + if s, ok := data.(Sizer); ok { + // Maximum allowed value size in Redis + if s.Size() >= (512 * 1024 * 1024) { + return false + } } - return true } From 8d3a27bce04b2c7f6a0f7290414ad7476f7036b3 Mon Sep 17 00:00:00 2001 From: Dmitry Tyzhnenko Date: Tue, 15 Oct 2019 00:53:11 +0300 Subject: [PATCH 4/6] Fix comments --- README.md | 20 +++--- cache_test.go | 86 +++++++++++++++++++++---- go.mod | 3 +- go.sum | 6 +- options.go | 14 ----- redis_cache.go | 36 +++++++---- redis_cache_test.go | 150 ++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 262 insertions(+), 53 deletions(-) create mode 100644 redis_cache_test.go diff --git a/README.md b/README.md index fa5b26f..5a573e0 100644 --- a/README.md +++ b/README.md @@ -3,26 +3,26 @@ The library adds a thin layer on top of [lru cache](https://github.com/hashicorp/golang-lru) and [patrickmn/go-cache](https://github.com/patrickmn/go-cache). -| Cache name | Constructor | Defaults | Description | -| -------------- | --------------------- | ----------------- | --------------------- | -| LruCache | lcw.NewLruCache | keys=1000 | LRU cache with limits | -| ExpirableCache | lcw.NewExpirableCache | keys=1000, ttl=5m | TTL cache with limits | -| Nop | lcw.NewNopCache | | Do-nothing cache | +| Cache name | Constructor | Defaults | Description | +| -------------- | --------------------- | ----------------- | ----------------------- | +| LruCache | lcw.NewLruCache | keys=1000 | LRU cache with limits | +| ExpirableCache | lcw.NewExpirableCache | keys=1000, ttl=5m | TTL cache with limits | +| RedisCache | lcw.NewRedisCache | ttl=5m | Redis cache with limits | +| Nop | lcw.NewNopCache | | Do-nothing cache | Main features: - - LoadingCache (guava style) - Limit maximum cache size (in bytes) - Limit maximum key size -- Limit maximum size of a value +- Limit maximum size of a value - Limit number of keys -- TTL support (`ExpirableCache` only) -- Callback on eviction event +- TTL support (`ExpirableCache` and `RedisCache`) +- Callback on eviction event (not supported in `RedisCache`) - Functional style invalidation - Functional options - Sane defaults - + ## Install and update `go get -u github.com/go-pkgz/lcw` diff --git a/cache_test.go b/cache_test.go index 6fb2695..e7636f8 100644 --- a/cache_test.go +++ b/cache_test.go @@ -10,6 +10,7 @@ import ( "testing" "time" + redis "github.com/go-redis/redis/v7" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -112,6 +113,9 @@ func TestCache_MaxValueSize(t *testing.T) { res, err = c.Get("key-Z", func() (Value, error) { return sizedString("result-Zzzz"), nil }) + if s, ok := res.(string); ok { + res = sizedString(s) + } assert.NoError(t, err) assert.Equal(t, sizedString("result-Z"), res.(sizedString), "got cached value") @@ -119,12 +123,18 @@ func TestCache_MaxValueSize(t *testing.T) { res, err = c.Get("key-Big", func() (Value, error) { return sizedString("1234567890"), nil }) + if s, ok := res.(string); ok { + res = sizedString(s) + } assert.NoError(t, err) assert.Equal(t, sizedString("1234567890"), res.(sizedString)) res, err = c.Get("key-Big", func() (Value, error) { return sizedString("result-big"), nil }) + if s, ok := res.(string); ok { + res = sizedString(s) + } assert.NoError(t, err) assert.Equal(t, sizedString("result-big"), res.(sizedString), "got not cached value") @@ -154,28 +164,39 @@ func TestCache_MaxCacheSize(t *testing.T) { return sizedString("result-Z"), nil }) assert.NoError(t, err) + if s, ok := res.(string); ok { + res = sizedString(s) + } assert.Equal(t, sizedString("result-Z"), res.(sizedString)) - res, err = c.Get("key-Z", func() (Value, error) { return sizedString("result-Zzzz"), nil }) + if s, ok := res.(string); ok { + res = sizedString(s) + } assert.NoError(t, err) assert.Equal(t, sizedString("result-Z"), res.(sizedString), "got cached value") - assert.Equal(t, int64(8), c.size()) - + if _, ok := c.(*RedisCache); !ok { + assert.Equal(t, int64(8), c.size()) + } _, err = c.Get("key-Z2", func() (Value, error) { return sizedString("result-Y"), nil }) assert.Nil(t, err) - assert.Equal(t, int64(16), c.size()) + if _, ok := c.(*RedisCache); !ok { + assert.Equal(t, int64(16), c.size()) + } // this will cause removal _, err = c.Get("key-Z3", func() (Value, error) { return sizedString("result-Z"), nil }) assert.Nil(t, err) - assert.Equal(t, int64(16), c.size()) - assert.Equal(t, 2, c.keys()) + if _, ok := c.(*RedisCache); !ok { + assert.Equal(t, int64(16), c.size()) + // Due RedisCache does not support MaxCacheSize this assert should be skipped + assert.Equal(t, 2, c.keys()) + } }) } } @@ -371,11 +392,14 @@ func TestCache_Delete(t *testing.T) { require.Nil(t, err) } assert.Equal(t, 1000, c.Stat().Keys) - assert.Equal(t, int64(9890), c.Stat().Size) - + if _, ok := c.(*RedisCache); !ok { + assert.Equal(t, int64(9890), c.Stat().Size) + } c.Delete("key-2") assert.Equal(t, 999, c.Stat().Keys) - assert.Equal(t, int64(9890-8), c.Stat().Size) + if _, ok := c.(*RedisCache); !ok { + assert.Equal(t, int64(9890-8), c.Stat().Size) + } }) } } @@ -392,8 +416,12 @@ func TestCache_DeleteWithEvent(t *testing.T) { caches := cachesTestList(t, OnEvicted(onEvict)) for _, c := range caches { + evKey, evVal, evCount = "", "", 0 t.Run(strings.Replace(fmt.Sprintf("%T", c), "*lcw.", "", 1), func(t *testing.T) { + if _, ok := c.(*RedisCache); ok { + t.Skip("RedisCache doesn't support delete events") + } // fill cache for i := 0; i < 1000; i++ { _, err := c.Get(fmt.Sprintf("key-%d", i), func() (Value, error) { @@ -425,25 +453,45 @@ func TestCache_Stats(t *testing.T) { require.Nil(t, err) } stats := c.Stat() - assert.Equal(t, CacheStat{Hits: 0, Misses: 100, Keys: 100, Size: 890}, stats) + switch c.(type) { + case *RedisCache: + assert.Equal(t, CacheStat{Hits: 0, Misses: 100, Keys: 100, Size: 0}, stats) + default: + assert.Equal(t, CacheStat{Hits: 0, Misses: 100, Keys: 100, Size: 890}, stats) + } _, err := c.Get("key-1", func() (Value, error) { return "xyz", nil }) require.NoError(t, err) - assert.Equal(t, CacheStat{Hits: 1, Misses: 100, Keys: 100, Size: 890}, c.Stat()) + switch c.(type) { + case *RedisCache: + assert.Equal(t, CacheStat{Hits: 1, Misses: 100, Keys: 100, Size: 0}, c.Stat()) + default: + assert.Equal(t, CacheStat{Hits: 1, Misses: 100, Keys: 100, Size: 890}, c.Stat()) + } _, err = c.Get("key-1123", func() (Value, error) { return sizedString("xyz"), nil }) require.NoError(t, err) - assert.Equal(t, CacheStat{Hits: 1, Misses: 101, Keys: 101, Size: 893}, c.Stat()) + switch c.(type) { + case *RedisCache: + assert.Equal(t, CacheStat{Hits: 1, Misses: 101, Keys: 101, Size: 0}, c.Stat()) + default: + assert.Equal(t, CacheStat{Hits: 1, Misses: 101, Keys: 101, Size: 893}, c.Stat()) + } _, err = c.Get("key-9999", func() (Value, error) { return nil, errors.New("err") }) require.NotNil(t, err) - assert.Equal(t, CacheStat{Hits: 1, Misses: 101, Keys: 101, Size: 893, Errors: 1}, c.Stat()) + switch c.(type) { + case *RedisCache: + assert.Equal(t, CacheStat{Hits: 1, Misses: 101, Keys: 101, Size: 0, Errors: 1}, c.Stat()) + default: + assert.Equal(t, CacheStat{Hits: 1, Misses: 101, Keys: 101, Size: 893, Errors: 1}, c.Stat()) + } }) } @@ -515,9 +563,21 @@ func cachesTestList(t *testing.T, opts ...Option) []countedCache { lc, err := NewLruCache(opts...) require.NoError(t, err, "can't make lru cache") caches = append(caches, lc) + + server := newTestRedisServer() + client := redis.NewClient(&redis.Options{ + Addr: server.Addr()}) + rc, err := NewRedisCache(client, opts...) + require.NoError(t, err, "can't make redis cache") + caches = append(caches, rc) + return caches } type sizedString string func (s sizedString) Size() int { return len(s) } + +func (s sizedString) MarshalBinary() (data []byte, err error) { + return []byte(s), nil +} diff --git a/go.mod b/go.mod index 4872ee2..2035511 100644 --- a/go.mod +++ b/go.mod @@ -1,8 +1,9 @@ module github.com/go-pkgz/lcw require ( + github.com/alicebob/miniredis v2.5.0+incompatible github.com/davecgh/go-spew v1.1.1 // indirect - github.com/go-redis/redis v6.15.6+incompatible + github.com/go-redis/redis/v7 v7.0.0-beta.4 github.com/hashicorp/golang-lru v0.5.0 github.com/patrickmn/go-cache v2.1.0+incompatible github.com/pkg/errors v0.8.1 diff --git a/go.sum b/go.sum index 40d1d23..a5a0b78 100644 --- a/go.sum +++ b/go.sum @@ -1,9 +1,11 @@ +github.com/alicebob/miniredis v2.5.0+incompatible h1:yBHoLpsyjupjz3NL3MhKMVkR41j82Yjf3KFv7ApYzUI= +github.com/alicebob/miniredis v2.5.0+incompatible/go.mod h1:8HZjEj4yU0dwhYHky+DxYx+6BMjkBbe5ONFIF1MXffk= github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/go-redis/redis v6.15.6+incompatible h1:H9evprGPLI8+ci7fxQx6WNZHJSb7be8FqJQRhdQZ5Sg= -github.com/go-redis/redis v6.15.6+incompatible/go.mod h1:NAIEuMOZ/fxfXJIrKDQDz8wamY7mA7PouImQ2Jvg6kA= +github.com/go-redis/redis/v7 v7.0.0-beta.4 h1:p6z7Pde69EGRWvlC++y8aFcaWegyrKHzOBGo0zUACTQ= +github.com/go-redis/redis/v7 v7.0.0-beta.4/go.mod h1:xhhSbUMTsleRPur+Vgx9sUHtyN33bdjxY+9/0n9Ig8s= github.com/hashicorp/golang-lru v0.5.0 h1:CL2msUPvZTLb5O648aiLNJw3hnBxN2+1Jq8rCOH9wdo= github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8= github.com/patrickmn/go-cache v2.1.0+incompatible h1:HRMgzkcYKYpi3C8ajMPV8OFXaaRUnok+kx1WdO15EQc= diff --git a/options.go b/options.go index 35c1028..4ee964a 100644 --- a/options.go +++ b/options.go @@ -3,8 +3,6 @@ package lcw import ( "errors" "time" - - "github.com/go-redis/redis" ) type options struct { @@ -14,7 +12,6 @@ type options struct { maxCacheSize int64 ttl time.Duration onEvicted func(key string, value Value) - redisOptions *redis.Options } // Option func type @@ -87,14 +84,3 @@ func OnEvicted(fn func(key string, value Value)) Option { return nil } } - -// RedisOptions functional option defines duration. -// Works for RedisCache only -func RedisOptions(Addr string, Password string, DB int) Option { - return func(o *options) error { - o.redisOptions.Addr = Addr - o.redisOptions.Password = Password - o.redisOptions.DB = DB - return nil - } -} diff --git a/redis_cache.go b/redis_cache.go index 8fc6069..393c1db 100644 --- a/redis_cache.go +++ b/redis_cache.go @@ -4,10 +4,13 @@ import ( "sync/atomic" "time" - "github.com/go-redis/redis" + redis "github.com/go-redis/redis/v7" "github.com/pkg/errors" ) +// RedisSizeLimit is maximum allowed value size in Redis +const RedisSizeLimit = 512 * 1024 * 1024 + // RedisCache implements LoadingCache for Redis. type RedisCache struct { options @@ -16,7 +19,7 @@ type RedisCache struct { } // NewRedisCache makes Redis LoadingCache implementation. -func NewRedisCache(opts ...Option) (*RedisCache, error) { +func NewRedisCache(backend *redis.Client, opts ...Option) (*RedisCache, error) { res := RedisCache{ options: options{ @@ -29,7 +32,7 @@ func NewRedisCache(opts ...Option) (*RedisCache, error) { } } - res.backend = redis.NewClient(res.redisOptions) + res.backend = backend return &res, nil } @@ -37,24 +40,29 @@ func NewRedisCache(opts ...Option) (*RedisCache, error) { // Get gets value by key or load with fn if not found in cache func (c *RedisCache) Get(key string, fn func() (Value, error)) (data Value, err error) { - v, ok := c.backend.Get(key).Result() - if ok == nil { + v, getErr := c.backend.Get(key).Result() + switch getErr { + case nil: atomic.AddInt64(&c.Hits, 1) return v, nil - } - if ok == redis.Nil { + case redis.Nil: if data, err = fn(); err != nil { atomic.AddInt64(&c.Errors, 1) return data, err } - } else if ok != nil { + default: atomic.AddInt64(&c.Errors, 1) - return v, ok + return v, getErr } atomic.AddInt64(&c.Misses, 1) if c.allowed(key, data) { - c.backend.Set(key, data, c.ttl) + _, setErr := c.backend.Set(key, data, c.ttl).Result() + if setErr != nil { + atomic.AddInt64(&c.Errors, 1) + return data, setErr + } + } return data, nil } @@ -108,15 +116,17 @@ func (c *RedisCache) keys() int { } func (c *RedisCache) allowed(key string, data Value) bool { - if c.backend.DBSize().Val() >= int64(c.maxKeys) { + if c.maxKeys > 0 && c.backend.DBSize().Val() >= int64(c.maxKeys) { return false } if c.maxKeySize > 0 && len(key) > c.maxKeySize { return false } if s, ok := data.(Sizer); ok { - // Maximum allowed value size in Redis - if s.Size() >= (512 * 1024 * 1024) { + if c.maxValueSize > 0 && (s.Size() >= c.maxValueSize || s.Size() >= RedisSizeLimit) { + return false + } + if c.maxValueSize <= 0 && s.Size() >= RedisSizeLimit { return false } } diff --git a/redis_cache_test.go b/redis_cache_test.go new file mode 100644 index 0000000..0da8fd4 --- /dev/null +++ b/redis_cache_test.go @@ -0,0 +1,150 @@ +package lcw + +import ( + "fmt" + "log" + "sync/atomic" + "testing" + "time" + + "github.com/alicebob/miniredis" + redis "github.com/go-redis/redis/v7" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newTestRedis returns a redis.Cmdable. +func newTestRedisServer() *miniredis.Miniredis { + mr, err := miniredis.Run() + if err != nil { + panic(err) + } + + return mr +} + +func TestExpirableRedisCache(t *testing.T) { + server := newTestRedisServer() + client := redis.NewClient(&redis.Options{ + Addr: server.Addr()}) + lc, err := NewRedisCache(client, MaxKeys(5), TTL(time.Second*6)) + if err != nil { + log.Fatalf("can't make redis cache, %v", err) + } + require.NoError(t, err) + for i := 0; i < 5; i++ { + _, e := lc.Get(fmt.Sprintf("key-%d", i), func() (Value, error) { + return fmt.Sprintf("result-%d", i), nil + }) + assert.NoError(t, e) + server.FastForward(1000 * time.Millisecond) + } + + assert.Equal(t, 5, lc.Stat().Keys) + assert.Equal(t, int64(5), lc.Stat().Misses) + + _, e := lc.Get("key-xx", func() (Value, error) { + return "result-xx", nil + }) + assert.NoError(t, e) + assert.Equal(t, 5, lc.Stat().Keys) + assert.Equal(t, int64(6), lc.Stat().Misses) + + server.FastForward(1000 * time.Millisecond) + assert.Equal(t, 4, lc.Stat().Keys) + + server.FastForward(4000 * time.Millisecond) + assert.Equal(t, 0, lc.keys()) + +} + +// func Test1(t *testing.T) { +// server := newTestRedisServer() +// client := redis.NewClient(&redis.Options{ +// Addr: server.Addr()}) +// lc, err := NewRedisCache(client, MaxKeys(5), MaxValSize(10), MaxKeySize(10)) +// if err != nil { +// log.Fatalf("can't make redis cache, %v", err) +// } + +// } + +func TestRedisCache(t *testing.T) { + var coldCalls int32 + + server := newTestRedisServer() + client := redis.NewClient(&redis.Options{ + Addr: server.Addr()}) + lc, err := NewRedisCache(client, MaxKeys(5), MaxValSize(10), MaxKeySize(10)) + if err != nil { + log.Fatalf("can't make redis cache, %v", err) + } + // put 5 keys to cache + for i := 0; i < 5; i++ { + res, e := lc.Get(fmt.Sprintf("key-%d", i), func() (Value, error) { + atomic.AddInt32(&coldCalls, 1) + return fmt.Sprintf("result-%d", i), nil + }) + assert.Nil(t, e) + assert.Equal(t, fmt.Sprintf("result-%d", i), res.(string)) + assert.Equal(t, int32(i+1), atomic.LoadInt32(&coldCalls)) + } + + // check if really cached + res, err := lc.Get("key-3", func() (Value, error) { + return "result-blah", nil + }) + assert.Nil(t, err) + assert.Equal(t, "result-3", res.(string), "should be cached") + + // try to cache after maxKeys reached + res, err = lc.Get("key-X", func() (Value, error) { + return "result-X", nil + }) + assert.Nil(t, err) + assert.Equal(t, "result-X", res.(string)) + assert.Equal(t, int64(5), lc.backend.DBSize().Val()) + + // put to cache and make sure it cached + res, err = lc.Get("key-Z", func() (Value, error) { + return "result-Z", nil + }) + assert.Nil(t, err) + assert.Equal(t, "result-Z", res.(string)) + + res, err = lc.Get("key-Z", func() (Value, error) { + return "result-Zzzz", nil + }) + assert.Nil(t, err) + assert.Equal(t, "result-Zzzz", res.(string), "got non-cached value") + assert.Equal(t, 5, lc.keys()) + + res, err = lc.Get("key-Zzzzzzz", func() (Value, error) { + return "result-Zzzz", nil + }) + assert.Nil(t, err) + assert.Equal(t, "result-Zzzz", res.(string), "got non-cached value") + assert.Equal(t, 5, lc.keys()) + +} + +func TestRedisCache_BadOptions(t *testing.T) { + server := newTestRedisServer() + client := redis.NewClient(&redis.Options{ + Addr: server.Addr()}) + + _, err := NewRedisCache(client, MaxCacheSize(-1)) + assert.EqualError(t, err, "failed to set cache option: negative max cache size") + + _, err = NewRedisCache(client, MaxCacheSize(-1)) + assert.EqualError(t, err, "failed to set cache option: negative max cache size") + + _, err = NewRedisCache(client, MaxKeys(-1)) + assert.EqualError(t, err, "failed to set cache option: negative max keys") + + _, err = NewRedisCache(client, MaxValSize(-1)) + assert.EqualError(t, err, "failed to set cache option: negative max value size") + + _, err = NewRedisCache(client, TTL(-1)) + assert.EqualError(t, err, "failed to set cache option: negative ttl") +} From e886488750c91714f7f3e53005efe650bf2c6ba5 Mon Sep 17 00:00:00 2001 From: Dmitry Tyzhnenko Date: Thu, 17 Oct 2019 20:41:31 +0300 Subject: [PATCH 5/6] Fix comments, increase test coverage --- redis_cache.go | 16 +++++++++----- redis_cache_test.go | 54 ++++++++++++++++++++++++++++++++++++--------- 2 files changed, 53 insertions(+), 17 deletions(-) diff --git a/redis_cache.go b/redis_cache.go index 393c1db..1e3d53c 100644 --- a/redis_cache.go +++ b/redis_cache.go @@ -8,8 +8,8 @@ import ( "github.com/pkg/errors" ) -// RedisSizeLimit is maximum allowed value size in Redis -const RedisSizeLimit = 512 * 1024 * 1024 +// RedisValueSizeLimit is maximum allowed value size in Redis +const RedisValueSizeLimit = 512 * 1024 * 1024 // RedisCache implements LoadingCache for Redis. type RedisCache struct { @@ -32,6 +32,10 @@ func NewRedisCache(backend *redis.Client, opts ...Option) (*RedisCache, error) { } } + if res.maxValueSize <= 0 { + res.maxValueSize = RedisValueSizeLimit + } + res.backend = backend return &res, nil @@ -42,14 +46,17 @@ func (c *RedisCache) Get(key string, fn func() (Value, error)) (data Value, err v, getErr := c.backend.Get(key).Result() switch getErr { + // RedisClient returns nil when find a key in DB case nil: atomic.AddInt64(&c.Hits, 1) return v, nil + // RedisClient returns redis.Nil when doesn't find a key in DB case redis.Nil: if data, err = fn(); err != nil { atomic.AddInt64(&c.Errors, 1) return data, err } + // RedisClient returns !nil when somthing goes wrong while get data default: atomic.AddInt64(&c.Errors, 1) return v, getErr @@ -123,10 +130,7 @@ func (c *RedisCache) allowed(key string, data Value) bool { return false } if s, ok := data.(Sizer); ok { - if c.maxValueSize > 0 && (s.Size() >= c.maxValueSize || s.Size() >= RedisSizeLimit) { - return false - } - if c.maxValueSize <= 0 && s.Size() >= RedisSizeLimit { + if c.maxValueSize > 0 && (s.Size() >= c.maxValueSize) { return false } } diff --git a/redis_cache_test.go b/redis_cache_test.go index 0da8fd4..a65c3cb 100644 --- a/redis_cache_test.go +++ b/redis_cache_test.go @@ -1,6 +1,7 @@ package lcw import ( + "errors" "fmt" "log" "sync/atomic" @@ -23,6 +24,8 @@ func newTestRedisServer() *miniredis.Miniredis { return mr } +type fakeString string + func TestExpirableRedisCache(t *testing.T) { server := newTestRedisServer() client := redis.NewClient(&redis.Options{ @@ -58,17 +61,6 @@ func TestExpirableRedisCache(t *testing.T) { } -// func Test1(t *testing.T) { -// server := newTestRedisServer() -// client := redis.NewClient(&redis.Options{ -// Addr: server.Addr()}) -// lc, err := NewRedisCache(client, MaxKeys(5), MaxValSize(10), MaxKeySize(10)) -// if err != nil { -// log.Fatalf("can't make redis cache, %v", err) -// } - -// } - func TestRedisCache(t *testing.T) { var coldCalls int32 @@ -126,6 +118,42 @@ func TestRedisCache(t *testing.T) { assert.Equal(t, "result-Zzzz", res.(string), "got non-cached value") assert.Equal(t, 5, lc.keys()) + res, ok := lc.Peek("error-key-Z2") + assert.False(t, ok) + assert.Nil(t, res) +} + +func TestRedisCacheErrors(t *testing.T) { + + server := newTestRedisServer() + client := redis.NewClient(&redis.Options{ + Addr: server.Addr()}) + lc, err := NewRedisCache(client) + if err != nil { + log.Fatalf("can't make redis cache, %v", err) + } + + res, err := lc.Get("error-key-Z", func() (Value, error) { + return "error-result-Z", errors.New("some error") + }) + assert.NotNil(t, err) + assert.Equal(t, "error-result-Z", res.(string)) + assert.Equal(t, int64(1), lc.Stat().Errors) + + res, err = lc.Get("error-key-Z2", func() (Value, error) { + return fakeString("error-result-Z2"), nil + }) + assert.NotNil(t, err) + assert.Equal(t, fakeString("error-result-Z2"), res.(fakeString)) + assert.Equal(t, int64(2), lc.Stat().Errors) + + server.Close() + res, err = lc.Get("error-key-Z3", func() (Value, error) { + return fakeString("error-result-Z3"), nil + }) + assert.NotNil(t, err) + assert.Equal(t, "", res.(string)) + assert.Equal(t, int64(3), lc.Stat().Errors) } func TestRedisCache_BadOptions(t *testing.T) { @@ -147,4 +175,8 @@ func TestRedisCache_BadOptions(t *testing.T) { _, err = NewRedisCache(client, TTL(-1)) assert.EqualError(t, err, "failed to set cache option: negative ttl") + + _, err = NewRedisCache(client, MaxKeySize(-1)) + assert.EqualError(t, err, "failed to set cache option: negative max key size") + } From 2041909f014b2495df72769ddc6e0bf58c114fe6 Mon Sep 17 00:00:00 2001 From: Dmitry Tyzhnenko Date: Fri, 18 Oct 2019 20:47:53 +0300 Subject: [PATCH 6/6] Add additional condition in RedisCache constructor --- redis_cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/redis_cache.go b/redis_cache.go index 1e3d53c..ec928ea 100644 --- a/redis_cache.go +++ b/redis_cache.go @@ -32,7 +32,7 @@ func NewRedisCache(backend *redis.Client, opts ...Option) (*RedisCache, error) { } } - if res.maxValueSize <= 0 { + if res.maxValueSize <= 0 || res.maxValueSize > RedisValueSizeLimit { res.maxValueSize = RedisValueSizeLimit }