From 8d54a99d128539343f6f1a8359299281f6917be1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Giedrius=20Statkevi=C4=8Dius?= Date: Wed, 27 Feb 2019 15:03:18 +0200 Subject: [PATCH 1/5] store/cache: do not forget to increase c.current on adding new items --- pkg/store/cache.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/store/cache.go b/pkg/store/cache.go index 5acd2c104c..f0343026ea 100644 --- a/pkg/store/cache.go +++ b/pkg/store/cache.go @@ -151,6 +151,7 @@ func (c *indexCache) setPostings(b ulid.ULID, l labels.Label, v []byte) { c.lru.Add(cacheItem{b, cacheKeyPostings(l)}, cv) c.currentSize.WithLabelValues(cacheTypePostings).Add(float64(len(v))) + c.current.WithLabelValues(cacheTypePostings).Inc() } func (c *indexCache) postings(b ulid.ULID, l labels.Label) ([]byte, bool) { @@ -185,6 +186,7 @@ func (c *indexCache) setSeries(b ulid.ULID, id uint64, v []byte) { c.lru.Add(cacheItem{b, cacheKeySeries(id)}, cv) c.currentSize.WithLabelValues(cacheTypeSeries).Add(float64(len(v))) + c.current.WithLabelValues(cacheTypeSeries).Inc() } func (c *indexCache) series(b ulid.ULID, id uint64) ([]byte, bool) { From fbaae7c4b7dce8c5d78b026acee9b576c597ec15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Giedrius=20Statkevi=C4=8Dius?= Date: Thu, 28 Feb 2019 13:42:41 +0200 Subject: [PATCH 2/5] store/cache: properly adjust c.curSize --- pkg/store/cache.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/store/cache.go b/pkg/store/cache.go index f0343026ea..acdedd1d8a 100644 --- a/pkg/store/cache.go +++ b/pkg/store/cache.go @@ -128,7 +128,10 @@ func (c *indexCache) ensureFits(b []byte) bool { return false } for c.curSize+uint64(len(b)) > c.maxSize { - c.lru.RemoveOldest() + if _, val, ok := c.lru.RemoveOldest(); ok { + v := val.([]byte) + c.curSize -= uint64(len(v)) + } } return true } @@ -152,6 +155,7 @@ func (c *indexCache) setPostings(b ulid.ULID, l labels.Label, v []byte) { c.currentSize.WithLabelValues(cacheTypePostings).Add(float64(len(v))) c.current.WithLabelValues(cacheTypePostings).Inc() + c.curSize += uint64(len(v)) } func (c *indexCache) postings(b ulid.ULID, l labels.Label) ([]byte, bool) { @@ -187,6 +191,7 @@ func (c *indexCache) setSeries(b ulid.ULID, id uint64, v []byte) { c.currentSize.WithLabelValues(cacheTypeSeries).Add(float64(len(v))) c.current.WithLabelValues(cacheTypeSeries).Inc() + c.curSize += uint64(len(v)) } func (c *indexCache) series(b ulid.ULID, id uint64) ([]byte, bool) { From 9a6477143d675534a40078cd4b73cf1db8b267aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Giedrius=20Statkevi=C4=8Dius?= Date: Thu, 28 Feb 2019 13:48:28 +0200 Subject: [PATCH 3/5] store/cache: prevent uint64 overflow by switching operands Adding uint64(len(b)) to c.curSize might potentially overflow uint64 if the numbers are big enough and then we might not remove enough items from the LRU to satisfy the request. On the other hand, switching the operands avoids this problem because we check before if uint64(len(b)) is bigger than c.maxSize so subtracting uint64(len(b)) will *never* overflow because we know that it is less or equal to c.maxSize. --- pkg/store/cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/store/cache.go b/pkg/store/cache.go index acdedd1d8a..812c019eec 100644 --- a/pkg/store/cache.go +++ b/pkg/store/cache.go @@ -127,7 +127,7 @@ func (c *indexCache) ensureFits(b []byte) bool { if uint64(len(b)) > c.maxSize { return false } - for c.curSize+uint64(len(b)) > c.maxSize { + for c.curSize > c.maxSize-uint64(len(b)) { if _, val, ok := c.lru.RemoveOldest(); ok { v := val.([]byte) c.curSize -= uint64(len(v)) From 22ab76a2cd07e2735f7617ebd0ea7fe95b6d7b27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Giedrius=20Statkevi=C4=8Dius?= Date: Fri, 1 Mar 2019 21:53:08 +0200 Subject: [PATCH 4/5] store/cache: revert ensureFits() changes c.curSize is lowered in onEvict. --- pkg/store/cache.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/pkg/store/cache.go b/pkg/store/cache.go index 812c019eec..58e720790b 100644 --- a/pkg/store/cache.go +++ b/pkg/store/cache.go @@ -128,10 +128,7 @@ func (c *indexCache) ensureFits(b []byte) bool { return false } for c.curSize > c.maxSize-uint64(len(b)) { - if _, val, ok := c.lru.RemoveOldest(); ok { - v := val.([]byte) - c.curSize -= uint64(len(v)) - } + c.lru.RemoveOldest() } return true } From 9679a193f433353287ea3052320dbc9e46bc3e9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Giedrius=20Statkevi=C4=8Dius?= Date: Sun, 3 Mar 2019 12:31:53 +0200 Subject: [PATCH 5/5] store/cache: add smoke tests Add smoke tests for the index cache which check if we set curSize properly, and if removal works. --- pkg/store/cache_test.go | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/pkg/store/cache_test.go b/pkg/store/cache_test.go index 2846513a0f..0c3d241668 100644 --- a/pkg/store/cache_test.go +++ b/pkg/store/cache_test.go @@ -3,9 +3,13 @@ package store import ( "testing" + "time" + "github.com/fortytw2/leaktest" "github.com/improbable-eng/thanos/pkg/testutil" + "github.com/oklog/ulid" "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/tsdb/labels" ) // TestIndexCacheEdge tests the index cache edge cases. @@ -20,3 +24,38 @@ func TestIndexCacheEdge(t *testing.T) { fits = cache.ensureFits([]byte{42}) testutil.Equals(t, fits, true) } + +// TestIndexCacheSmoke runs the smoke tests for the index cache. +func TestIndexCacheSmoke(t *testing.T) { + defer leaktest.CheckTimeout(t, 10*time.Second)() + + metrics := prometheus.NewRegistry() + cache, err := newIndexCache(metrics, 20) + testutil.Ok(t, err) + + blid := ulid.ULID([16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}) + labels := labels.Label{Name: "test", Value: "123"} + + cache.setPostings(blid, labels, []byte{42}) + + p, ok := cache.postings(blid, labels) + testutil.Equals(t, ok, true) + testutil.Equals(t, p, []byte{42}) + testutil.Equals(t, cache.curSize, uint64(1)) + + cache.setSeries(blid, 1234, []byte{42, 42}) + + s, ok := cache.series(blid, 1234) + testutil.Equals(t, ok, true) + testutil.Equals(t, s, []byte{42, 42}) + testutil.Equals(t, cache.curSize, uint64(3)) + + cache.lru.RemoveOldest() + testutil.Equals(t, cache.curSize, uint64(2)) + + cache.lru.RemoveOldest() + testutil.Equals(t, cache.curSize, uint64(0)) + + cache.lru.RemoveOldest() + testutil.Equals(t, cache.curSize, uint64(0)) +}