Add lru cache for query conversion #1398

benraskin92 · 2019-02-21T18:09:49Z

W/O cache

W/ cache

codecov · 2019-02-21T18:11:53Z

Codecov Report

Merging #1398 into master will decrease coverage by 31.5%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #1398      +/-   ##
=========================================
- Coverage    70.7%   39.1%   -31.6%     
=========================================
  Files         827       5     -822     
  Lines       71238     342   -70896     
=========================================
- Hits        50414     134   -50280     
+ Misses      17519     194   -17325     
+ Partials     3305      14    -3291

Flag	Coverage Δ
#aggregator	`?`
#cluster	`?`
#collector	`39.1% <ø> (-24.6%)`	⬇️
#dbnode	`?`
#m3em	`?`
#m3ninx	`?`
#m3nsch	`?`
#metrics	`?`
#msg	`?`
#query	`?`
#x	`?`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 667f864...c007e30. Read the comment docs.

codecov · 2019-02-21T18:12:04Z

Codecov Report

Merging #1398 into master will decrease coverage by 7.6%.
The diff coverage is 60.6%.

@@           Coverage Diff            @@
##           master   #1398     +/-   ##
========================================
- Coverage    70.8%   63.1%   -7.7%     
========================================
  Files         832     831      -1     
  Lines       71370   71248    -122     
========================================
- Hits        50542   45001   -5541     
- Misses      17526   23103   +5577     
+ Partials     3302    3144    -158

Flag	Coverage Δ
#aggregator	`69.2% <ø> (-13.2%)`	⬇️
#cluster	`67.7% <ø> (-18.2%)`	⬇️
#collector	`47.9% <ø> (-15.8%)`	⬇️
#dbnode	`79.6% <ø> (-1.2%)`	⬇️
#m3em	`66.7% <ø> (-6.5%)`	⬇️
#m3ninx	`70.9% <ø> (-3.4%)`	⬇️
#m3nsch	`28.4% <ø> (-22.8%)`	⬇️
#metrics	`17.6% <ø> (ø)`	⬆️
#msg	`74.9% <ø> (ø)`	⬆️
#query	`46.7% <60.6%> (-18.9%)`	⬇️
#x	`68.1% <ø> (-8%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0647a12...5963fda. Read the comment docs.

benraskin92 · 2019-02-21T18:18:00Z

src/cmd/services/m3query/config/config.go

@@ -53,6 +53,8 @@ const (
 	errNoIDGenerationScheme            = "error: a recent breaking change means that an ID " +
 		"generation scheme is required in coordinator configuration settings. " +
 		"More information is available here: %s"
+
+	defaultQueryConversionCacheSize = 100


Probably can be a lot higher?

Yeah, make it at least 4096

benraskin92 · 2019-02-21T18:18:42Z

src/cmd/services/m3query/config/config.go

@@ -120,6 +122,9 @@ type Configuration struct {

 	// LookbackDuration determines the lookback duration for queries
 	LookbackDuration *time.Duration `yaml:"lookbackDuration"`
+
+	// Cache configurations.
+	Cache CacheConfigurations `yaml:"cache"`


Added this instead of directly adding query conversion cache in case we have more cache options to add in the future

benraskin92 · 2019-02-21T18:23:32Z

scripts/development/m3_stack/m3coordinator.yml

@@ -55,3 +55,7 @@ carbon:



Either add this to all configs or remove since it defaults

I'd err towards default

andrewmains12 · 2019-02-21T18:40:24Z

src/query/storage/query_conversion_lru.go

+// AddWithLock adds a value to the cache. Returns true if an eviction occurred.
+func (c *QueryConversionLRU) AddWithLock(key string, value idx.Query) (evicted bool) {
+	// Check for existing item
+	c.Lock()


I'm a fan of the pattern the hashicorp implementation uses for this--implement all the datastructure operations without a lock, and then wrap that class in a thin class with a lock. Saves you on defer overhead while still maintaining lock safety fairly easily. Thoughts?

andrewmains12 · 2019-02-21T18:41:07Z

src/query/storage/query_conversion_lru.go

+}
+
+// GetWithLock looks up a key's value from the cache.
+func (c *QueryConversionLRU) GetWithLock(key string) (value idx.Query, ok bool) {


If key is always a string, why is items map[interface]?

andrewmains12 · 2019-02-21T18:42:36Z

src/query/storage/query_conversion_lru.go

+
+// QueryConversionLRU implements a fixed size LRU cache
+type QueryConversionLRU struct {
+	sync.Mutex


Nit: embedding is weird here imo (sort of implies that the LRU is a lock); I would just use an explicit field.

andrewmains12 · 2019-02-21T18:43:17Z

src/query/storage/query_conversion_lru.go

+	c := &QueryConversionLRU{
+		size:      size,
+		evictList: list.New(),
+		items:     make(map[interface{}]*list.Element),


Size is known here, so let's use it in make

andrewmains12 · 2019-02-21T18:44:40Z

src/query/storage/query_conversion_lru.go

+}
+
+// AddWithLock adds a value to the cache. Returns true if an eviction occurred.
+func (c *QueryConversionLRU) AddWithLock(key string, value idx.Query) (evicted bool) {


Nit: I don't think the WithLock suffixes are really necessary; we can comment the class as thread safe or not instead.

arnikola · 2019-02-22T19:27:37Z

src/cmd/services/m3query/config/config.go

@@ -53,6 +53,8 @@ const (
 	errNoIDGenerationScheme            = "error: a recent breaking change means that an ID " +
 		"generation scheme is required in coordinator configuration settings. " +
 		"More information is available here: %s"
+
+	defaultQueryConversionCacheSize = 100


Yeah, make it at least 4096

arnikola · 2019-02-22T19:28:40Z

src/cmd/services/m3query/config/config.go

@@ -146,6 +151,37 @@ type FilterConfiguration struct {
 	CompleteTags Filter `yaml:"completeTags"`
 }

+// CacheConfigurations is the cache configurations.
+type CacheConfigurations struct {


nit: CacheConfiguration

I think it's better with the s as it implies it can be used for multiple cache configs.

All other configs are XyzConfiguration even with multiple configs, should probably stick to the precedent here

src/cmd/services/m3query/config/config.go

src/query/storage/index.go

arnikola · 2019-02-22T19:32:37Z

src/query/storage/index.go


 	"github.com/m3db/m3/src/dbnode/storage/index"
 	"github.com/m3db/m3/src/m3ninx/idx"
 	"github.com/m3db/m3/src/query/models"
 	"github.com/m3db/m3x/ident"
 )

+// QueryConvserionCache represents the query conversion LRU cache


QueryConvserionCache -> QueryConversionCache

arnikola · 2019-02-22T20:12:21Z

src/query/storage/query_conversion_lru_test.go

+	lru.Add("c", idx.NewTermQuery([]byte("bar"), []byte("foo")))
+	lru.Add("d", idx.NewTermQuery([]byte("baz"), []byte("biz")))
+	lru.Add("e", idx.NewTermQuery([]byte("qux"), []byte("quz")))
+	lru.Add("f", idx.NewTermQuery([]byte("quz"), []byte("qux")))


Can you check that the return types for the others are false and the return type for this one is true?

arnikola · 2019-02-22T20:14:16Z

src/query/storage/m3/storage.go

 	opts := m3db.NewOptions().
 		SetTagOptions(tagOptions).
 		SetLookbackDuration(lookbackDuration).
 		SetConsolidationFunc(consolidators.TakeLast)

+	conversionLRU, err := storage.NewQueryConversionLRU(conversionCacheSize)


I think it would make a bit more sense to create the cache above and then pass the cache into the storage constructor?

arnikola · 2019-02-22T20:16:26Z

src/query/storage/m3/storage.go

 	return &m3storage{
 		clusters:        clusters,
 		readWorkerPool:  readWorkerPool,
 		writeWorkerPool: writeWorkerPool,
 		opts:            opts,
 		nowFn:           time.Now,
-	}
+		conversionCache: &storage.QueryConvserionCache{LRU: conversionLRU},


Can you make a constructor for this, taking in the LRU as an argument?

arnikola · 2019-02-22T20:17:04Z

src/query/storage/index.go

 	// Optimization for single matcher case.
 	if len(matchers) == 1 {
 		q, err := matcherToQuery(matchers[0])
 		if err != nil {
 			return index.Query{}, err
 		}

+		cache.LRU.Add(string(k), q)


nit: rather than calling .LRU.Add(), can you put a thin wrapper Add function onto the query conversion cache?

Any specific reason to do this?

Reads better, more clarity; also if at some point we want to add more logic to the add function we can keep it in one place

arnikola · 2019-02-22T20:18:29Z

src/query/storage/index.go


 	"github.com/m3db/m3/src/dbnode/storage/index"
 	"github.com/m3db/m3/src/m3ninx/idx"
 	"github.com/m3db/m3/src/query/models"
 	"github.com/m3db/m3x/ident"
 )

+// QueryConvserionCache represents the query conversion LRU cache
+type QueryConvserionCache struct {
+	mu sync.RWMutex


Since you're never using the Read lock, might be better to just make this a simple sync.Mutex

benraskin92 · 2019-02-25T17:01:06Z

src/query/storage/m3/storage.go

 	return &m3storage{
 		clusters:        clusters,
 		readWorkerPool:  readWorkerPool,
 		writeWorkerPool: writeWorkerPool,
 		opts:            opts,
 		nowFn:           time.Now,
-	}
+		// conversionCache: &storage.QueryConversionCache{LRU: conversionLRU},


benraskin92 · 2019-02-25T17:01:13Z

src/query/storage/m3/storage.go

 	opts := m3db.NewOptions().
 		SetTagOptions(tagOptions).
 		SetLookbackDuration(lookbackDuration).
 		SetConsolidationFunc(consolidators.TakeLast)

+	// conversionLRU, err := storage.NewQueryConversionLRU(conversionCacheSize)


arnikola · 2019-02-25T17:04:19Z

scripts/development/m3_stack/m3coordinator.yml

+
+cache:
+  queryConversion:
+    size: 200


nit: this is very low

arnikola · 2019-02-25T17:04:25Z

src/cmd/services/m3dbnode/config/cache.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2017 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.


nit: revert

arnikola

Couple of nits, and Validate() method on the configs

arnikola · 2019-02-25T21:35:04Z

src/query/storage/index.go

+type QueryConversionCache struct {
+	mu sync.Mutex
+
+	LRU *QueryConversionLRU


nit: can make this private now

arnikola · 2019-02-25T21:35:43Z

src/query/storage/index_test.go

+}
+
+func TestQueryKey(t *testing.T) {
+	matchers := models.Matchers{


Can you add a few matchers here, maybe with different Types

arnikola · 2019-02-25T21:45:25Z

src/query/storage/query_conversion_lru_test.go

+	require.False(t, ok)
+
+	// make sure "b" is still in the cache
+	_, ok = lru.Get([]byte("b"))


nit: might as well check value here

arnikola · 2019-02-25T22:19:19Z

src/cmd/services/m3query/config/config.go

+
+// Validate validates the QueryConversionCacheConfiguration settings.
+func (q *QueryConversionCacheConfiguration) Validate() error {
+	if *q.Size <= 0 {


Is 0 valid?

src/query/server/server.go

src/query/storage/index_test.go

src/query/storage/query_conversion_lru_test.go

arnikola

Approved with a couple of nits

arnikola · 2019-02-26T14:51:46Z

src/cmd/services/m3query/config/config.go

+
+// Validate validates the QueryConversionCacheConfiguration settings.
+func (q *QueryConversionCacheConfiguration) Validate() error {
+	switch {


Would be cleaner as

if q.Size != nil && q.Size <= 0 { // return error...

src/query/server/server.go

arnikola · 2019-02-26T14:57:54Z

src/query/storage/query_conversion_lru_test.go

+	// rewrite "e" and make sure nothing gets evicted
+	// since "e" is already in the cache.
+	evicted = lru.Set([]byte("e"), idx.NewTermQuery([]byte("qux"), []byte("quz")))
+	require.False(t, evicted)


nit: check that the value of e changes?

arnikola · 2019-02-26T14:58:28Z

src/cmd/services/m3query/config/config_test.go

@@ -162,3 +162,13 @@ func TestTagOptionsConfig(t *testing.T) {
 	assert.Equal(t, []byte("foo"), opts.BucketName())
 	assert.Equal(t, models.TypePrependMeta, opts.IDSchemeType())
 }
+
+func TestNegativeQueryConversionSize(t *testing.T) {


Add test to validate nil case maybe? Up to you

benraskin92 commented Feb 21, 2019

View reviewed changes

andrewmains12 reviewed Feb 21, 2019

View reviewed changes

benraskin92 force-pushed the braskin/lru_cache_query branch from 5456b6e to 9199bf5 Compare February 22, 2019 18:23

benraskin92 changed the title ~~[WIP][DON'T REVIEW] Add lru cache for query conversion~~ Add lru cache for query conversion Feb 22, 2019

arnikola reviewed Feb 22, 2019

View reviewed changes

benraskin92 commented Feb 25, 2019

View reviewed changes

arnikola reviewed Feb 25, 2019

View reviewed changes

benraskin92 force-pushed the braskin/lru_cache_query branch 2 times, most recently from 5963fda to 642cbd2 Compare February 25, 2019 19:50

arnikola reviewed Feb 25, 2019

View reviewed changes

Benjamin Raskin added 15 commits February 26, 2019 09:27

Add lru cache for query conversion

838288f

Spelling

af5fc9c

Add locks to add

cecd94e

Small fix

69981c7

Address comments

cde1040

Fix test

74541b6

Write tests

74b1a9d

Remove prints

ece218f

Address comments

ac0f410

Address more comments

d8ebfb9

Fix test

8631ef7

Address more comments

0b5ab74

Fix test

d1d8aa7

Address comments

ed654fc

Fix test

b88193f

More tests

9d932b9

benraskin92 force-pushed the braskin/lru_cache_query branch from 2949dbc to 9d932b9 Compare February 26, 2019 14:28

arnikola approved these changes Feb 26, 2019

View reviewed changes

Benjamin Raskin added 2 commits February 26, 2019 10:21

Last couple nits

5d0e46c

Add one test

94e7c12

benraskin92 merged commit 8277c7a into master Feb 26, 2019

arnikola mentioned this pull request Feb 26, 2019

Basic cache implementation for query conversion #1386

Closed

arnikola deleted the braskin/lru_cache_query branch February 26, 2019 15:54

arnikola mentioned this pull request Mar 12, 2019

Complete changelog for 0.7.0 #1443

Merged

		@@ -1,4 +1,4 @@
		// Copyright (c) 2017 Uber Technologies, Inc.
		// Copyright (c) 2019 Uber Technologies, Inc.

Add lru cache for query conversion #1398

Add lru cache for query conversion #1398

Conversation

benraskin92 commented Feb 21, 2019

codecov bot commented Feb 21, 2019

Codecov Report

codecov bot commented Feb 21, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 21, 2019 •

edited

Loading