addressing comments

Signed-off-by: Sudharshann D <sudhar287@gmail.com>

CHANGELOG.md

@@ -24,6 +24,7 @@ We use _breaking :warning:_ to mark changes that are not backward compatible (re
 
 ### Added
 
+- [#3579](https://github.com/thanos-io/thanos/pull/3579) Cache: Added inmemory cache for caching bucket.
 - [#3469](https://github.com/thanos-io/thanos/pull/3469) StoreAPI: Added `hints` field to `LabelNamesRequest` and `LabelValuesRequest`. Hints in an opaque data structure that can be used to carry additional information from the store and its content is implementation specific.
 - [#3421](https://github.com/thanos-io/thanos/pull/3421) Tools: Added `thanos tools bucket rewrite` command allowing to delete series from given block.
 - [#3509](https://github.com/thanos-io/thanos/pull/3509) Store: Added touch series limit

go.mod

@@ -15,7 +15,6 @@ require (
 	github.com/cortexproject/cortex v1.6.1-0.20210108144208-6c2dab103f20
 	github.com/davecgh/go-spew v1.1.1
 	github.com/facette/natsort v0.0.0-20181210072756-2cd4dd1e2dcb
-	github.com/fatih/color v1.10.0 // indirect
 	github.com/fatih/structtag v1.1.0
 	github.com/felixge/fgprof v0.9.1
 	github.com/fsnotify/fsnotify v1.4.9
@@ -69,7 +68,6 @@ require (
 	gopkg.in/fsnotify.v1 v1.4.7
 	gopkg.in/yaml.v2 v2.3.0
 	gopkg.in/yaml.v3 v3.0.0-20200615113413-eeeca48fe776
-	honnef.co/go/tools v0.0.1-2020.1.6 // indirect
 )
 
 replace (

go.sum

@@ -309,8 +309,6 @@ github.com/facette/natsort v0.0.0-20181210072756-2cd4dd1e2dcb/go.mod h1:bH6Xx7IW
 github.com/fatih/color v1.7.0/go.mod h1:Zm6kSWBoL9eyXnKyktHP6abPY2pDugNf5KwzbycvMj4=
 github.com/fatih/color v1.9.0 h1:8xPHl4/q1VyqGIPif1F+1V3Y3lSmrq01EabUW3CoW5s=
 github.com/fatih/color v1.9.0/go.mod h1:eQcE1qtQxscV5RaZvpXrrb8Drkc3/DdQ+uUYCNjL+zU=
-github.com/fatih/color v1.10.0 h1:s36xzo75JdqLaaWoiEHk767eHiwo0598uUxyfiPkDsg=
-github.com/fatih/color v1.10.0/go.mod h1:ELkj/draVOlAH/xkhN6mQ50Qd0MPOk5AAr3maGEBuJM=
 github.com/fatih/structtag v1.1.0 h1:6j4mUV/ES2duvnAzKMFkN6/A5mCaNYPD3xfbAkLLOF8=
 github.com/fatih/structtag v1.1.0/go.mod h1:mBJUNpUnHmRKrKlQQlmCrh5PuhftFbNv8Ys4/aAZl94=
 github.com/felixge/fgprof v0.9.1 h1:E6FUJ2Mlv043ipLOCFqo8+cHo9MhQ203E2cdEK/isEs=
@@ -809,8 +807,6 @@ github.com/mattn/go-colorable v0.0.9/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaO
 github.com/mattn/go-colorable v0.1.4/go.mod h1:U0ppj6V5qS13XJ6of8GYAs25YV2eR4EVcfRqFIhoBtE=
 github.com/mattn/go-colorable v0.1.6 h1:6Su7aK7lXmJ/U79bYtBjLNaha4Fs1Rg9plHpcH+vvnE=
 github.com/mattn/go-colorable v0.1.6/go.mod h1:u6P/XSegPjTcexA+o6vUJrdnUu04hMope9wVRipJSqc=
-github.com/mattn/go-colorable v0.1.8 h1:c1ghPdyEDarC70ftn0y+A/Ee++9zz8ljHG1b13eJ0s8=
-github.com/mattn/go-colorable v0.1.8/go.mod h1:u6P/XSegPjTcexA+o6vUJrdnUu04hMope9wVRipJSqc=
 github.com/mattn/go-ieproxy v0.0.0-20190610004146-91bb50d98149/go.mod h1:31jz6HNzdxOmlERGGEc4v/dMssOfmp2p5bT/okiKFFc=
 github.com/mattn/go-ieproxy v0.0.0-20190702010315-6dee0af9227d/go.mod h1:31jz6HNzdxOmlERGGEc4v/dMssOfmp2p5bT/okiKFFc=
 github.com/mattn/go-ieproxy v0.0.0-20191113090002-7c0f6868bffe h1:YioO2TiJyAHWHyCRQCP8jk5IzTqmsbGc5qQPIhHo6xs=
@@ -1538,7 +1534,6 @@ golang.org/x/tools v0.0.0-20200227222343-706bc42d1f0d/go.mod h1:TB2adYChydJhpapK
 golang.org/x/tools v0.0.0-20200304193943-95d2e580d8eb/go.mod h1:o4KQGtdN14AW+yjsvvwRTJJuXz8XRtIHtEnmAXLyFUw=
 golang.org/x/tools v0.0.0-20200312045724-11d5b4c81c7d/go.mod h1:o4KQGtdN14AW+yjsvvwRTJJuXz8XRtIHtEnmAXLyFUw=
 golang.org/x/tools v0.0.0-20200331025713-a30bf2db82d4/go.mod h1:Sl4aGygMT6LrqrWclx+PTx3U+LnKx/seiNR+3G19Ar8=
-golang.org/x/tools v0.0.0-20200410194907-79a7a3126eef/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE=
 golang.org/x/tools v0.0.0-20200501065659-ab2804fb9c9d/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE=
 golang.org/x/tools v0.0.0-20200512131952-2bc93b1c0c88/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE=
 golang.org/x/tools v0.0.0-20200513201620-d5fe73897c97/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE=
@@ -1703,8 +1698,6 @@ honnef.co/go/tools v0.0.1-2020.1.3 h1:sXmLre5bzIR6ypkjXCDI3jHPssRhc8KD/Ome589sc3
 honnef.co/go/tools v0.0.1-2020.1.3/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k=
 honnef.co/go/tools v0.0.1-2020.1.4 h1:UoveltGrhghAA7ePc+e+QYDHXrBps2PqFZiHkGR/xK8=
 honnef.co/go/tools v0.0.1-2020.1.4/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k=
-honnef.co/go/tools v0.0.1-2020.1.6 h1:W18jzjh8mfPez+AwGLxmOImucz/IFjpNlrKVnaj2YVc=
-honnef.co/go/tools v0.0.1-2020.1.6/go.mod h1:pyyisuGw24ruLjrr1ddx39WE0y9OooInRzEYLhQB2YY=
 howett.net/plist v0.0.0-20181124034731-591f970eefbb h1:jhnBjNi9UFpfpl8YZhA9CrOqpnJdvzuiHsl/dnxl11M=
 howett.net/plist v0.0.0-20181124034731-591f970eefbb/go.mod h1:vMygbs4qMhSZSc4lCUl2OEE+rDiIIJAIdR4m7MiMcm0=
 k8s.io/api v0.0.0-20191115095533-47f6de673b26/go.mod h1:iA/8arsvelvo4IDqIhX4IbjTEKBGgvsf2OraTuRtLFU=

pkg/cache/inmemory.go

@@ -7,7 +7,6 @@ import (
 	"context"
 	"sync"
 	"time"
-	"unsafe"
 
 	"github.com/go-kit/kit/log"
 	"github.com/go-kit/kit/log/level"
@@ -28,10 +27,6 @@ var (
 
 const (
 	maxInt = int(^uint(0) >> 1)
-	// Size of a golang slice i.e unsafe.Sizeof([]byte{}) on amd64.
-	sliceHeaderSize = 16
-	// As the cache key is of type String its size is 8 bytes.
-	keyHeaderSize = 8
 )
 
 // InMemoryCacheConfig holds the in-memory cache config.
@@ -43,19 +38,19 @@ type InMemoryCacheConfig struct {
 }
 
 type InMemoryCache struct {
-	mtx sync.Mutex
-
 	logger           log.Logger
-	lru              *lru.LRU
 	maxSizeBytes     uint64
 	maxItemSizeBytes uint64
 
-	curSize uint64
-
-	evicted          prometheus.Counter
-	requests         prometheus.Counter
-	hits             prometheus.Counter
-	hitsExpired      prometheus.Counter
+	mtx         sync.Mutex
+	curSize     uint64
+	lru         *lru.LRU
+	evicted     prometheus.Counter
+	requests    prometheus.Counter
+	hits        prometheus.Counter
+	hitsExpired prometheus.Counter
+	// The input cache value would be copied to an inmemory array
+	// instead of simply using the one sent by the caller.
 	added            prometheus.Counter
 	current          prometheus.Gauge
 	currentSize      prometheus.Gauge
@@ -64,7 +59,12 @@ type InMemoryCache struct {
 }
 
 type cacheDataWithTTLWrapper struct {
-	data       []byte
+	data []byte
+	// The objects that are over the TTL are not destroyed eagerly.
+	// When there is a hit for an item that is over the TTL, the object is removed from the cache
+	// and null is returned.
+	// There is ongoing effort to integrate TTL within the Hashicorp golang cache itself.
+	// This https://github.com/hashicorp/golang-lru/pull/41 can be used here once complete.
 	expiryTime time.Time
 }
 
@@ -78,7 +78,7 @@ func parseInMemoryCacheConfig(conf []byte) (InMemoryCacheConfig, error) {
 	return config, nil
 }
 
-// NewInMemoryCache creates a new thread-safe LRU cache for chunks and metadata and ensures the total cache
+// NewInMemoryCache creates a new thread-safe LRU cache and ensures the total cache
 // size approximately does not exceed maxBytes.
 func NewInMemoryCache(name string, logger log.Logger, reg prometheus.Registerer, conf []byte) (*InMemoryCache, error) {
 	config, err := parseInMemoryCacheConfig(conf)
@@ -89,7 +89,7 @@ func NewInMemoryCache(name string, logger log.Logger, reg prometheus.Registerer,
 	return NewInMemoryCacheWithConfig(name, logger, reg, config)
 }
 
-// NewInMemoryCacheWithConfig creates a new thread-safe LRU cache chunks and metadata ensures the total cache
+// NewInMemoryCacheWithConfig creates a new thread-safe LRU cache and ensures the total cache
 // size approximately does not exceed maxBytes.
 func NewInMemoryCacheWithConfig(name string, logger log.Logger, reg prometheus.Registerer, config InMemoryCacheConfig) (*InMemoryCache, error) {
 	if config.MaxItemSize > config.MaxSize {
@@ -103,66 +103,70 @@ func NewInMemoryCacheWithConfig(name string, logger log.Logger, reg prometheus.R
 	}
 
 	c.evicted = promauto.With(reg).NewCounter(prometheus.CounterOpts{
-		Name:        "thanos_store_cache_items_evicted_total",
+		Name:        "thanos_cache_inmemory_items_evicted_total",
 		Help:        "Total number of items that were evicted from the inmemory cache.",
 		ConstLabels: prometheus.Labels{"name": name},
 	})
 
 	c.added = promauto.With(reg).NewCounter(prometheus.CounterOpts{
-		Name:        "thanos_store_cache_items_added_total",
+		Name:        "thanos_cache_inmemory_items_added_total",
 		Help:        "Total number of items that were added to the inmemory cache.",
 		ConstLabels: prometheus.Labels{"name": name},
 	})
 
 	c.requests = promauto.With(reg).NewCounter(prometheus.CounterOpts{
-		Name:        "thanos_store_cache_requests_total",
+		Name:        "thanos_cache_inmemory_requests_total",
 		Help:        "Total number of requests to the inmemory cache.",
 		ConstLabels: prometheus.Labels{"name": name},
 	})
 
 	c.hitsExpired = promauto.With(reg).NewCounter(prometheus.CounterOpts{
-		Name:        "thanos_store_cache_hits_on_expired_data_total",
+		Name:        "thanos_cache_inmemory_hits_on_expired_data_total",
 		Help:        "Total number of requests to the inmemory cache that were a hit but needed to be evicted due to TTL.",
 		ConstLabels: prometheus.Labels{"name": name},
 	})
 
 	c.overflow = promauto.With(reg).NewCounter(prometheus.CounterOpts{
-		Name:        "thanos_store_cache_items_overflowed_total",
+		Name:        "thanos_cache_inmemory_items_overflowed_total",
 		Help:        "Total number of items that could not be added to the inmemory cache due to being too big.",
 		ConstLabels: prometheus.Labels{"name": name},
 	})
 
 	c.hits = promauto.With(reg).NewCounter(prometheus.CounterOpts{
-		Name:        "thanos_store_cache_hits_total",
+		Name:        "thanos_cache_inmemory_hits_total",
 		Help:        "Total number of requests to the inmemory cache that were a hit.",
 		ConstLabels: prometheus.Labels{"name": name},
 	})
 
 	c.current = promauto.With(reg).NewGauge(prometheus.GaugeOpts{
-		Name:        "thanos_store_cache_items",
+		Name:        "thanos_cache_inmemory_items",
 		Help:        "Current number of items in the inmemory cache.",
 		ConstLabels: prometheus.Labels{"name": name},
 	})
 
 	c.currentSize = promauto.With(reg).NewGauge(prometheus.GaugeOpts{
-		Name: "thanos_store_cache_items_size_bytes",
-		Help: "Current byte size of items in the inmemory cache.",
+		Name:        "thanos_cache_inmemory_items_size_bytes",
+		Help:        "Current byte size of items in the inmemory cache.",
+		ConstLabels: prometheus.Labels{"name": name},
 	})
 
 	c.totalCurrentSize = promauto.With(reg).NewGauge(prometheus.GaugeOpts{
-		Name: "thanos_store_cache_total_size_bytes",
-		Help: "Current byte size of items (both value and key) in the inmemory cache.",
+		Name:        "thanos_cache_inmemory_total_size_bytes",
+		Help:        "Current byte size of items (both value and key) in the inmemory cache.",
+		ConstLabels: prometheus.Labels{"name": name},
 	})
 
 	_ = promauto.With(reg).NewGaugeFunc(prometheus.GaugeOpts{
-		Name: "thanos_store_cache_max_size_bytes",
-		Help: "Maximum number of bytes to be held in the inmemory cache.",
+		Name:        "thanos_cache_inmemory_max_size_bytes",
+		Help:        "Maximum number of bytes to be held in the inmemory cache.",
+		ConstLabels: prometheus.Labels{"name": name},
 	}, func() float64 {
 		return float64(c.maxSizeBytes)
 	})
 	_ = promauto.With(reg).NewGaugeFunc(prometheus.GaugeOpts{
-		Name: "thanos_store_cache_max_item_size_bytes",
-		Help: "Maximum number of bytes for single entry to be held in the inmemory cache.",
+		Name:        "thanos_cache_inmemory_max_item_size_bytes",
+		Help:        "Maximum number of bytes for single entry to be held in the inmemory cache.",
+		ConstLabels: prometheus.Labels{"name": name},
 	}, func() float64 {
 		return float64(c.maxItemSizeBytes)
 	})
@@ -185,8 +189,8 @@ func NewInMemoryCacheWithConfig(name string, logger log.Logger, reg prometheus.R
 }
 
 func (c *InMemoryCache) onEvict(key, val interface{}) {
-	keySize := keyHeaderSize + uint64(len(key.(string)))
-	entrySize := sliceHeaderSize + uint64(len(val.(cacheDataWithTTLWrapper).data)) + uint64(unsafe.Sizeof(val.(cacheDataWithTTLWrapper).expiryTime))
+	keySize := uint64(len(key.(string)))
+	entrySize := uint64(len(val.(cacheDataWithTTLWrapper).data))
 
 	c.evicted.Inc()
 	c.current.Dec()
@@ -217,9 +221,8 @@ func (c *InMemoryCache) get(key string) ([]byte, bool) {
 }
 
 func (c *InMemoryCache) set(key string, val []byte, ttl time.Duration) {
-	var size = sliceHeaderSize + uint64(len(val))
-	keySize := keyHeaderSize + uint64(len(key))
-	entrySize := sliceHeaderSize + uint64(len(val)) + uint64(unsafe.Sizeof(time.Now().Add(ttl)))
+	var size = uint64(len(val))
+	keySize := uint64(len(key))
 
 	c.mtx.Lock()
 	defer c.mtx.Unlock()
@@ -241,7 +244,7 @@ func (c *InMemoryCache) set(key string, val []byte, ttl time.Duration) {
 
 	c.added.Inc()
 	c.currentSize.Add(float64(size))
-	c.totalCurrentSize.Add(float64(keySize + entrySize))
+	c.totalCurrentSize.Add(float64(keySize + size))
 	c.current.Inc()
 	c.curSize += size
 }

pkg/cache/inmemory_test.go

@@ -102,3 +102,14 @@ max_item_size: 2KB
 		})
 	}
 }
+
+func TestNewInMemoryCache(t *testing.T) {
+	// Should return error when the max size of the cache is smaller than the max size of
+	conf := []byte(`
+max_size: 2KB
+max_item_size: 1MB
+`)
+	cache, err := NewInMemoryCache("test", log.NewNopLogger(), nil, conf)
+	testutil.NotOk(t, err)
+	testutil.Equals(t, (*InMemoryCache)(nil), cache)
+}

pkg/cache/memcached_test.go

@@ -15,7 +15,7 @@ import (
 	"github.com/thanos-io/thanos/pkg/testutil"
 )
 
-func TestMemcachedIndexCache(t *testing.T) {
+func TestMemcachedCache(t *testing.T) {
 	t.Parallel()
 
 	// Init some data to conveniently define test cases later one.

pkg/store/cache/inmemory_test.go

@@ -44,6 +44,20 @@ max_item_size: 2KB
 	testutil.Ok(t, err)
 	testutil.Equals(t, uint64(1024*1024), cache.maxSizeBytes)
 	testutil.Equals(t, uint64(2*1024), cache.maxItemSizeBytes)
+
+	// Should instance an in-memory index cache with specified YAML config.s with units.
+	conf = []byte(`
+max_size: 2KB
+max_item_size: 1MB
+`)
+	cache, err = NewInMemoryIndexCache(log.NewNopLogger(), nil, conf)
+	testutil.NotOk(t, err)
+	testutil.Equals(t, (*InMemoryIndexCache)(nil), cache)
+	// testutil.Equals(t, uint64(1024*1024), cache.maxSizeBytes)
+	// testutil.Equals(t, uint64(2*1024), cache.maxItemSizeBytes)
+
+	// testutil.Equals(t, uint64(1024*1024), cache.maxItemSizeBytes)
+	// testutil.Equals(t, uint64(2*1024), cache.maxSizeBytes)
 }
 
 func TestInMemoryIndexCache_AvoidsDeadlock(t *testing.T) {