From 223db0c05c1dc4000aaa94ede3d6289727c31566 Mon Sep 17 00:00:00 2001 From: Allen Lai Date: Tue, 3 Dec 2019 21:55:02 -0800 Subject: [PATCH] MM-20105: Migrate channelByNameCache cache from store/sqlstore/channel_store.go to the new store/localcachelayer (#13189) * migrate * add unit test * change syntax * delete comments * remove includedeleted search method * remove include search method in correct file * change constant * add Mock.On * change to &channel * fix type for Mock.on().return() * remove inc counter * suggestions takened * get rid of unneeded concat * go fmt * import v5 --- store/localcachelayer/channel_layer.go | 74 +++++++++++++++++++++ store/localcachelayer/channel_layer_test.go | 65 ++++++++++++++++++ store/localcachelayer/layer.go | 6 ++ store/localcachelayer/main_test.go | 5 ++ store/sqlstore/channel_store.go | 50 +------------- 5 files changed, 151 insertions(+), 49 deletions(-) diff --git a/store/localcachelayer/channel_layer.go b/store/localcachelayer/channel_layer.go index 4e586a8036..a82977bf47 100644 --- a/store/localcachelayer/channel_layer.go +++ b/store/localcachelayer/channel_layer.go @@ -21,6 +21,14 @@ func (s *LocalCacheChannelStore) handleClusterInvalidateChannelMemberCounts(msg } } +func (s *LocalCacheChannelStore) handleClusterInvalidateChannelByName(msg *model.ClusterMessage) { + if msg.Data == CLEAR_CACHE_MESSAGE_DATA { + s.rootStore.channelByNameCache.Purge() + } else { + s.rootStore.channelByNameCache.Remove(msg.Data) + } +} + func (s *LocalCacheChannelStore) handleClusterInvalidateChannelPinnedPostCount(msg *model.ClusterMessage) { if msg.Data == CLEAR_CACHE_MESSAGE_DATA { s.rootStore.channelPinnedPostCountsCache.Purge() @@ -39,12 +47,14 @@ func (s *LocalCacheChannelStore) handleClusterInvalidateChannelGuestCounts(msg * func (s LocalCacheChannelStore) ClearCaches() { s.rootStore.doClearCacheCluster(s.rootStore.channelMemberCountsCache) + s.rootStore.doClearCacheCluster(s.rootStore.channelByNameCache) s.rootStore.doClearCacheCluster(s.rootStore.channelPinnedPostCountsCache) s.rootStore.doClearCacheCluster(s.rootStore.channelGuestCountCache) s.ChannelStore.ClearCaches() if s.rootStore.metrics != nil { s.rootStore.metrics.IncrementMemCacheInvalidationCounter("Channel Pinned Post Counts - Purge") s.rootStore.metrics.IncrementMemCacheInvalidationCounter("Channel Member Counts - Purge") + s.rootStore.metrics.IncrementMemCacheInvalidationCounter("Channel By Name - Purge") s.rootStore.metrics.IncrementMemCacheInvalidationCounter("Channel Guest Count - Purge") } } @@ -63,6 +73,13 @@ func (s LocalCacheChannelStore) InvalidateMemberCount(channelId string) { } } +func (s LocalCacheChannelStore) InvalidateChannelByName(teamId, name string) { + s.rootStore.doInvalidateCacheCluster(s.rootStore.channelByNameCache, teamId+name) + if s.rootStore.metrics != nil { + s.rootStore.metrics.IncrementMemCacheInvalidationCounter("Channel by Name - Remove by TeamId and Name") + } +} + func (s LocalCacheChannelStore) InvalidateGuestCount(channelId string) { s.rootStore.doInvalidateCacheCluster(s.rootStore.channelGuestCountCache, channelId) if s.rootStore.metrics != nil { @@ -113,6 +130,63 @@ func (s LocalCacheChannelStore) GetMemberCountFromCache(channelId string) int64 return count } +// ChannelCacheByName methods +func (s LocalCacheChannelStore) GetByName(teamId string, name string, allowFromCache bool) (*model.Channel, *model.AppError) { + return s.getByName(teamId, name, false, allowFromCache) +} + +func (s LocalCacheChannelStore) GetByNames(teamId string, names []string, allowFromCache bool) ([]*model.Channel, *model.AppError) { + var channels []*model.Channel + + if allowFromCache { + var misses []string + visited := make(map[string]struct{}) + for _, name := range names { + if _, ok := visited[name]; ok { + continue + } + visited[name] = struct{}{} + if cacheItem := s.rootStore.doStandardReadCache(s.rootStore.channelByNameCache, teamId+name); cacheItem != nil { + channels = append(channels, cacheItem.(*model.Channel)) + } else { + misses = append(misses, name) + } + } + names = misses + } + + if len(names) > 0 { + dbChannels, err := s.ChannelStore.GetByNames(teamId, names, allowFromCache) + + if err != nil { + return nil, err + } + + for _, channel := range dbChannels { + s.rootStore.doStandardAddToCache(s.rootStore.channelByNameCache, teamId+channel.Name, channel) + channels = append(channels, channel) // add missing channels to the ones just found + } + } + + return channels, nil +} + +func (s LocalCacheChannelStore) getByName(teamId string, name string, includeDeleted bool, allowFromCache bool) (*model.Channel, *model.AppError) { + if allowFromCache { + if cacheItem := s.rootStore.doStandardReadCache(s.rootStore.channelByNameCache, teamId+name); cacheItem != nil { + return cacheItem.(*model.Channel), nil + } + } + + channel, err := s.ChannelStore.GetByName(teamId, name, allowFromCache) + + if allowFromCache && err == nil { + s.rootStore.doStandardAddToCache(s.rootStore.channelByNameCache, teamId+name, channel) + } + + return channel, err +} + func (s LocalCacheChannelStore) GetPinnedPostCount(channelId string, allowFromCache bool) (int64, *model.AppError) { if allowFromCache { if count := s.rootStore.doStandardReadCache(s.rootStore.channelPinnedPostCountsCache, channelId); count != nil { diff --git a/store/localcachelayer/channel_layer_test.go b/store/localcachelayer/channel_layer_test.go index 35dd08b6b2..11cfb165ee 100644 --- a/store/localcachelayer/channel_layer_test.go +++ b/store/localcachelayer/channel_layer_test.go @@ -4,6 +4,7 @@ package localcachelayer import ( + "github.com/mattermost/mattermost-server/v5/model" "testing" "github.com/stretchr/testify/assert" @@ -91,6 +92,70 @@ func TestChannelStoreChannelMemberCountsCache(t *testing.T) { }) } +func TestChannelStoreChannelByNameCache(t *testing.T) { + teamIdString := "teamID123" + nameString := "nameId987" + fakeChannel := model.Channel{Name: nameString, TeamId: teamIdString} + + t.Run("first call by name not cached, second cached and returning same data", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + channel, err := cachedStore.Channel().GetByName(teamIdString, nameString, true) + require.Nil(t, err) + assert.Equal(t, channel, &fakeChannel) + mockStore.Channel().(*mocks.ChannelStore).AssertNumberOfCalls(t, "GetByName", 1) + channel, err = cachedStore.Channel().GetByName(teamIdString, nameString, true) + require.Nil(t, err) + assert.Equal(t, channel, &fakeChannel) + mockStore.Channel().(*mocks.ChannelStore).AssertNumberOfCalls(t, "GetByName", 1) + }) + + t.Run("first call by name not cached, second force no cached", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + cachedStore.Channel().GetByName(teamIdString, nameString, true) + mockStore.Channel().(*mocks.ChannelStore).AssertNumberOfCalls(t, "GetByName", 1) + cachedStore.Channel().GetByName(teamIdString, nameString, false) + mockStore.Channel().(*mocks.ChannelStore).AssertNumberOfCalls(t, "GetByName", 2) + }) + + t.Run("first call force no cached, second not cached, third cached", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + cachedStore.Channel().GetByName(teamIdString, nameString, false) + mockStore.Channel().(*mocks.ChannelStore).AssertNumberOfCalls(t, "GetByName", 1) + cachedStore.Channel().GetByName(teamIdString, nameString, true) + mockStore.Channel().(*mocks.ChannelStore).AssertNumberOfCalls(t, "GetByName", 2) + cachedStore.Channel().GetByName(teamIdString, nameString, true) + mockStore.Channel().(*mocks.ChannelStore).AssertNumberOfCalls(t, "GetByName", 2) + }) + + t.Run("first call not cached, clear cache, second call not cached", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + cachedStore.Channel().GetByName(teamIdString, nameString, true) + mockStore.Channel().(*mocks.ChannelStore).AssertNumberOfCalls(t, "GetByName", 1) + cachedStore.Channel().ClearCaches() + cachedStore.Channel().GetByName(teamIdString, nameString, true) + mockStore.Channel().(*mocks.ChannelStore).AssertNumberOfCalls(t, "GetByName", 2) + }) + + t.Run("first call not cached, invalidate cache, second call not cached", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + cachedStore.Channel().GetByName(teamIdString, nameString, true) + mockStore.Channel().(*mocks.ChannelStore).AssertNumberOfCalls(t, "GetByName", 1) + cachedStore.Channel().InvalidateChannelByName(teamIdString, nameString) + cachedStore.Channel().GetByName(teamIdString, nameString, true) + mockStore.Channel().(*mocks.ChannelStore).AssertNumberOfCalls(t, "GetByName", 2) + }) +} + func TestChannelStoreChannelPinnedPostsCountsCache(t *testing.T) { countResult := int64(10) diff --git a/store/localcachelayer/layer.go b/store/localcachelayer/layer.go index 4975ba18f3..136d698a6d 100644 --- a/store/localcachelayer/layer.go +++ b/store/localcachelayer/layer.go @@ -35,6 +35,8 @@ const ( CHANNEL_MEMBERS_COUNTS_CACHE_SIZE = model.CHANNEL_CACHE_SIZE CHANNEL_MEMBERS_COUNTS_CACHE_SEC = 30 * 60 + CHANNEL_CACHE_SEC = 900 // 15 mins + LAST_POSTS_CACHE_SIZE = 20000 LAST_POSTS_CACHE_SEC = 30 * 60 @@ -62,6 +64,7 @@ type LocalCacheStore struct { emojiIdCacheByName *utils.Cache channel LocalCacheChannelStore channelMemberCountsCache *utils.Cache + channelByNameCache *utils.Cache channelGuestCountCache *utils.Cache channelPinnedPostCountsCache *utils.Cache webhook LocalCacheWebhookStore @@ -93,6 +96,7 @@ func NewLocalCacheLayer(baseStore store.Store, metrics einterfaces.MetricsInterf localCacheStore.emoji = LocalCacheEmojiStore{EmojiStore: baseStore.Emoji(), rootStore: &localCacheStore} localCacheStore.channelPinnedPostCountsCache = utils.NewLruWithParams(CHANNEL_PINNEDPOSTS_COUNTS_CACHE_SIZE, "ChannelPinnedPostsCounts", CHANNEL_PINNEDPOSTS_COUNTS_CACHE_SEC, model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_CHANNEL_PINNEDPOSTS_COUNTS) 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.channelByNameCache = utils.NewLruWithParams(CHANNEL_CACHE_SEC, "ChannelByName", CHANNEL_CACHE_SEC, model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_CHANNEL_BY_NAME) localCacheStore.channelGuestCountCache = utils.NewLruWithParams(CHANNEL_GUEST_COUNT_CACHE_SIZE, "ChannelGuestsCount", CHANNEL_GUEST_COUNT_CACHE_SEC, model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_CHANNEL_GUEST_COUNT) 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) @@ -111,6 +115,7 @@ func NewLocalCacheLayer(baseStore store.Store, metrics einterfaces.MetricsInterf cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_EMOJIS_ID_BY_NAME, localCacheStore.emoji.handleClusterInvalidateEmojiIdByName) cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_CHANNEL_PINNEDPOSTS_COUNTS, localCacheStore.channel.handleClusterInvalidateChannelPinnedPostCount) cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_CHANNEL_MEMBER_COUNTS, localCacheStore.channel.handleClusterInvalidateChannelMemberCounts) + cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_CHANNEL_BY_NAME, localCacheStore.channel.handleClusterInvalidateChannelByName) cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_CHANNEL_GUEST_COUNT, localCacheStore.channel.handleClusterInvalidateChannelGuestCounts) cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_LAST_POSTS, localCacheStore.post.handleClusterInvalidateLastPosts) cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_PROFILE_BY_IDS, localCacheStore.user.handleClusterInvalidateScheme) @@ -209,6 +214,7 @@ func (s *LocalCacheStore) Invalidate() { s.doClearCacheCluster(s.emojiCacheById) s.doClearCacheCluster(s.emojiIdCacheByName) s.doClearCacheCluster(s.channelMemberCountsCache) + s.doClearCacheCluster(s.channelByNameCache) s.doClearCacheCluster(s.channelPinnedPostCountsCache) s.doClearCacheCluster(s.channelGuestCountCache) s.doClearCacheCluster(s.postLastPostsCache) diff --git a/store/localcachelayer/main_test.go b/store/localcachelayer/main_test.go index 513fcff510..a08c9edff2 100644 --- a/store/localcachelayer/main_test.go +++ b/store/localcachelayer/main_test.go @@ -58,11 +58,16 @@ func getMockStore() *mocks.Store { mockStore.On("Emoji").Return(&mockEmojiStore) mockCount := int64(10) + teamIdString := "teamID123" + nameString := "nameId987" + fakeChannel := model.Channel{Name: nameString, TeamId: teamIdString} mockGuestCount := int64(12) mockChannelStore := mocks.ChannelStore{} mockChannelStore.On("ClearCaches").Return() mockChannelStore.On("GetMemberCount", "id", true).Return(mockCount, nil) mockChannelStore.On("GetMemberCount", "id", false).Return(mockCount, nil) + mockChannelStore.On("GetByName", teamIdString, nameString, true).Return(&fakeChannel, nil) + mockChannelStore.On("GetByName", teamIdString, nameString, false).Return(&fakeChannel, nil) mockChannelStore.On("GetGuestCount", "id", true).Return(mockGuestCount, nil) mockChannelStore.On("GetGuestCount", "id", false).Return(mockGuestCount, nil) mockStore.On("Channel").Return(&mockChannelStore) diff --git a/store/sqlstore/channel_store.go b/store/sqlstore/channel_store.go index 989f3b5ed9..7b94661250 100644 --- a/store/sqlstore/channel_store.go +++ b/store/sqlstore/channel_store.go @@ -277,19 +277,16 @@ type publicChannel struct { var allChannelMembersForUserCache = utils.NewLru(ALL_CHANNEL_MEMBERS_FOR_USER_CACHE_SIZE) var allChannelMembersNotifyPropsForChannelCache = utils.NewLru(ALL_CHANNEL_MEMBERS_NOTIFY_PROPS_FOR_CHANNEL_CACHE_SIZE) var channelCache = utils.NewLru(model.CHANNEL_CACHE_SIZE) -var channelByNameCache = utils.NewLru(model.CHANNEL_CACHE_SIZE) func (s SqlChannelStore) ClearCaches() { allChannelMembersForUserCache.Purge() allChannelMembersNotifyPropsForChannelCache.Purge() channelCache.Purge() - channelByNameCache.Purge() if s.metrics != nil { s.metrics.IncrementMemCacheInvalidationCounter("All Channel Members for User - Purge") s.metrics.IncrementMemCacheInvalidationCounter("All Channel Members Notify Props for Channel - Purge") s.metrics.IncrementMemCacheInvalidationCounter("Channel - Purge") - s.metrics.IncrementMemCacheInvalidationCounter("Channel By Name - Purge") } } @@ -671,10 +668,6 @@ func (s SqlChannelStore) InvalidateChannel(id string) { } func (s SqlChannelStore) InvalidateChannelByName(teamId, name string) { - channelByNameCache.Remove(teamId + name) - if s.metrics != nil { - s.metrics.IncrementMemCacheInvalidationCounter("Channel by Name - Remove by TeamId and Name") - } } func (s SqlChannelStore) Get(id string, allowFromCache bool) (*model.Channel, *model.AppError) { @@ -1123,29 +1116,6 @@ func (s SqlChannelStore) GetByName(teamId string, name string, allowFromCache bo func (s SqlChannelStore) GetByNames(teamId string, names []string, allowFromCache bool) ([]*model.Channel, *model.AppError) { var channels []*model.Channel - if allowFromCache { - var misses []string - visited := make(map[string]struct{}) - for _, name := range names { - if _, ok := visited[name]; ok { - continue - } - visited[name] = struct{}{} - if cacheItem, ok := channelByNameCache.Get(teamId + name); ok { - if s.metrics != nil { - s.metrics.IncrementMemCacheHitCounter("Channel By Name") - } - channels = append(channels, cacheItem.(*model.Channel)) - } else { - if s.metrics != nil { - s.metrics.IncrementMemCacheMissCounter("Channel By Name") - } - misses = append(misses, name) - } - } - names = misses - } - if len(names) > 0 { props := map[string]interface{}{} var namePlaceholders []string @@ -1163,14 +1133,9 @@ func (s SqlChannelStore) GetByNames(teamId string, names []string, allowFromCach query = `SELECT * FROM Channels WHERE Name IN (` + strings.Join(namePlaceholders, ", ") + `) AND TeamId = :TeamId AND DeleteAt = 0` } - var dbChannels []*model.Channel - if _, err := s.GetReplica().Select(&dbChannels, query, props); err != nil && err != sql.ErrNoRows { + if _, err := s.GetReplica().Select(&channels, query, props); err != nil && err != sql.ErrNoRows { return nil, model.NewAppError("SqlChannelStore.GetByName", "store.sql_channel.get_by_name.existing.app_error", nil, "teamId="+teamId+", "+err.Error(), http.StatusInternalServerError) } - for _, channel := range dbChannels { - channelByNameCache.AddWithExpiresInSecs(teamId+channel.Name, channel, CHANNEL_CACHE_SEC) - channels = append(channels, channel) - } } return channels, nil @@ -1189,18 +1154,6 @@ func (s SqlChannelStore) getByName(teamId string, name string, includeDeleted bo } channel := model.Channel{} - if allowFromCache { - if cacheItem, ok := channelByNameCache.Get(teamId + name); ok { - if s.metrics != nil { - s.metrics.IncrementMemCacheHitCounter("Channel By Name") - } - return cacheItem.(*model.Channel), nil - } - if s.metrics != nil { - s.metrics.IncrementMemCacheMissCounter("Channel By Name") - } - } - if err := s.GetReplica().SelectOne(&channel, query, map[string]interface{}{"TeamId": teamId, "Name": name}); err != nil { if err == sql.ErrNoRows { return nil, model.NewAppError("SqlChannelStore.GetByName", store.MISSING_CHANNEL_ERROR, nil, "teamId="+teamId+", "+"name="+name+", "+err.Error(), http.StatusNotFound) @@ -1208,7 +1161,6 @@ func (s SqlChannelStore) getByName(teamId string, name string, includeDeleted bo return nil, model.NewAppError("SqlChannelStore.GetByName", "store.sql_channel.get_by_name.existing.app_error", nil, "teamId="+teamId+", "+"name="+name+", "+err.Error(), http.StatusInternalServerError) } - channelByNameCache.AddWithExpiresInSecs(teamId+name, &channel, CHANNEL_CACHE_SEC) return &channel, nil }