diff options
author | Jaime Martinez <jmartinez@gitlab.com> | 2020-03-03 17:50:04 +0300 |
---|---|---|
committer | Alessio Caiazza <acaiazza@gitlab.com> | 2020-03-03 17:50:04 +0300 |
commit | 536bdde15bc09aab0bd1a15a5ac9f4b5133716bd (patch) | |
tree | 3c6691e7f821b6dc11f95c094d742cfafb92fd4f /internal/source/gitlab/cache | |
parent | 3ae8b1c077bef83c480cba2695e3943ba152e00c (diff) |
Add default cache configuration and pass down to memstore and Entry.
Wrap global variables into one default struct.
Update test steps so that tests run every time so we don't get
the cached results.
Diffstat (limited to 'internal/source/gitlab/cache')
-rw-r--r-- | internal/source/gitlab/cache/cache.go | 34 | ||||
-rw-r--r-- | internal/source/gitlab/cache/cache_test.go | 33 | ||||
-rw-r--r-- | internal/source/gitlab/cache/const.go | 11 | ||||
-rw-r--r-- | internal/source/gitlab/cache/entry.go | 45 | ||||
-rw-r--r-- | internal/source/gitlab/cache/entry_test.go | 78 | ||||
-rw-r--r-- | internal/source/gitlab/cache/memstore.go | 19 | ||||
-rw-r--r-- | internal/source/gitlab/cache/retriever.go | 21 |
7 files changed, 145 insertions, 96 deletions
diff --git a/internal/source/gitlab/cache/cache.go b/internal/source/gitlab/cache/cache.go index 9a4ab753..bb3567b4 100644 --- a/internal/source/gitlab/cache/cache.go +++ b/internal/source/gitlab/cache/cache.go @@ -2,22 +2,44 @@ package cache import ( "context" + "time" "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api" "gitlab.com/gitlab-org/gitlab-pages/metrics" ) +var defaultCacheConfig = cacheConfig{ + cacheExpiry: 10 * time.Minute, + entryRefreshTimeout: 30 * time.Second, + retrievalTimeout: 5 * time.Second, + maxRetrievalInterval: time.Second, + maxRetrievalRetries: 3, +} + // Cache is a short and long caching mechanism for GitLab source type Cache struct { - client api.Client - store Store + client api.Client + store Store + cacheConfig *cacheConfig +} + +type cacheConfig struct { + cacheExpiry time.Duration + entryRefreshTimeout time.Duration + retrievalTimeout time.Duration + maxRetrievalInterval time.Duration + maxRetrievalRetries int } // NewCache creates a new instance of Cache. -func NewCache(client api.Client) *Cache { +func NewCache(client api.Client, cc *cacheConfig) *Cache { + if cc == nil { + cc = &defaultCacheConfig + } + return &Cache{ client: client, - store: newMemStore(), + store: newMemStore(client, cc), } } @@ -25,8 +47,8 @@ func NewCache(client api.Client) *Cache { // algorithm works as follows: // - We first check if the cache entry exists, and if it is up-to-date. If it // is fresh we return the lookup entry from cache and it is a cache hit. -// - If entry is not up-to-date, what means that it has been created in a cache -// more than `shortCacheExpiry` duration ago, we schedule an asynchronous +// - If entry is not up-to-date, that means it has been created in a cache +// more than `entryRefreshTimeout` duration ago, we schedule an asynchronous // retrieval of the latest configuration we are going to obtain through the // API, and we immediately return an old value, to avoid blocking clients. In // this case it is also a cache hit. diff --git a/internal/source/gitlab/cache/cache_test.go b/internal/source/gitlab/cache/cache_test.go index a0ee507d..c3e03dec 100644 --- a/internal/source/gitlab/cache/cache_test.go +++ b/internal/source/gitlab/cache/cache_test.go @@ -69,7 +69,7 @@ func (c *client) GetLookup(ctx context.Context, _ string) api.Lookup { return lookup } -func withTestCache(config resolverConfig, block func(*Cache, *client)) { +func withTestCache(config resolverConfig, cacheConfig *cacheConfig, block func(*Cache, *client)) { var chanSize int if config.buffered { @@ -84,7 +84,7 @@ func withTestCache(config resolverConfig, block func(*Cache, *client)) { failure: config.failure, } - cache := NewCache(resolver) + cache := NewCache(resolver, cacheConfig) block(cache, resolver) } @@ -122,7 +122,7 @@ type entryConfig struct { func TestResolve(t *testing.T) { t.Run("when item is not cached", func(t *testing.T) { - withTestCache(resolverConfig{buffered: true}, func(cache *Cache, resolver *client) { + withTestCache(resolverConfig{buffered: true}, nil, func(cache *Cache, resolver *client) { resolver.domain <- "my.gitlab.com" lookup := cache.Resolve(context.Background(), "my.gitlab.com") @@ -134,7 +134,7 @@ func TestResolve(t *testing.T) { }) t.Run("when item is not cached and accessed multiple times", func(t *testing.T) { - withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) { + withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) { wg := &sync.WaitGroup{} ctx := context.Background() @@ -158,7 +158,7 @@ func TestResolve(t *testing.T) { }) t.Run("when item is in short cache", func(t *testing.T) { - withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) { + withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) { cache.withTestEntry(entryConfig{expired: false, retrieved: true}, func(*Entry) { lookup := cache.Resolve(context.Background(), "my.gitlab.com") @@ -169,7 +169,7 @@ func TestResolve(t *testing.T) { }) t.Run("when a non-retrieved new item is in short cache", func(t *testing.T) { - withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) { + withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) { cache.withTestEntry(entryConfig{expired: false, retrieved: false}, func(*Entry) { lookup := make(chan *api.Lookup, 1) @@ -192,7 +192,7 @@ func TestResolve(t *testing.T) { }) t.Run("when item is in long cache only", func(t *testing.T) { - withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) { + withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) { cache.withTestEntry(entryConfig{expired: true, retrieved: true}, func(*Entry) { lookup := cache.Resolve(context.Background(), "my.gitlab.com") @@ -206,7 +206,7 @@ func TestResolve(t *testing.T) { }) t.Run("when item in long cache is requested multiple times", func(t *testing.T) { - withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) { + withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) { cache.withTestEntry(entryConfig{expired: true, retrieved: true}, func(*Entry) { cache.Resolve(context.Background(), "my.gitlab.com") cache.Resolve(context.Background(), "my.gitlab.com") @@ -221,9 +221,10 @@ func TestResolve(t *testing.T) { }) t.Run("when retrieval failed with an error", func(t *testing.T) { - withTestCache(resolverConfig{failure: errors.New("500 err")}, func(cache *Cache, resolver *client) { - maxRetrievalInterval = 0 + cc := defaultCacheConfig + cc.maxRetrievalInterval = 0 + withTestCache(resolverConfig{failure: errors.New("500 err")}, &cc, func(cache *Cache, resolver *client) { lookup := cache.Resolve(context.Background(), "my.gitlab.com") require.Equal(t, uint64(3), resolver.stats.getLookups()) @@ -235,7 +236,7 @@ func TestResolve(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() - withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) { + withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) { lookup := cache.Resolve(ctx, "my.gitlab.com") require.Equal(t, uint64(0), resolver.stats.getLookups()) @@ -244,12 +245,10 @@ func TestResolve(t *testing.T) { }) t.Run("when retrieval failed because of an internal retriever context timeout", func(t *testing.T) { - t.Skip("Data race") + cc := defaultCacheConfig + cc.retrievalTimeout = 0 - retrievalTimeout = 0 - defer func() { retrievalTimeout = 5 * time.Second }() - - withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) { + withTestCache(resolverConfig{}, &cc, func(cache *Cache, resolver *client) { lookup := cache.Resolve(context.Background(), "my.gitlab.com") require.Equal(t, uint64(0), resolver.stats.getLookups()) @@ -258,7 +257,7 @@ func TestResolve(t *testing.T) { }) t.Run("when retrieval failed because of resolution context being canceled", func(t *testing.T) { - withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) { + withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) { cache.withTestEntry(entryConfig{expired: false, retrieved: false}, func(entry *Entry) { ctx, cancel := context.WithCancel(context.Background()) diff --git a/internal/source/gitlab/cache/const.go b/internal/source/gitlab/cache/const.go deleted file mode 100644 index 90c5bddc..00000000 --- a/internal/source/gitlab/cache/const.go +++ /dev/null @@ -1,11 +0,0 @@ -package cache - -import "time" - -var ( - shortCacheExpiry = 30 * time.Second - longCacheExpiry = 10 * time.Minute - retrievalTimeout = 5 * time.Second - maxRetrievalInterval = time.Second - maxRetrievalRetries = 3 -) diff --git a/internal/source/gitlab/cache/entry.go b/internal/source/gitlab/cache/entry.go index d91bb331..191ef789 100644 --- a/internal/source/gitlab/cache/entry.go +++ b/internal/source/gitlab/cache/entry.go @@ -13,23 +13,28 @@ import ( // holds a pointer to *api.Lookup when the domain lookup has been retrieved // successfully type Entry struct { - domain string - created time.Time - retrieve *sync.Once - refresh *sync.Once - mux *sync.RWMutex - retrieved chan struct{} - response *api.Lookup + domain string + created time.Time + retrieve *sync.Once + refresh *sync.Once + mux *sync.RWMutex + retrieved chan struct{} + response *api.Lookup + refreshTimeout time.Duration + retriever *Retriever } -func newCacheEntry(domain string) *Entry { +func newCacheEntry(domain string, refreshTimeout time.Duration, retriever *Retriever) *Entry { + return &Entry{ - domain: domain, - created: time.Now(), - retrieve: &sync.Once{}, - refresh: &sync.Once{}, - mux: &sync.RWMutex{}, - retrieved: make(chan struct{}), + domain: domain, + created: time.Now(), + retrieve: &sync.Once{}, + refresh: &sync.Once{}, + mux: &sync.RWMutex{}, + retrieved: make(chan struct{}), + refreshTimeout: refreshTimeout, + retriever: retriever, } } @@ -61,7 +66,7 @@ func (e *Entry) Lookup() *api.Lookup { // Retrieve perform a blocking retrieval of the cache entry response. func (e *Entry) Retrieve(ctx context.Context, client api.Client) (lookup *api.Lookup) { - e.retrieve.Do(func() { go e.retrieveWithClient(client) }) + e.retrieve.Do(func() { go e.setResponse(e.retriever.Retrieve(e.domain)) }) select { case <-ctx.Done(): @@ -77,7 +82,7 @@ func (e *Entry) Retrieve(ctx context.Context, client api.Client) (lookup *api.Lo func (e *Entry) Refresh(client api.Client, store Store) { e.refresh.Do(func() { go func() { - entry := newCacheEntry(e.domain) + entry := newCacheEntry(e.domain, e.refreshTimeout, e.retriever) entry.Retrieve(context.Background(), client) @@ -86,12 +91,6 @@ func (e *Entry) Refresh(client api.Client, store Store) { }) } -func (e *Entry) retrieveWithClient(client api.Client) { - retriever := Retriever{client: client} - - e.setResponse(retriever.Retrieve(e.domain)) -} - func (e *Entry) setResponse(lookup api.Lookup) { e.mux.Lock() defer e.mux.Unlock() @@ -101,7 +100,7 @@ func (e *Entry) setResponse(lookup api.Lookup) { } func (e *Entry) isExpired() bool { - return time.Since(e.created) > shortCacheExpiry + return time.Since(e.created) > e.refreshTimeout } func (e *Entry) isResolved() bool { diff --git a/internal/source/gitlab/cache/entry_test.go b/internal/source/gitlab/cache/entry_test.go index 118981d0..e6a86557 100644 --- a/internal/source/gitlab/cache/entry_test.go +++ b/internal/source/gitlab/cache/entry_test.go @@ -10,35 +10,55 @@ import ( ) func TestIsUpToDateAndNeedsRefresh(t *testing.T) { - t.Run("when is resolved and not expired", func(t *testing.T) { - entry := newCacheEntry("my.gitlab.com") - entry.response = &api.Lookup{} + tests := []struct { + name string + resolved bool + expired bool + expectedIsUpToDate bool + expectedNeedRefresh bool + }{ + { + name: "resolved_and_not_expired", + resolved: true, + expired: false, + expectedIsUpToDate: true, + expectedNeedRefresh: false, + }, + { + name: "resolved_and_expired", + resolved: true, + expired: true, + expectedIsUpToDate: false, + expectedNeedRefresh: true, + }, + { + name: "not_resolved_and_not_expired", + resolved: false, + expired: false, + expectedIsUpToDate: false, + expectedNeedRefresh: false, + }, + { + name: "not_resolved_and_expired", + resolved: false, + expired: true, + expectedIsUpToDate: false, + expectedNeedRefresh: false, + }, + } - require.True(t, entry.IsUpToDate()) - require.False(t, entry.NeedsRefresh()) - }) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + entry := newCacheEntry("my.gitlab.com", defaultCacheConfig.entryRefreshTimeout, nil) + if tt.resolved { + entry.response = &api.Lookup{} + } + if tt.expired { + entry.created = time.Now().Add(-time.Hour) + } - t.Run("when is resolved and is expired", func(t *testing.T) { - entry := newCacheEntry("my.gitlab.com") - entry.response = &api.Lookup{} - entry.created = time.Now().Add(-time.Hour) - - require.False(t, entry.IsUpToDate()) - require.True(t, entry.NeedsRefresh()) - }) - - t.Run("when is not resolved and not expired", func(t *testing.T) { - entry := newCacheEntry("my.gitlab.com") - - require.False(t, entry.IsUpToDate()) - require.False(t, entry.NeedsRefresh()) - }) - - t.Run("when is not resolved and is expired", func(t *testing.T) { - entry := newCacheEntry("my.gitlab.com") - entry.created = time.Now().Add(-time.Hour) - - require.False(t, entry.IsUpToDate()) - require.False(t, entry.NeedsRefresh()) - }) + require.Equal(t, tt.expectedIsUpToDate, entry.IsUpToDate()) + require.Equal(t, tt.expectedNeedRefresh, entry.NeedsRefresh()) + }) + } } diff --git a/internal/source/gitlab/cache/memstore.go b/internal/source/gitlab/cache/memstore.go index 08eccd94..1d7c678d 100644 --- a/internal/source/gitlab/cache/memstore.go +++ b/internal/source/gitlab/cache/memstore.go @@ -5,17 +5,24 @@ import ( "time" cache "github.com/patrickmn/go-cache" + + "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api" ) type memstore struct { - store *cache.Cache - mux *sync.RWMutex + store *cache.Cache + mux *sync.RWMutex + retriever *Retriever + entryRefreshTimeout time.Duration } -func newMemStore() Store { +func newMemStore(client api.Client, cc *cacheConfig) Store { + retriever := NewRetriever(client, cc.retrievalTimeout, cc.maxRetrievalInterval, cc.maxRetrievalRetries) return &memstore{ - store: cache.New(longCacheExpiry, time.Minute), - mux: &sync.RWMutex{}, + store: cache.New(cc.cacheExpiry, time.Minute), + mux: &sync.RWMutex{}, + retriever: retriever, + entryRefreshTimeout: cc.entryRefreshTimeout, } } @@ -37,7 +44,7 @@ func (m *memstore) LoadOrCreate(domain string) *Entry { return entry.(*Entry) } - newEntry := newCacheEntry(domain) + newEntry := newCacheEntry(domain, m.entryRefreshTimeout, m.retriever) m.store.SetDefault(domain, newEntry) return newEntry diff --git a/internal/source/gitlab/cache/retriever.go b/internal/source/gitlab/cache/retriever.go index cf0541b1..de37c231 100644 --- a/internal/source/gitlab/cache/retriever.go +++ b/internal/source/gitlab/cache/retriever.go @@ -13,13 +13,26 @@ import ( // Retriever is an utility type that performs an HTTP request with backoff in // case of errors type Retriever struct { - client api.Client + client api.Client + retrievalTimeout time.Duration + maxRetrievalInterval time.Duration + maxRetrievalRetries int +} + +// NewRetriever creates a Retriever with a client +func NewRetriever(client api.Client, retrievalTimeout, maxRetrievalInterval time.Duration, maxRetrievalRetries int) *Retriever { + return &Retriever{ + client: client, + retrievalTimeout: retrievalTimeout, + maxRetrievalInterval: maxRetrievalInterval, + maxRetrievalRetries: maxRetrievalRetries, + } } // Retrieve retrieves a lookup response from external source with timeout and // backoff. It has its own context with timeout. func (r *Retriever) Retrieve(domain string) (lookup api.Lookup) { - ctx, cancel := context.WithTimeout(context.Background(), retrievalTimeout) + ctx, cancel := context.WithTimeout(context.Background(), r.retrievalTimeout) defer cancel() select { @@ -39,11 +52,11 @@ func (r *Retriever) resolveWithBackoff(ctx context.Context, domain string) <-cha go func() { var lookup api.Lookup - for i := 1; i <= maxRetrievalRetries; i++ { + for i := 1; i <= r.maxRetrievalRetries; i++ { lookup = r.client.GetLookup(ctx, domain) if lookup.Error != nil { - time.Sleep(maxRetrievalInterval) + time.Sleep(r.maxRetrievalInterval) } else { break } |