From cd382412fdececb0f98f01c4f65bc0058ae90ca6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jes=C3=BAs=20Espino?= Date: Mon, 9 Mar 2020 18:34:25 +0100 Subject: [PATCH] Migrating file info store cache to the cache layer (#13045) * Migrating file info store cache to the cache layer * Remove unrelated changes * Updating timer layer store * Fixing usage of invalidate cache * Removed repeated tests and unnecesiary require * Using doInvalidateCacheCluster instead of removing the cache directly * Addressing PR comments * Fixing imports * Fixing tests * Fixing license header --- app/post.go | 3 +- model/cluster_message.go | 1 + store/localcachelayer/file_info_layer.go | 66 +++++++++++++++++++ store/localcachelayer/file_info_layer_test.go | 59 +++++++++++++++++ store/localcachelayer/layer.go | 18 +++++ store/localcachelayer/main_test.go | 12 ++++ store/opentracing_layer.go | 4 +- store/sqlstore/file_info_store.go | 44 +------------ store/store.go | 2 +- store/storetest/mocks/FileInfoStore.go | 6 +- store/timer_layer.go | 4 +- 11 files changed, 167 insertions(+), 52 deletions(-) create mode 100644 store/localcachelayer/file_info_layer.go create mode 100644 store/localcachelayer/file_info_layer_test.go diff --git a/app/post.go b/app/post.go index ceb34dc7e7..424090834e 100644 --- a/app/post.go +++ b/app/post.go @@ -1141,7 +1141,8 @@ func (a *App) GetFileInfosForPostWithMigration(postId string) ([]*model.FileInfo post := result.Data.(*model.Post) if len(post.Filenames) > 0 { - a.Srv().Store.FileInfo().InvalidateFileInfosForPostCache(postId) + a.Srv().Store.FileInfo().InvalidateFileInfosForPostCache(postId, false) + a.Srv().Store.FileInfo().InvalidateFileInfosForPostCache(postId, true) // The post has Filenames that need to be replaced with FileInfos infos = a.MigrateFilenamesToFileInfos(post) } diff --git a/model/cluster_message.go b/model/cluster_message.go index ec62eb7588..0105d9481e 100644 --- a/model/cluster_message.go +++ b/model/cluster_message.go @@ -27,6 +27,7 @@ const ( CLUSTER_EVENT_INVALIDATE_CACHE_FOR_PROFILE_BY_IDS = "inv_profile_ids" CLUSTER_EVENT_INVALIDATE_CACHE_FOR_PROFILE_IN_CHANNEL = "inv_profile_in_channel" CLUSTER_EVENT_INVALIDATE_CACHE_FOR_SCHEMES = "inv_schemes" + CLUSTER_EVENT_INVALIDATE_CACHE_FOR_FILE_INFOS = "inv_file_infos" CLUSTER_EVENT_INVALIDATE_CACHE_FOR_WEBHOOKS = "inv_webhooks" CLUSTER_EVENT_INVALIDATE_CACHE_FOR_EMOJIS_BY_ID = "inv_emojis_by_id" CLUSTER_EVENT_INVALIDATE_CACHE_FOR_EMOJIS_ID_BY_NAME = "inv_emojis_id_by_name" diff --git a/store/localcachelayer/file_info_layer.go b/store/localcachelayer/file_info_layer.go new file mode 100644 index 0000000000..1d02a5cbd7 --- /dev/null +++ b/store/localcachelayer/file_info_layer.go @@ -0,0 +1,66 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package localcachelayer + +import ( + "github.com/mattermost/mattermost-server/v5/model" + "github.com/mattermost/mattermost-server/v5/store" +) + +type LocalCacheFileInfoStore struct { + store.FileInfoStore + rootStore *LocalCacheStore +} + +func (s *LocalCacheFileInfoStore) handleClusterInvalidateFileInfo(msg *model.ClusterMessage) { + if msg.Data == CLEAR_CACHE_MESSAGE_DATA { + s.rootStore.fileInfoCache.Purge() + return + } + s.rootStore.fileInfoCache.Remove(msg.Data) +} + +func (s LocalCacheFileInfoStore) GetForPost(postId string, readFromMaster, includeDeleted, allowFromCache bool) ([]*model.FileInfo, *model.AppError) { + if !allowFromCache { + return s.FileInfoStore.GetForPost(postId, readFromMaster, includeDeleted, allowFromCache) + } + + cacheKey := postId + if includeDeleted { + cacheKey += "_deleted" + } + + if fileInfo := s.rootStore.doStandardReadCache(s.rootStore.fileInfoCache, cacheKey); fileInfo != nil { + return fileInfo.([]*model.FileInfo), nil + } + + fileInfos, err := s.FileInfoStore.GetForPost(postId, readFromMaster, includeDeleted, allowFromCache) + if err != nil { + return nil, err + } + + if len(fileInfos) > 0 { + s.rootStore.doStandardAddToCache(s.rootStore.fileInfoCache, cacheKey, fileInfos) + } + + return fileInfos, nil +} + +func (s LocalCacheFileInfoStore) ClearCaches() { + s.rootStore.fileInfoCache.Purge() + if s.rootStore.metrics != nil { + s.rootStore.metrics.IncrementMemCacheInvalidationCounter("File Info Cache - Purge") + } +} + +func (s LocalCacheFileInfoStore) InvalidateFileInfosForPostCache(postId string, deleted bool) { + cacheKey := postId + if deleted { + cacheKey += "_deleted" + } + s.rootStore.doInvalidateCacheCluster(s.rootStore.fileInfoCache, cacheKey) + if s.rootStore.metrics != nil { + s.rootStore.metrics.IncrementMemCacheInvalidationCounter("File Info Cache - Remove by PostId") + } +} diff --git a/store/localcachelayer/file_info_layer_test.go b/store/localcachelayer/file_info_layer_test.go new file mode 100644 index 0000000000..29e4ef92cb --- /dev/null +++ b/store/localcachelayer/file_info_layer_test.go @@ -0,0 +1,59 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package localcachelayer + +import ( + "testing" + + "github.com/mattermost/mattermost-server/v5/model" + "github.com/mattermost/mattermost-server/v5/store/storetest" + "github.com/mattermost/mattermost-server/v5/store/storetest/mocks" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFileInfoStore(t *testing.T) { + StoreTest(t, storetest.TestFileInfoStore) +} + +func TestFileInfoStoreCache(t *testing.T) { + fakeFileInfo := model.FileInfo{PostId: "123"} + + t.Run("first call not cached, second cached and returning same data", func(t *testing.T) { + mockStore := getMockStore() + mockCacheProvider := getMockCacheProvider() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider) + + fileInfos, err := cachedStore.FileInfo().GetForPost("123", true, true, true) + require.Nil(t, err) + assert.Equal(t, fileInfos, []*model.FileInfo{&fakeFileInfo}) + mockStore.FileInfo().(*mocks.FileInfoStore).AssertNumberOfCalls(t, "GetForPost", 1) + assert.Equal(t, fileInfos, []*model.FileInfo{&fakeFileInfo}) + cachedStore.FileInfo().GetForPost("123", true, true, true) + mockStore.FileInfo().(*mocks.FileInfoStore).AssertNumberOfCalls(t, "GetForPost", 1) + }) + + t.Run("first call not cached, second force no cached", func(t *testing.T) { + mockStore := getMockStore() + mockCacheProvider := getMockCacheProvider() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider) + + cachedStore.FileInfo().GetForPost("123", true, true, true) + mockStore.FileInfo().(*mocks.FileInfoStore).AssertNumberOfCalls(t, "GetForPost", 1) + cachedStore.FileInfo().GetForPost("123", true, true, false) + mockStore.FileInfo().(*mocks.FileInfoStore).AssertNumberOfCalls(t, "GetForPost", 2) + }) + + t.Run("first call not cached, invalidate, and then not cached again", func(t *testing.T) { + mockStore := getMockStore() + mockCacheProvider := getMockCacheProvider() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider) + + cachedStore.FileInfo().GetForPost("123", true, true, true) + mockStore.FileInfo().(*mocks.FileInfoStore).AssertNumberOfCalls(t, "GetForPost", 1) + cachedStore.FileInfo().InvalidateFileInfosForPostCache("123", true) + cachedStore.FileInfo().GetForPost("123", true, true, true) + mockStore.FileInfo().(*mocks.FileInfoStore).AssertNumberOfCalls(t, "GetForPost", 2) + }) +} diff --git a/store/localcachelayer/layer.go b/store/localcachelayer/layer.go index 81b040cef3..f6f2f54209 100644 --- a/store/localcachelayer/layer.go +++ b/store/localcachelayer/layer.go @@ -20,6 +20,9 @@ const ( SCHEME_CACHE_SIZE = 20000 SCHEME_CACHE_SEC = 30 * 60 + FILE_INFO_CACHE_SIZE = 25000 + FILE_INFO_CACHE_SEC = 30 * 60 + CHANNEL_GUEST_COUNT_CACHE_SIZE = model.CHANNEL_CACHE_SIZE CHANNEL_GUEST_COUNT_CACHE_SEC = 30 * 60 @@ -65,6 +68,9 @@ type LocalCacheStore struct { reaction LocalCacheReactionStore reactionCache cache.Cache + fileInfo LocalCacheFileInfoStore + fileInfoCache cache.Cache + role LocalCacheRoleStore roleCache cache.Cache @@ -118,6 +124,10 @@ func NewLocalCacheLayer(baseStore store.Store, metrics einterfaces.MetricsInterf localCacheStore.schemeCache = cacheProvider.NewCacheWithParams(SCHEME_CACHE_SIZE, "Scheme", SCHEME_CACHE_SEC, model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_SCHEMES) localCacheStore.scheme = LocalCacheSchemeStore{SchemeStore: baseStore.Scheme(), rootStore: &localCacheStore} + // FileInfo + localCacheStore.fileInfoCache = cacheProvider.NewCacheWithParams(FILE_INFO_CACHE_SIZE, "FileInfo", FILE_INFO_CACHE_SEC, model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_FILE_INFOS) + localCacheStore.fileInfo = LocalCacheFileInfoStore{FileInfoStore: baseStore.FileInfo(), rootStore: &localCacheStore} + // Webhooks localCacheStore.webhookCache = cacheProvider.NewCacheWithParams(WEBHOOK_CACHE_SIZE, "Webhook", WEBHOOK_CACHE_SEC, model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_WEBHOOKS) localCacheStore.webhook = LocalCacheWebhookStore{WebhookStore: baseStore.Webhook(), rootStore: &localCacheStore} @@ -156,6 +166,7 @@ func NewLocalCacheLayer(baseStore store.Store, metrics einterfaces.MetricsInterf cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_REACTIONS, localCacheStore.reaction.handleClusterInvalidateReaction) cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_ROLES, localCacheStore.role.handleClusterInvalidateRole) cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_SCHEMES, localCacheStore.scheme.handleClusterInvalidateScheme) + cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_FILE_INFOS, localCacheStore.fileInfo.handleClusterInvalidateFileInfo) cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_LAST_POST_TIME, localCacheStore.post.handleClusterInvalidateLastPostTime) cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_WEBHOOKS, localCacheStore.webhook.handleClusterInvalidateWebhook) cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_EMOJIS_BY_ID, localCacheStore.emoji.handleClusterInvalidateEmojiById) @@ -185,6 +196,10 @@ func (s LocalCacheStore) Scheme() store.SchemeStore { return s.scheme } +func (s LocalCacheStore) FileInfo() store.FileInfoStore { + return s.fileInfo +} + func (s LocalCacheStore) Webhook() store.WebhookStore { return s.webhook } @@ -263,6 +278,9 @@ func (s *LocalCacheStore) doClearCacheCluster(cache cache.Cache) { func (s *LocalCacheStore) Invalidate() { s.doClearCacheCluster(s.reactionCache) + s.doClearCacheCluster(s.schemeCache) + s.doClearCacheCluster(s.roleCache) + s.doClearCacheCluster(s.fileInfoCache) s.doClearCacheCluster(s.webhookCache) s.doClearCacheCluster(s.emojiCacheById) s.doClearCacheCluster(s.emojiIdCacheByName) diff --git a/store/localcachelayer/main_test.go b/store/localcachelayer/main_test.go index a745935be2..93543ed395 100644 --- a/store/localcachelayer/main_test.go +++ b/store/localcachelayer/main_test.go @@ -116,6 +116,12 @@ func getMockCacheProvider() *mocks.CacheProvider { mock.AnythingOfType("int64"), mock.AnythingOfType("string")).Return(lru.New(128)) + mockCacheProvider.On("NewCacheWithParams", + mock.AnythingOfType("int"), + "FileInfo", + mock.AnythingOfType("int64"), + mock.AnythingOfType("string")).Return(lru.New(128)) + return &mockCacheProvider } @@ -147,6 +153,12 @@ func getMockStore() *mocks.Store { mockSchemesStore.On("PermanentDeleteAll").Return(nil) mockStore.On("Scheme").Return(&mockSchemesStore) + fakeFileInfo := model.FileInfo{PostId: "123"} + mockFileInfoStore := mocks.FileInfoStore{} + mockFileInfoStore.On("GetForPost", "123", true, true, false).Return([]*model.FileInfo{&fakeFileInfo}, nil) + mockFileInfoStore.On("GetForPost", "123", true, true, true).Return([]*model.FileInfo{&fakeFileInfo}, nil) + mockStore.On("FileInfo").Return(&mockFileInfoStore) + fakeWebhook := model.IncomingWebhook{Id: "123"} mockWebhookStore := mocks.WebhookStore{} mockWebhookStore.On("GetIncoming", "123", true).Return(&fakeWebhook, nil) diff --git a/store/opentracing_layer.go b/store/opentracing_layer.go index 5816fd74ab..8998cbc614 100644 --- a/store/opentracing_layer.go +++ b/store/opentracing_layer.go @@ -2694,7 +2694,7 @@ func (s *OpenTracingLayerFileInfoStore) GetWithOptions(page int, perPage int, op return resultVar0, resultVar1 } -func (s *OpenTracingLayerFileInfoStore) InvalidateFileInfosForPostCache(postId string) { +func (s *OpenTracingLayerFileInfoStore) InvalidateFileInfosForPostCache(postId string, deleted bool) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "FileInfoStore.InvalidateFileInfosForPostCache") s.Root.Store.SetContext(newCtx) @@ -2703,7 +2703,7 @@ func (s *OpenTracingLayerFileInfoStore) InvalidateFileInfosForPostCache(postId s }() defer span.Finish() - s.FileInfoStore.InvalidateFileInfosForPostCache(postId) + s.FileInfoStore.InvalidateFileInfosForPostCache(postId, deleted) } diff --git a/store/sqlstore/file_info_store.go b/store/sqlstore/file_info_store.go index c485f7369f..d500fc987e 100644 --- a/store/sqlstore/file_info_store.go +++ b/store/sqlstore/file_info_store.go @@ -12,8 +12,6 @@ import ( "github.com/mattermost/mattermost-server/v5/einterfaces" "github.com/mattermost/mattermost-server/v5/model" - "github.com/mattermost/mattermost-server/v5/services/cache" - "github.com/mattermost/mattermost-server/v5/services/cache/lru" "github.com/mattermost/mattermost-server/v5/store" ) @@ -22,18 +20,7 @@ type SqlFileInfoStore struct { metrics einterfaces.MetricsInterface } -const ( - FILE_INFO_CACHE_SIZE = 25000 - FILE_INFO_CACHE_SEC = 1800 // 30 minutes -) - -var fileInfoCache cache.Cache = lru.New(FILE_INFO_CACHE_SIZE) - func (fs SqlFileInfoStore) ClearCaches() { - fileInfoCache.Purge() - if fs.metrics != nil { - fs.metrics.IncrementMemCacheInvalidationCounter("File Info Cache - Purge") - } } func newSqlFileInfoStore(sqlStore SqlStore, metrics einterfaces.MetricsInterface) store.FileInfoStore { @@ -182,35 +169,10 @@ func (fs SqlFileInfoStore) GetByPath(path string) (*model.FileInfo, *model.AppEr return info, nil } -func (fs SqlFileInfoStore) InvalidateFileInfosForPostCache(postId string) { - fileInfoCache.Remove(postId) - if fs.metrics != nil { - fs.metrics.IncrementMemCacheInvalidationCounter("File Info Cache - Remove by PostId") - } +func (fs SqlFileInfoStore) InvalidateFileInfosForPostCache(postId string, deleted bool) { } func (fs SqlFileInfoStore) GetForPost(postId string, readFromMaster, includeDeleted, allowFromCache bool) ([]*model.FileInfo, *model.AppError) { - cacheKey := postId - if includeDeleted { - cacheKey += "_deleted" - } - if allowFromCache { - if cacheItem, ok := fileInfoCache.Get(cacheKey); ok { - if fs.metrics != nil { - fs.metrics.IncrementMemCacheHitCounter("File Info Cache") - } - - return cacheItem.([]*model.FileInfo), nil - } - if fs.metrics != nil { - fs.metrics.IncrementMemCacheMissCounter("File Info Cache") - } - } else { - if fs.metrics != nil { - fs.metrics.IncrementMemCacheMissCounter("File Info Cache") - } - } - var infos []*model.FileInfo dbmap := fs.GetReplica() @@ -238,10 +200,6 @@ func (fs SqlFileInfoStore) GetForPost(postId string, readFromMaster, includeDele return nil, model.NewAppError("SqlFileInfoStore.GetForPost", "store.sql_file_info.get_for_post.app_error", nil, "post_id="+postId+", "+err.Error(), http.StatusInternalServerError) } - if len(infos) > 0 { - fileInfoCache.AddWithExpiresInSecs(cacheKey, infos, FILE_INFO_CACHE_SEC) - } - return infos, nil } diff --git a/store/store.go b/store/store.go index 1a86267e03..29ccdafceb 100644 --- a/store/store.go +++ b/store/store.go @@ -500,7 +500,7 @@ type FileInfoStore interface { GetForPost(postId string, readFromMaster, includeDeleted, allowFromCache bool) ([]*model.FileInfo, *model.AppError) GetForUser(userId string) ([]*model.FileInfo, *model.AppError) GetWithOptions(page, perPage int, opt *model.GetFileInfosOptions) ([]*model.FileInfo, *model.AppError) - InvalidateFileInfosForPostCache(postId string) + InvalidateFileInfosForPostCache(postId string, deleted bool) AttachToPost(fileId string, postId string, creatorId string) *model.AppError DeleteForPost(postId string) (string, *model.AppError) PermanentDelete(fileId string) *model.AppError diff --git a/store/storetest/mocks/FileInfoStore.go b/store/storetest/mocks/FileInfoStore.go index e327aa466c..a93d284e89 100644 --- a/store/storetest/mocks/FileInfoStore.go +++ b/store/storetest/mocks/FileInfoStore.go @@ -183,9 +183,9 @@ func (_m *FileInfoStore) GetWithOptions(page int, perPage int, opt *model.GetFil return r0, r1 } -// InvalidateFileInfosForPostCache provides a mock function with given fields: postId -func (_m *FileInfoStore) InvalidateFileInfosForPostCache(postId string) { - _m.Called(postId) +// InvalidateFileInfosForPostCache provides a mock function with given fields: postId, deleted +func (_m *FileInfoStore) InvalidateFileInfosForPostCache(postId string, deleted bool) { + _m.Called(postId, deleted) } // PermanentDelete provides a mock function with given fields: fileId diff --git a/store/timer_layer.go b/store/timer_layer.go index 92f5ef88aa..620f9479a5 100644 --- a/store/timer_layer.go +++ b/store/timer_layer.go @@ -2479,10 +2479,10 @@ func (s *TimerLayerFileInfoStore) GetWithOptions(page int, perPage int, opt *mod return resultVar0, resultVar1 } -func (s *TimerLayerFileInfoStore) InvalidateFileInfosForPostCache(postId string) { +func (s *TimerLayerFileInfoStore) InvalidateFileInfosForPostCache(postId string, deleted bool) { start := timemodule.Now() - s.FileInfoStore.InvalidateFileInfosForPostCache(postId) + s.FileInfoStore.InvalidateFileInfosForPostCache(postId, deleted) elapsed := float64(timemodule.Since(start)) / float64(timemodule.Second) if s.Root.Metrics != nil {