diff options
author | Vladimir Shushlin <vshushlin@gitlab.com> | 2021-11-17 16:56:11 +0300 |
---|---|---|
committer | Vladimir Shushlin <vshushlin@gitlab.com> | 2021-11-17 16:56:11 +0300 |
commit | 1a00719667fdd852727b8fcbad67f722296fa5f7 (patch) | |
tree | 076d05f7ad27dac666827150f16af919df278aff | |
parent | dbb496cb31e211a0df7b4a1603a56ba5337a3404 (diff) | |
parent | 94465c8fbbcc08ecceab7a0990d06445b7e561ea (diff) |
Merge branch 'fix/decouple-memstore-retriever' into 'master'
refactor: decouple memstore from retriever in cache package
Closes #553 and #368
See merge request gitlab-org/gitlab-pages!494
-rw-r--r-- | internal/source/gitlab/cache/cache.go | 56 | ||||
-rw-r--r-- | internal/source/gitlab/cache/entry.go | 47 | ||||
-rw-r--r-- | internal/source/gitlab/cache/entry_test.go | 30 | ||||
-rw-r--r-- | internal/source/gitlab/cache/memstore.go | 9 |
4 files changed, 68 insertions, 74 deletions
diff --git a/internal/source/gitlab/cache/cache.go b/internal/source/gitlab/cache/cache.go index cb12c088..7a81b3af 100644 --- a/internal/source/gitlab/cache/cache.go +++ b/internal/source/gitlab/cache/cache.go @@ -2,6 +2,7 @@ package cache import ( "context" + "errors" "gitlab.com/gitlab-org/gitlab-pages/internal/config" "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api" @@ -10,15 +11,16 @@ import ( // Cache is a short and long caching mechanism for GitLab source type Cache struct { - client api.Client - store Store + store Store + retriever *Retriever } // NewCache creates a new instance of Cache. func NewCache(client api.Client, cc *config.Cache) *Cache { + r := NewRetriever(client, cc.RetrievalTimeout, cc.MaxRetrievalInterval, cc.MaxRetrievalRetries) return &Cache{ - client: client, - store: newMemStore(client, cc), + store: newMemStore(cc), + retriever: r, } } @@ -80,12 +82,54 @@ func (c *Cache) Resolve(ctx context.Context, domain string) *api.Lookup { } if entry.NeedsRefresh() { - entry.Refresh(c.store) + c.Refresh(entry) metrics.DomainsSourceCacheHit.Inc() return entry.Lookup() } metrics.DomainsSourceCacheMiss.Inc() - return entry.Retrieve(ctx) + return c.retrieve(ctx, entry) +} + +func (c *Cache) retrieve(ctx context.Context, entry *Entry) *api.Lookup { + // We run the code within an additional func() to run both `e.setResponse` + // and `c.retriever.Retrieve` asynchronously. + entry.retrieve.Do(func() { go func() { entry.setResponse(c.retriever.Retrieve(ctx, entry.domain)) }() }) + + var lookup *api.Lookup + select { + case <-ctx.Done(): + lookup = &api.Lookup{Name: entry.domain, Error: errors.New("context done")} + case <-entry.retrieved: + lookup = entry.Lookup() + } + + return lookup +} + +// Refresh will update the entry in the store only when it gets resolved successfully. +// If an existing successful entry exists, it will only be replaced if the new resolved +// entry is successful too. +// Errored refreshed Entry responses will not replace the previously successful entry.response +// for a maximum time of e.expirationTimeout. +func (c *Cache) Refresh(entry *Entry) { + entry.refresh.Do(func() { + go c.refreshFunc(entry) + }) +} + +func (c *Cache) refreshFunc(e *Entry) { + entry := newCacheEntry(e.domain, e.refreshTimeout, e.expirationTimeout) + + c.retrieve(context.Background(), entry) + + // do not replace existing Entry `e.response` when `entry.response` has an error + // and `e` has not expired. See https://gitlab.com/gitlab-org/gitlab-pages/-/issues/281. + if !e.isExpired() && entry.hasTemporaryError() { + entry.response = e.response + entry.refreshedOriginalTimestamp = e.created + } + + c.store.ReplaceOrCreate(e.domain, entry) } diff --git a/internal/source/gitlab/cache/entry.go b/internal/source/gitlab/cache/entry.go index 769f1713..742df9c8 100644 --- a/internal/source/gitlab/cache/entry.go +++ b/internal/source/gitlab/cache/entry.go @@ -1,7 +1,6 @@ package cache import ( - "context" "errors" "os" "sync" @@ -25,10 +24,9 @@ type Entry struct { response *api.Lookup refreshTimeout time.Duration expirationTimeout time.Duration - retriever *Retriever } -func newCacheEntry(domain string, refreshTimeout, entryExpirationTimeout time.Duration, retriever *Retriever) *Entry { +func newCacheEntry(domain string, refreshTimeout, entryExpirationTimeout time.Duration) *Entry { return &Entry{ domain: domain, created: time.Now(), @@ -38,7 +36,6 @@ func newCacheEntry(domain string, refreshTimeout, entryExpirationTimeout time.Du retrieved: make(chan struct{}), refreshTimeout: refreshTimeout, expirationTimeout: entryExpirationTimeout, - retriever: retriever, } } @@ -68,48 +65,6 @@ func (e *Entry) Lookup() *api.Lookup { return e.response } -// Retrieve perform a blocking retrieval of the cache entry response. -func (e *Entry) Retrieve(ctx context.Context) (lookup *api.Lookup) { - // We run the code within an additional func() to run both `e.setResponse` - // and `e.retrieve.Retrieve` asynchronously. - e.retrieve.Do(func() { go func() { e.setResponse(e.retriever.Retrieve(ctx, e.domain)) }() }) - - select { - case <-ctx.Done(): - lookup = &api.Lookup{Name: e.domain, Error: errors.New("context done")} - case <-e.retrieved: - lookup = e.Lookup() - } - - return lookup -} - -// Refresh will update the entry in the store only when it gets resolved successfully. -// If an existing successful entry exists, it will only be replaced if the new resolved -// entry is successful too. -// Errored refreshed Entry responses will not replace the previously successful entry.response -// for a maximum time of e.expirationTimeout. -func (e *Entry) Refresh(store Store) { - e.refresh.Do(func() { - go e.refreshFunc(store) - }) -} - -func (e *Entry) refreshFunc(store Store) { - entry := newCacheEntry(e.domain, e.refreshTimeout, e.expirationTimeout, e.retriever) - - entry.Retrieve(context.Background()) - - // do not replace existing Entry `e.response` when `entry.response` has an error - // and `e` has not expired. See https://gitlab.com/gitlab-org/gitlab-pages/-/issues/281. - if !e.isExpired() && entry.hasTemporaryError() { - entry.response = e.response - entry.refreshedOriginalTimestamp = e.created - } - - store.ReplaceOrCreate(e.domain, entry) -} - func (e *Entry) setResponse(lookup api.Lookup) { e.mux.Lock() defer e.mux.Unlock() diff --git a/internal/source/gitlab/cache/entry_test.go b/internal/source/gitlab/cache/entry_test.go index 07282cb5..d50822ce 100644 --- a/internal/source/gitlab/cache/entry_test.go +++ b/internal/source/gitlab/cache/entry_test.go @@ -55,7 +55,7 @@ func TestIsUpToDateAndNeedsRefresh(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - entry := newCacheEntry("my.gitlab.com", testCacheConfig.EntryRefreshTimeout, testCacheConfig.CacheExpiry, nil) + entry := newCacheEntry("my.gitlab.com", testCacheConfig.EntryRefreshTimeout, testCacheConfig.CacheExpiry) if tt.resolved { entry.response = &api.Lookup{} } @@ -85,6 +85,7 @@ func TestEntryRefresh(t *testing.T) { }, }, } + cc := &config.Cache{ CacheExpiry: 100 * time.Millisecond, EntryRefreshTimeout: time.Millisecond, @@ -92,26 +93,25 @@ func TestEntryRefresh(t *testing.T) { MaxRetrievalInterval: time.Millisecond, MaxRetrievalRetries: 1, } - - store := newMemStore(client, cc) + cache := NewCache(client, cc) t.Run("entry is the same after refreshed lookup has error", func(t *testing.T) { - entry := newCacheEntry("test.gitlab.io", cc.EntryRefreshTimeout, cc.CacheExpiry, store.(*memstore).retriever) + entry := newCacheEntry("test.gitlab.io", cc.EntryRefreshTimeout, cc.CacheExpiry) originalEntryCreated := entry.created ctx, cancel := context.WithTimeout(context.Background(), cc.RetrievalTimeout) defer cancel() - lookup := entry.Retrieve(ctx) + lookup := cache.retrieve(ctx, entry) require.NoError(t, lookup.Error) require.Eventually(t, entry.NeedsRefresh, 100*time.Millisecond, time.Millisecond, "entry should need refresh") - entry.refreshFunc(store) + cache.refreshFunc(entry) require.True(t, client.failed, "refresh should have failed") - storedEntry := loadEntry(t, "test.gitlab.io", store) + storedEntry := loadEntry(t, "test.gitlab.io", cache.store) require.NoError(t, storedEntry.Lookup().Error, "resolving failed but lookup should still be valid") require.Equal(t, storedEntry.refreshedOriginalTimestamp.UnixNano(), originalEntryCreated.UnixNano(), @@ -123,23 +123,23 @@ func TestEntryRefresh(t *testing.T) { client.failed = false err := os.Setenv("FF_DISABLE_REFRESH_TEMPORARY_ERROR", "true") - entry := newCacheEntry("test.gitlab.io", cc.EntryRefreshTimeout, cc.CacheExpiry, store.(*memstore).retriever) + entry := newCacheEntry("test.gitlab.io", cc.EntryRefreshTimeout, cc.CacheExpiry) ctx, cancel := context.WithTimeout(context.Background(), cc.RetrievalTimeout) defer cancel() - lookup := entry.Retrieve(ctx) + lookup := cache.retrieve(ctx, entry) require.NoError(t, lookup.Error) require.Eventually(t, entry.NeedsRefresh, 100*time.Millisecond, time.Millisecond, "entry should need refresh") require.NoError(t, err) - entry.refreshFunc(store) + cache.refreshFunc(entry) require.True(t, client.failed, "refresh should have failed") - storedEntry := loadEntry(t, "test.gitlab.io", store) + storedEntry := loadEntry(t, "test.gitlab.io", cache.store) require.Error(t, storedEntry.Lookup().Error, "resolving failed") require.True(t, storedEntry.refreshedOriginalTimestamp.IsZero()) @@ -149,23 +149,23 @@ func TestEntryRefresh(t *testing.T) { t.Run("entry is different after it expired and calling refresh on it", func(t *testing.T) { client.failed = false - entry := newCacheEntry("error.gitlab.io", cc.EntryRefreshTimeout, cc.CacheExpiry, store.(*memstore).retriever) + entry := newCacheEntry("error.gitlab.io", cc.EntryRefreshTimeout, cc.CacheExpiry) ctx, cancel := context.WithTimeout(context.Background(), cc.RetrievalTimeout) defer cancel() - lookup := entry.Retrieve(ctx) + lookup := cache.retrieve(ctx, entry) require.Error(t, lookup.Error) require.Eventually(t, entry.NeedsRefresh, 100*time.Millisecond, time.Millisecond, "entry should need refresh") // wait for entry to expire time.Sleep(cc.CacheExpiry) // refreshing the entry after it has expired should create a completely new one - entry.refreshFunc(store) + cache.refreshFunc(entry) require.True(t, client.failed, "refresh should have failed") - storedEntry := loadEntry(t, "error.gitlab.io", store) + storedEntry := loadEntry(t, "error.gitlab.io", cache.store) require.NotEqual(t, storedEntry, entry, "stored entry should be different") require.Greater(t, storedEntry.created.UnixNano(), entry.created.UnixNano(), "") }) diff --git a/internal/source/gitlab/cache/memstore.go b/internal/source/gitlab/cache/memstore.go index 8ba069e4..721a2b0a 100644 --- a/internal/source/gitlab/cache/memstore.go +++ b/internal/source/gitlab/cache/memstore.go @@ -7,24 +7,19 @@ import ( "github.com/patrickmn/go-cache" "gitlab.com/gitlab-org/gitlab-pages/internal/config" - "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api" ) type memstore struct { store *cache.Cache mux *sync.RWMutex - retriever *Retriever entryRefreshTimeout time.Duration entryExpirationTimeout time.Duration } -func newMemStore(client api.Client, cc *config.Cache) Store { - retriever := NewRetriever(client, cc.RetrievalTimeout, cc.MaxRetrievalInterval, cc.MaxRetrievalRetries) - +func newMemStore(cc *config.Cache) Store { return &memstore{ store: cache.New(cc.CacheExpiry, cc.CacheCleanupInterval), mux: &sync.RWMutex{}, - retriever: retriever, entryRefreshTimeout: cc.EntryRefreshTimeout, entryExpirationTimeout: cc.CacheExpiry, } @@ -48,7 +43,7 @@ func (m *memstore) LoadOrCreate(domain string) *Entry { return entry.(*Entry) } - newEntry := newCacheEntry(domain, m.entryRefreshTimeout, m.entryExpirationTimeout, m.retriever) + newEntry := newCacheEntry(domain, m.entryRefreshTimeout, m.entryExpirationTimeout) m.store.SetDefault(domain, newEntry) return newEntry |