From f9861b86660e162adf41d333b3afdb12238f9b04 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Fri, 1 Mar 2024 09:46:09 +0530 Subject: [PATCH] MM-56987: Improve cache API (#26298) This is in preparation to make the codebase ready to use Redis. In Redis, iterating all the keys at once is an expensive operation. It is recommended to work on batches of keys. Remove the Len method as it was unused. I tried to repurpose the Keys method to iterate on keys rather than returning all keys at once, but it has other complicacies because the code calls other cache functions on those keys, so to handle the LRU cache properly, it becomes slightly more painful. For now, we keep it like this and rather collect all keys from Redis and then return. https://mattermost.atlassian.net/browse/MM-56987 ```release-note NONE ``` --- server/channels/app/platform/session.go | 7 ------- .../localcachelayer/terms_of_service_layer.go | 8 +++----- server/platform/services/cache/cache.go | 3 --- server/platform/services/cache/lru_test.go | 9 +++++---- .../platform/services/cache/provider_test.go | 18 ------------------ 5 files changed, 8 insertions(+), 37 deletions(-) diff --git a/server/channels/app/platform/session.go b/server/channels/app/platform/session.go index 87262b465c..cfcd976c46 100644 --- a/server/channels/app/platform/session.go +++ b/server/channels/app/platform/session.go @@ -45,13 +45,6 @@ func (ps *PlatformService) AddSessionToCache(session *model.Session) { ps.sessionCache.SetWithExpiry(session.Token, session, time.Duration(int64(*ps.Config().ServiceSettings.SessionCacheInMinutes))*time.Minute) } -func (ps *PlatformService) SessionCacheLength() int { - if l, err := ps.sessionCache.Len(); err == nil { - return l - } - return 0 -} - func (ps *PlatformService) ClearUserSessionCacheLocal(userID string) { if keys, err := ps.sessionCache.Keys(); err == nil { var session *model.Session diff --git a/server/channels/store/localcachelayer/terms_of_service_layer.go b/server/channels/store/localcachelayer/terms_of_service_layer.go index 39338d7711..d90ebd73a1 100644 --- a/server/channels/store/localcachelayer/terms_of_service_layer.go +++ b/server/channels/store/localcachelayer/terms_of_service_layer.go @@ -47,11 +47,9 @@ func (s LocalCacheTermsOfServiceStore) Save(termsOfService *model.TermsOfService func (s LocalCacheTermsOfServiceStore) GetLatest(allowFromCache bool) (*model.TermsOfService, error) { if allowFromCache { - if l, err := s.rootStore.termsOfServiceCache.Len(); err == nil && l != 0 { - var cacheItem *model.TermsOfService - if err := s.rootStore.doStandardReadCache(s.rootStore.termsOfServiceCache, LatestKey, &cacheItem); err == nil { - return cacheItem, nil - } + var cacheItem *model.TermsOfService + if err := s.rootStore.doStandardReadCache(s.rootStore.termsOfServiceCache, LatestKey, &cacheItem); err == nil { + return cacheItem, nil } } diff --git a/server/platform/services/cache/cache.go b/server/platform/services/cache/cache.go index 9e2530b0d9..8311ea734a 100644 --- a/server/platform/services/cache/cache.go +++ b/server/platform/services/cache/cache.go @@ -40,9 +40,6 @@ type Cache interface { // Keys returns a slice of the keys in the cache. Keys() ([]string, error) - // Len returns the number of items in the cache. - Len() (int, error) - // GetInvalidateClusterEvent returns the cluster event configured when this cache was created. GetInvalidateClusterEvent() model.ClusterEvent diff --git a/server/platform/services/cache/lru_test.go b/server/platform/services/cache/lru_test.go index b91ce4c871..8e1417ab48 100644 --- a/server/platform/services/cache/lru_test.go +++ b/server/platform/services/cache/lru_test.go @@ -26,8 +26,10 @@ func TestLRU(t *testing.T) { err := l.Set(fmt.Sprintf("%d", i), i) require.NoError(t, err) } - size, err := l.Len() - require.NoError(t, err) + + lru, ok := l.(*LRU) + require.True(t, ok) + size := lru.len require.Equalf(t, size, 128, "bad len: %v", size) keys, err := l.Keys() @@ -69,8 +71,7 @@ func TestLRU(t *testing.T) { } l.Purge() - size, err = l.Len() - require.NoError(t, err) + size = lru.len require.Equalf(t, size, 0, "bad len: %v", size) err = l.Get("200", &v) require.Equal(t, err, ErrKeyNotFound, "should contain nothing") diff --git a/server/platform/services/cache/provider_test.go b/server/platform/services/cache/provider_test.go index e25981528b..e373e891b3 100644 --- a/server/platform/services/cache/provider_test.go +++ b/server/platform/services/cache/provider_test.go @@ -28,9 +28,6 @@ func TestNewCache(t *testing.T) { require.NoError(t, err) err = c.Set("key3", "val3") require.NoError(t, err) - l, err := c.Len() - require.NoError(t, err) - require.Equal(t, size, l) }) t.Run("with only size option given", func(t *testing.T) { @@ -48,9 +45,6 @@ func TestNewCache(t *testing.T) { require.NoError(t, err) err = c.Set("key3", "val3") require.NoError(t, err) - l, err := c.Len() - require.NoError(t, err) - require.Equal(t, size, l) }) t.Run("with all options specified", func(t *testing.T) { @@ -75,9 +69,6 @@ func TestNewCache(t *testing.T) { require.NoError(t, err) err = c.SetWithDefaultExpiry("key3", "val3") require.NoError(t, err) - l, err := c.Len() - require.NoError(t, err) - require.Equal(t, size, l) time.Sleep(expiry + 1*time.Second) @@ -109,9 +100,6 @@ func TestNewCache_Striped(t *testing.T) { require.NoError(t, err) err = c.Set("key3", "val3") require.NoError(t, err) - l, err := c.Len() - require.NoError(t, err) - require.Equal(t, size+1, l) // +10% from striping }) t.Run("with only size option given", func(t *testing.T) { @@ -131,9 +119,6 @@ func TestNewCache_Striped(t *testing.T) { require.NoError(t, err) err = c.Set("key3", "val3") require.NoError(t, err) - l, err := c.Len() - require.NoError(t, err) - require.Equal(t, size+1, l) // +10% rounded up from striped lru }) t.Run("with all options specified", func(t *testing.T) { @@ -160,9 +145,6 @@ func TestNewCache_Striped(t *testing.T) { require.NoError(t, err) err = c.SetWithDefaultExpiry("key3", "val3") require.NoError(t, err) - l, err := c.Len() - require.NoError(t, err) - require.Equal(t, size+1, l) // +10% from striping time.Sleep(expiry + 1*time.Second)