From 3a8fb53f3edd4a01b8ef113b252dd2b80a29546a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Villablanca=20V=C3=A1squez?= Date: Wed, 20 Nov 2019 10:03:08 -0400 Subject: [PATCH] Migrates the lastPostsCache from PostStore into cache layer (#13141) * Some advances * Partial advances * Cache moved * Tests finished * Removed all test for PostStore (store/sqlstore) related with Cache. This is tested in the cache layer now --- model/cluster_message.go | 1 + store/localcachelayer/layer.go | 13 +++++ store/localcachelayer/layer_test.go | 13 +++++ store/localcachelayer/main_test.go | 8 +++ store/localcachelayer/post_layer.go | 70 ++++++++++++++++++++++++ store/localcachelayer/post_layer_test.go | 65 ++++++++++++++++++++++ store/sqlstore/post_store.go | 32 +---------- store/storetest/post_store.go | 16 ++---- 8 files changed, 175 insertions(+), 43 deletions(-) create mode 100644 store/localcachelayer/post_layer.go create mode 100644 store/localcachelayer/post_layer_test.go diff --git a/model/cluster_message.go b/model/cluster_message.go index 4ce99843ef..6dfea6474e 100644 --- a/model/cluster_message.go +++ b/model/cluster_message.go @@ -27,6 +27,7 @@ const ( 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" CLUSTER_EVENT_INVALIDATE_CACHE_FOR_CHANNEL_MEMBER_COUNTS = "inv_channel_member_counts" + CLUSTER_EVENT_INVALIDATE_CACHE_FOR_LAST_POSTS = "inv_last_posts" CLUSTER_EVENT_CLEAR_SESSION_CACHE_FOR_ALL_USERS = "inv_all_user_sessions" CLUSTER_EVENT_INSTALL_PLUGIN = "install_plugin" CLUSTER_EVENT_REMOVE_PLUGIN = "remove_plugin" diff --git a/store/localcachelayer/layer.go b/store/localcachelayer/layer.go index 83973aedb6..437a3d0773 100644 --- a/store/localcachelayer/layer.go +++ b/store/localcachelayer/layer.go @@ -26,6 +26,9 @@ const ( CHANNEL_MEMBERS_COUNTS_CACHE_SIZE = model.CHANNEL_CACHE_SIZE CHANNEL_MEMBERS_COUNTS_CACHE_SEC = 30 * 60 + LAST_POSTS_CACHE_SIZE = 20000 + LAST_POSTS_CACHE_SEC = 30 * 60 + CLEAR_CACHE_MESSAGE_DATA = "" ) @@ -44,6 +47,8 @@ type LocalCacheStore struct { emojiIdCacheByName *utils.Cache channel LocalCacheChannelStore channelMemberCountsCache *utils.Cache + post LocalCachePostStore + postLastPostsCache *utils.Cache } func NewLocalCacheLayer(baseStore store.Store, metrics einterfaces.MetricsInterface, cluster einterfaces.ClusterInterface) LocalCacheStore { @@ -63,6 +68,8 @@ func NewLocalCacheLayer(baseStore store.Store, metrics einterfaces.MetricsInterf localCacheStore.emoji = LocalCacheEmojiStore{EmojiStore: baseStore.Emoji(), rootStore: &localCacheStore} localCacheStore.channelMemberCountsCache = utils.NewLruWithParams(CHANNEL_MEMBERS_COUNTS_CACHE_SIZE, "ChannelMemberCounts", CHANNEL_MEMBERS_COUNTS_CACHE_SEC, model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_CHANNEL_MEMBER_COUNTS) localCacheStore.channel = LocalCacheChannelStore{ChannelStore: baseStore.Channel(), rootStore: &localCacheStore} + localCacheStore.postLastPostsCache = utils.NewLruWithParams(LAST_POSTS_CACHE_SIZE, "LastPost", LAST_POSTS_CACHE_SEC, model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_LAST_POSTS) + localCacheStore.post = LocalCachePostStore{PostStore: baseStore.Post(), rootStore: &localCacheStore} if cluster != nil { cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_REACTIONS, localCacheStore.reaction.handleClusterInvalidateReaction) @@ -71,6 +78,7 @@ func NewLocalCacheLayer(baseStore store.Store, metrics einterfaces.MetricsInterf cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_EMOJIS_BY_ID, localCacheStore.emoji.handleClusterInvalidateEmojiById) cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_EMOJIS_ID_BY_NAME, localCacheStore.emoji.handleClusterInvalidateEmojiIdByName) cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_CHANNEL_MEMBER_COUNTS, localCacheStore.channel.handleClusterInvalidateChannelMemberCounts) + cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_LAST_POSTS, localCacheStore.post.handleClusterInvalidateLastPosts) } return localCacheStore } @@ -95,6 +103,10 @@ func (s LocalCacheStore) Channel() store.ChannelStore { return s.channel } +func (s LocalCacheStore) Post() store.PostStore { + return s.post +} + func (s LocalCacheStore) DropAllTables() { s.Invalidate() s.Store.DropAllTables() @@ -148,4 +160,5 @@ func (s *LocalCacheStore) Invalidate() { s.doClearCacheCluster(s.emojiCacheById) s.doClearCacheCluster(s.emojiIdCacheByName) s.doClearCacheCluster(s.channelMemberCountsCache) + s.doClearCacheCluster(s.postLastPostsCache) } diff --git a/store/localcachelayer/layer_test.go b/store/localcachelayer/layer_test.go index 6bea6f3ef7..fbee356b7f 100644 --- a/store/localcachelayer/layer_test.go +++ b/store/localcachelayer/layer_test.go @@ -35,6 +35,19 @@ func StoreTest(t *testing.T, f func(*testing.T, store.Store)) { } } +func StoreTestWithSqlSupplier(t *testing.T, f func(*testing.T, store.Store, storetest.SqlSupplier)) { + defer func() { + if err := recover(); err != nil { + tearDownStores() + panic(err) + } + }() + for _, st := range storeTypes { + st := st + t.Run(st.Name, func(t *testing.T) { f(t, st.Store, st.SqlSupplier) }) + } +} + func initStores() { storeTypes = append(storeTypes, &storeType{ Name: "LocalCache+MySQL", diff --git a/store/localcachelayer/main_test.go b/store/localcachelayer/main_test.go index b9ad5b20cf..09ba1f8b6c 100644 --- a/store/localcachelayer/main_test.go +++ b/store/localcachelayer/main_test.go @@ -57,6 +57,14 @@ func getMockStore() *mocks.Store { mockChannelStore.On("GetMemberCount", "id", false).Return(mockCount, nil) mockStore.On("Channel").Return(&mockChannelStore) + fakePosts := &model.PostList{} + fakeOptions := model.GetPostsOptions{ChannelId: "123", PerPage: 30} + mockPostStore := mocks.PostStore{} + mockPostStore.On("GetPosts", fakeOptions, true).Return(fakePosts, nil) + mockPostStore.On("GetPosts", fakeOptions, false).Return(fakePosts, nil) + mockPostStore.On("InvalidateLastPostTimeCache", "12360") + mockStore.On("Post").Return(&mockPostStore) + return &mockStore } diff --git a/store/localcachelayer/post_layer.go b/store/localcachelayer/post_layer.go new file mode 100644 index 0000000000..9c804cd89b --- /dev/null +++ b/store/localcachelayer/post_layer.go @@ -0,0 +1,70 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package localcachelayer + +import ( + "fmt" + "github.com/mattermost/mattermost-server/model" + "github.com/mattermost/mattermost-server/store" +) + +type LocalCachePostStore struct { + store.PostStore + rootStore *LocalCacheStore +} + +func (s *LocalCachePostStore) handleClusterInvalidateLastPosts(msg *model.ClusterMessage) { + if msg.Data == CLEAR_CACHE_MESSAGE_DATA { + s.rootStore.postLastPostsCache.Purge() + } else { + s.rootStore.postLastPostsCache.Remove(msg.Data) + } +} + +func (s LocalCachePostStore) ClearCaches() { + s.PostStore.ClearCaches() + + s.rootStore.postLastPostsCache.Purge() + if s.rootStore.metrics != nil { + s.rootStore.metrics.IncrementMemCacheInvalidationCounter("Last Posts Cache - Purge") + } +} + +func (s LocalCachePostStore) InvalidateLastPostTimeCache(channelId string) { + s.PostStore.InvalidateLastPostTimeCache(channelId) + + // Keys are "{channelid}{limit}" and caching only occurs on limits of 30 and 60 + s.rootStore.doInvalidateCacheCluster(s.rootStore.postLastPostsCache, channelId+"30") + s.rootStore.doInvalidateCacheCluster(s.rootStore.postLastPostsCache, channelId+"60") + + if s.rootStore.metrics != nil { + s.rootStore.metrics.IncrementMemCacheInvalidationCounter("Last Posts Cache - Remove by Channel Id") + } +} + +func (s LocalCachePostStore) GetPosts(options model.GetPostsOptions, allowFromCache bool) (*model.PostList, *model.AppError) { + if !allowFromCache { + return s.PostStore.GetPosts(options, allowFromCache) + } + + offset := options.PerPage * options.Page + // Caching only occurs on limits of 30 and 60, the common limits requested by MM clients + if offset == 0 && (options.PerPage == 60 || options.PerPage == 30) { + if cacheItem := s.rootStore.doStandardReadCache(s.rootStore.postLastPostsCache, fmt.Sprintf("%s%v", options.ChannelId, options.PerPage)); cacheItem != nil { + return cacheItem.(*model.PostList), nil + } + } + + list, err := s.PostStore.GetPosts(options, false) + if err != nil { + return nil, err + } + + // Caching only occurs on limits of 30 and 60, the common limits requested by MM clients + if offset == 0 && (options.PerPage == 60 || options.PerPage == 30) { + s.rootStore.doStandardAddToCache(s.rootStore.postLastPostsCache, fmt.Sprintf("%s%v", options.ChannelId, options.PerPage), list) + } + + return list, err +} diff --git a/store/localcachelayer/post_layer_test.go b/store/localcachelayer/post_layer_test.go new file mode 100644 index 0000000000..6d8e57c2e7 --- /dev/null +++ b/store/localcachelayer/post_layer_test.go @@ -0,0 +1,65 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package localcachelayer + +import ( + "testing" + + "github.com/mattermost/mattermost-server/model" + "github.com/mattermost/mattermost-server/store/storetest" + "github.com/mattermost/mattermost-server/store/storetest/mocks" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPostStore(t *testing.T) { + StoreTestWithSqlSupplier(t, storetest.TestPostStore) +} + +func TestPostStoreCache(t *testing.T) { + fakePosts := &model.PostList{} + fakeOptions := model.GetPostsOptions{ChannelId: "123", PerPage: 30} + + t.Run("first call not cached, second cached and returning same data", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + gotPosts, err := cachedStore.Post().GetPosts(fakeOptions, true) + require.Nil(t, err) + assert.Equal(t, fakePosts, gotPosts) + mockStore.Post().(*mocks.PostStore).AssertNumberOfCalls(t, "GetPosts", 1) + + _, _ = cachedStore.Post().GetPosts(fakeOptions, true) + mockStore.Post().(*mocks.PostStore).AssertNumberOfCalls(t, "GetPosts", 1) + }) + + t.Run("first call not cached, second force no cached", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + gotPosts, err := cachedStore.Post().GetPosts(fakeOptions, true) + require.Nil(t, err) + assert.Equal(t, fakePosts, gotPosts) + mockStore.Post().(*mocks.PostStore).AssertNumberOfCalls(t, "GetPosts", 1) + + _, _ = cachedStore.Post().GetPosts(fakeOptions, false) + mockStore.Post().(*mocks.PostStore).AssertNumberOfCalls(t, "GetPosts", 2) + }) + + t.Run("first call not cached, invalidate, and then not cached again", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + gotPosts, err := cachedStore.Post().GetPosts(fakeOptions, true) + require.Nil(t, err) + assert.Equal(t, fakePosts, gotPosts) + mockStore.Post().(*mocks.PostStore).AssertNumberOfCalls(t, "GetPosts", 1) + + cachedStore.Post().InvalidateLastPostTimeCache("12360") + + _, _ = cachedStore.Post().GetPosts(fakeOptions, true) + mockStore.Post().(*mocks.PostStore).AssertNumberOfCalls(t, "GetPosts", 1) + + }) +} diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index 3560fd61c7..03e69df73d 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -24,7 +24,6 @@ type SqlPostStore struct { SqlStore metrics einterfaces.MetricsInterface lastPostTimeCache *utils.Cache - lastPostsCache *utils.Cache maxPostSizeOnce sync.Once maxPostSizeCached int } @@ -32,18 +31,13 @@ type SqlPostStore struct { const ( LAST_POST_TIME_CACHE_SIZE = 25000 LAST_POST_TIME_CACHE_SEC = 900 // 15 minutes - - LAST_POSTS_CACHE_SIZE = 1000 - LAST_POSTS_CACHE_SEC = 900 // 15 minutes ) func (s *SqlPostStore) ClearCaches() { s.lastPostTimeCache.Purge() - s.lastPostsCache.Purge() if s.metrics != nil { s.metrics.IncrementMemCacheInvalidationCounter("Last Post Time - Purge") - s.metrics.IncrementMemCacheInvalidationCounter("Last Posts Cache - Purge") } } @@ -52,7 +46,6 @@ func NewSqlPostStore(sqlStore SqlStore, metrics einterfaces.MetricsInterface) st SqlStore: sqlStore, metrics: metrics, lastPostTimeCache: utils.NewLru(LAST_POST_TIME_CACHE_SIZE), - lastPostsCache: utils.NewLru(LAST_POSTS_CACHE_SIZE), maxPostSizeCached: model.POST_MESSAGE_MAX_RUNES_V1, } @@ -329,13 +322,8 @@ type etagPosts struct { func (s *SqlPostStore) InvalidateLastPostTimeCache(channelId string) { s.lastPostTimeCache.Remove(channelId) - // Keys are "{channelid}{limit}" and caching only occurs on limits of 30 and 60 - s.lastPostsCache.Remove(channelId + "30") - s.lastPostsCache.Remove(channelId + "60") - if s.metrics != nil { s.metrics.IncrementMemCacheInvalidationCounter("Last Post Time - Remove by Channel Id") - s.metrics.IncrementMemCacheInvalidationCounter("Last Posts Cache - Remove by Channel Id") } } @@ -451,24 +439,11 @@ func (s *SqlPostStore) PermanentDeleteByChannel(channelId string) *model.AppErro return nil } -func (s *SqlPostStore) GetPosts(options model.GetPostsOptions, allowFromCache bool) (*model.PostList, *model.AppError) { +func (s *SqlPostStore) GetPosts(options model.GetPostsOptions, _ bool) (*model.PostList, *model.AppError) { if options.PerPage > 1000 { return nil, model.NewAppError("SqlPostStore.GetLinearPosts", "store.sql_post.get_posts.app_error", nil, "channelId="+options.ChannelId, http.StatusBadRequest) } offset := options.PerPage * options.Page - // Caching only occurs on limits of 30 and 60, the common limits requested by MM clients - if allowFromCache && offset == 0 && (options.PerPage == 60 || options.PerPage == 30) { - if cacheItem, ok := s.lastPostsCache.Get(fmt.Sprintf("%s%v", options.ChannelId, options.PerPage)); ok { - if s.metrics != nil { - s.metrics.IncrementMemCacheHitCounter("Last Posts Cache") - } - return cacheItem.(*model.PostList), nil - } - } - - if s.metrics != nil { - s.metrics.IncrementMemCacheMissCounter("Last Posts Cache") - } rpc := make(chan store.StoreResult, 1) go func() { @@ -510,11 +485,6 @@ func (s *SqlPostStore) GetPosts(options model.GetPostsOptions, allowFromCache bo list.MakeNonNil() - // Caching only occurs on limits of 30 and 60, the common limits requested by MM clients - if offset == 0 && (options.PerPage == 60 || options.PerPage == 30) { - s.lastPostsCache.AddWithExpiresInSecs(fmt.Sprintf("%s%v", options.ChannelId, options.PerPage), list, LAST_POSTS_CACHE_SEC) - } - return list, err } diff --git a/store/storetest/post_store.go b/store/storetest/post_store.go index 21162536e6..f197bf15c2 100644 --- a/store/storetest/post_store.go +++ b/store/storetest/post_store.go @@ -697,7 +697,7 @@ func testPostStoreGetPostsWithDetails(t *testing.T, ss store.Store) { t.Fatal("Missing parent") } - r2, err := ss.Post().GetPosts(model.GetPostsOptions{ChannelId: o1.ChannelId, Page: 0, PerPage: 4}, true) + r2, err := ss.Post().GetPosts(model.GetPostsOptions{ChannelId: o1.ChannelId, Page: 0, PerPage: 4}, false) require.Nil(t, err) if r2.Order[0] != o5.Id { @@ -725,7 +725,7 @@ func testPostStoreGetPostsWithDetails(t *testing.T, ss store.Store) { } // Run once to fill cache - _, err = ss.Post().GetPosts(model.GetPostsOptions{ChannelId: o1.ChannelId, Page: 0, PerPage: 30}, true) + _, err = ss.Post().GetPosts(model.GetPostsOptions{ChannelId: o1.ChannelId, Page: 0, PerPage: 30}, false) require.Nil(t, err) o6 := &model.Post{} @@ -735,17 +735,9 @@ func testPostStoreGetPostsWithDetails(t *testing.T, ss store.Store) { _, err = ss.Post().Save(o6) require.Nil(t, err) - // Should only be 6 since we hit the cache - r3, err := ss.Post().GetPosts(model.GetPostsOptions{ChannelId: o1.ChannelId, Page: 0, PerPage: 30}, true) + r3, err := ss.Post().GetPosts(model.GetPostsOptions{ChannelId: o1.ChannelId, Page: 0, PerPage: 30}, false) require.Nil(t, err) - assert.Equal(t, 6, len(r3.Order)) - - ss.Post().InvalidateLastPostTimeCache(o1.ChannelId) - - // Cache was invalidated, we should get all the posts - r4, err := ss.Post().GetPosts(model.GetPostsOptions{ChannelId: o1.ChannelId, Page: 0, PerPage: 30}, true) - require.Nil(t, err) - assert.Equal(t, 7, len(r4.Order)) + assert.Equal(t, 7, len(r3.Order)) } func testPostStoreGetPostsBeforeAfter(t *testing.T, ss store.Store) {