From 186524bba4eef1a8119e6ff26c76e601b16ba9a5 Mon Sep 17 00:00:00 2001 From: Ali Farooq Date: Thu, 5 Dec 2019 16:15:07 -0500 Subject: [PATCH] Revert "MM-20105: Migrate channelByNameCache cache from store/sqlstore/channel_store.go to the new store/localcachelayer (#13189)" (#13329) Automatic Merge --- 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, 49 insertions(+), 151 deletions(-) diff --git a/store/localcachelayer/channel_layer.go b/store/localcachelayer/channel_layer.go index a82977bf47..4e586a8036 100644 --- a/store/localcachelayer/channel_layer.go +++ b/store/localcachelayer/channel_layer.go @@ -21,14 +21,6 @@ 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() @@ -47,14 +39,12 @@ 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") } } @@ -73,13 +63,6 @@ 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 { @@ -130,63 +113,6 @@ 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 11cfb165ee..35dd08b6b2 100644 --- a/store/localcachelayer/channel_layer_test.go +++ b/store/localcachelayer/channel_layer_test.go @@ -4,7 +4,6 @@ package localcachelayer import ( - "github.com/mattermost/mattermost-server/v5/model" "testing" "github.com/stretchr/testify/assert" @@ -92,70 +91,6 @@ 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 136d698a6d..4975ba18f3 100644 --- a/store/localcachelayer/layer.go +++ b/store/localcachelayer/layer.go @@ -35,8 +35,6 @@ 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 @@ -64,7 +62,6 @@ type LocalCacheStore struct { emojiIdCacheByName *utils.Cache channel LocalCacheChannelStore channelMemberCountsCache *utils.Cache - channelByNameCache *utils.Cache channelGuestCountCache *utils.Cache channelPinnedPostCountsCache *utils.Cache webhook LocalCacheWebhookStore @@ -96,7 +93,6 @@ 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) @@ -115,7 +111,6 @@ 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) @@ -214,7 +209,6 @@ 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 a08c9edff2..513fcff510 100644 --- a/store/localcachelayer/main_test.go +++ b/store/localcachelayer/main_test.go @@ -58,16 +58,11 @@ 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 7b94661250..989f3b5ed9 100644 --- a/store/sqlstore/channel_store.go +++ b/store/sqlstore/channel_store.go @@ -277,16 +277,19 @@ 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") } } @@ -668,6 +671,10 @@ 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) { @@ -1116,6 +1123,29 @@ 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 @@ -1133,9 +1163,14 @@ 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` } - if _, err := s.GetReplica().Select(&channels, query, props); err != nil && err != sql.ErrNoRows { + var dbChannels []*model.Channel + if _, err := s.GetReplica().Select(&dbChannels, 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 @@ -1154,6 +1189,18 @@ 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) @@ -1161,6 +1208,7 @@ 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 }