From 621d87d91310dc745990a40447bc709dcc3cb1d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Giedrius=20Statkevi=C4=8Dius?= Date: Wed, 30 Jan 2019 13:33:01 +0200 Subject: [PATCH 1/5] store/cache: add a case for when item > max LRU size This could potentially spin forever if the size of that item is bigger than the max size so add this special case to handle it instead of spinning forever. In this case, the oldest one will not be removed in this function but later an item might be evicted when we will add an item to the LRU with c.lru.Add() so it will be OK either way -- just that we will not spin here forever. --- pkg/store/cache.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/store/cache.go b/pkg/store/cache.go index 16cad121f5..c0d914818f 100644 --- a/pkg/store/cache.go +++ b/pkg/store/cache.go @@ -116,6 +116,9 @@ func newIndexCache(reg prometheus.Registerer, maxBytes uint64) (*indexCache, err } func (c *indexCache) ensureFits(b []byte) { + if uint64(len(b)) > c.maxSize { + return + } for c.curSize+uint64(len(b)) > c.maxSize { c.lru.RemoveOldest() } From 40e710867b173e29c8f43faad5eb5c3327e14159 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Giedrius=20Statkevi=C4=8Dius?= Date: Fri, 1 Feb 2019 14:23:45 +0200 Subject: [PATCH 2/5] store/cache: return true if items fit in the cache * ensureFits() now returns true if the passed items will fit in the cache * Wire up the results to a new counter metric thanos_store_index_cache_items_overflowed_total * Add cache tests to check the edge cases --- pkg/store/cache.go | 25 +++++++++++++++++++++---- pkg/store/cache_test.go | 21 +++++++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) create mode 100644 pkg/store/cache_test.go diff --git a/pkg/store/cache.go b/pkg/store/cache.go index c0d914818f..3c538754ed 100644 --- a/pkg/store/cache.go +++ b/pkg/store/cache.go @@ -43,6 +43,7 @@ type indexCache struct { added *prometheus.CounterVec current *prometheus.GaugeVec currentSize *prometheus.GaugeVec + overflow *prometheus.CounterVec } // newIndexCache creates a new LRU cache for index entries and ensures the total cache @@ -66,6 +67,11 @@ func newIndexCache(reg prometheus.Registerer, maxBytes uint64) (*indexCache, err Help: "Total number of requests to the cache.", }, []string{"item_type"}) + c.overflow = prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "thanos_store_index_cache_items_overflowed_total", + Help: "Total number of items that could not be added to the cache due to being too big.", + }, []string{"item_type"}) + c.hits = prometheus.NewCounterVec(prometheus.CounterOpts{ Name: "thanos_store_index_cache_hits_total", Help: "Total number of requests to the cache that were a hit.", @@ -115,13 +121,16 @@ func newIndexCache(reg prometheus.Registerer, maxBytes uint64) (*indexCache, err return c, nil } -func (c *indexCache) ensureFits(b []byte) { +// ensureFits tries to make sure that the passed slice will fit into the LRU cache. +// Returns true if it will fit. +func (c *indexCache) ensureFits(b []byte) bool { if uint64(len(b)) > c.maxSize { - return + return false } for c.curSize+uint64(len(b)) > c.maxSize { c.lru.RemoveOldest() } + return true } func (c *indexCache) setPostings(b ulid.ULID, l labels.Label, v []byte) { @@ -130,7 +139,11 @@ func (c *indexCache) setPostings(b ulid.ULID, l labels.Label, v []byte) { c.mtx.Lock() defer c.mtx.Unlock() - c.ensureFits(v) + fits := c.ensureFits(v) + if !fits { + c.overflow.WithLabelValues(cacheTypePostings).Add(1) + return + } // The caller may be passing in a sub-slice of a huge array. Copy the data // to ensure we don't waste huge amounts of space for something small. @@ -161,7 +174,11 @@ func (c *indexCache) setSeries(b ulid.ULID, id uint64, v []byte) { c.mtx.Lock() defer c.mtx.Unlock() - c.ensureFits(v) + fits := c.ensureFits(v) + if !fits { + c.overflow.WithLabelValues(cacheTypeSeries).Add(1) + return + } // The caller may be passing in a sub-slice of a huge array. Copy the data // to ensure we don't waste huge amounts of space for something small. diff --git a/pkg/store/cache_test.go b/pkg/store/cache_test.go new file mode 100644 index 0000000000..e100a91306 --- /dev/null +++ b/pkg/store/cache_test.go @@ -0,0 +1,21 @@ +// Tests out the edge cases of the index cache. +package store + +import ( + "testing" + + "github.com/improbable-eng/thanos/pkg/testutil" + "github.com/prometheus/client_golang/prometheus" +) + +func test_index_edge(t *testing.T) { + metrics := prometheus.NewRegistry() + cache, err := newIndexCache(metrics, 1) + testutil.Ok(t, err) + + fits := cache.ensureFits([]byte{42, 24}) + testutil.Equals(t, fits, false) + + fits = cache.ensureFits([]byte{42}) + testutil.Equals(t, fits, true) +} From 92dd5f73306522ba2b2a29a4586833c1d90e0cbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Giedrius=20Statkevi=C4=8Dius?= Date: Fri, 1 Feb 2019 14:26:56 +0200 Subject: [PATCH 3/5] store/cache_test: move around names and comments --- pkg/store/cache_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/store/cache_test.go b/pkg/store/cache_test.go index e100a91306..2846513a0f 100644 --- a/pkg/store/cache_test.go +++ b/pkg/store/cache_test.go @@ -1,4 +1,4 @@ -// Tests out the edge cases of the index cache. +// Tests out the index cache implementation. package store import ( @@ -8,7 +8,8 @@ import ( "github.com/prometheus/client_golang/prometheus" ) -func test_index_edge(t *testing.T) { +// TestIndexCacheEdge tests the index cache edge cases. +func TestIndexCacheEdge(t *testing.T) { metrics := prometheus.NewRegistry() cache, err := newIndexCache(metrics, 1) testutil.Ok(t, err) From fe8db57745a88747e1f7d997fdc8ea424655b827 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Giedrius=20Statkevi=C4=8Dius?= Date: Fri, 1 Feb 2019 15:42:56 +0200 Subject: [PATCH 4/5] store/cache: inline ensureFits() call No need to create an extra variable to hold a boolean. --- pkg/store/cache.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/store/cache.go b/pkg/store/cache.go index 3c538754ed..d3c243b99d 100644 --- a/pkg/store/cache.go +++ b/pkg/store/cache.go @@ -139,8 +139,7 @@ func (c *indexCache) setPostings(b ulid.ULID, l labels.Label, v []byte) { c.mtx.Lock() defer c.mtx.Unlock() - fits := c.ensureFits(v) - if !fits { + if !c.ensureFits(v) { c.overflow.WithLabelValues(cacheTypePostings).Add(1) return } @@ -174,8 +173,7 @@ func (c *indexCache) setSeries(b ulid.ULID, id uint64, v []byte) { c.mtx.Lock() defer c.mtx.Unlock() - fits := c.ensureFits(v) - if !fits { + if !c.ensureFits(v) { c.overflow.WithLabelValues(cacheTypeSeries).Add(1) return } From 045969daddee414474d4f77da8c971044d295a14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Giedrius=20Statkevi=C4=8Dius?= Date: Fri, 1 Feb 2019 15:46:19 +0200 Subject: [PATCH 5/5] store/cache: use Inc() instead of Add(1) Inc() is specifically designed for this case so use it instead of Add(1). --- pkg/store/cache.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/store/cache.go b/pkg/store/cache.go index d3c243b99d..5acd2c104c 100644 --- a/pkg/store/cache.go +++ b/pkg/store/cache.go @@ -140,7 +140,7 @@ func (c *indexCache) setPostings(b ulid.ULID, l labels.Label, v []byte) { defer c.mtx.Unlock() if !c.ensureFits(v) { - c.overflow.WithLabelValues(cacheTypePostings).Add(1) + c.overflow.WithLabelValues(cacheTypePostings).Inc() return } @@ -174,7 +174,7 @@ func (c *indexCache) setSeries(b ulid.ULID, id uint64, v []byte) { defer c.mtx.Unlock() if !c.ensureFits(v) { - c.overflow.WithLabelValues(cacheTypeSeries).Add(1) + c.overflow.WithLabelValues(cacheTypeSeries).Inc() return }