From e03411795c50b90d169cf3bf12a4fdfc0dae9503 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Villablanca=20V=C3=A1squez?= Date: Fri, 29 Nov 2019 07:56:50 -0300 Subject: [PATCH] Migrating team store cache to cache layer (#13075) Automatic Merge --- model/cluster_message.go | 1 + store/localcachelayer/layer.go | 13 ++++ store/localcachelayer/main_test.go | 6 ++ store/localcachelayer/team_layer.go | 79 ++++++++++++++++++++++++ store/localcachelayer/team_layer_test.go | 70 +++++++++++++++++++++ store/sqlstore/supplier.go | 2 +- store/sqlstore/team_store.go | 49 ++------------- 7 files changed, 175 insertions(+), 45 deletions(-) create mode 100644 store/localcachelayer/team_layer.go create mode 100644 store/localcachelayer/team_layer_test.go diff --git a/model/cluster_message.go b/model/cluster_message.go index ce093216a9ce..a3c169b34444 100644 --- a/model/cluster_message.go +++ b/model/cluster_message.go @@ -32,6 +32,7 @@ const ( CLUSTER_EVENT_INVALIDATE_CACHE_FOR_CHANNEL_PINNEDPOSTS_COUNTS = "inv_channel_pinnedposts_counts" CLUSTER_EVENT_INVALIDATE_CACHE_FOR_CHANNEL_MEMBER_COUNTS = "inv_channel_member_counts" CLUSTER_EVENT_INVALIDATE_CACHE_FOR_LAST_POSTS = "inv_last_posts" + CLUSTER_EVENT_INVALIDATE_CACHE_FOR_TEAMS = "inv_teams" 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 73de0b5e2519..93a5b34e848c 100644 --- a/store/localcachelayer/layer.go +++ b/store/localcachelayer/layer.go @@ -41,6 +41,9 @@ const ( USER_PROFILE_BY_ID_CACHE_SIZE = 20000 USER_PROFILE_BY_ID_SEC = 30 * 60 + TEAM_CACHE_SIZE = 20000 + TEAM_CACHE_SEC = 30 * 60 + CLEAR_CACHE_MESSAGE_DATA = "" ) @@ -67,6 +70,8 @@ type LocalCacheStore struct { postLastPostsCache *utils.Cache user LocalCacheUserStore userProfileByIdsCache *utils.Cache + team LocalCacheTeamStore + teamAllTeamIdsForUserCache *utils.Cache } func NewLocalCacheLayer(baseStore store.Store, metrics einterfaces.MetricsInterface, cluster einterfaces.ClusterInterface) LocalCacheStore { @@ -94,6 +99,8 @@ func NewLocalCacheLayer(baseStore store.Store, metrics einterfaces.MetricsInterf localCacheStore.post = LocalCachePostStore{PostStore: baseStore.Post(), rootStore: &localCacheStore} localCacheStore.userProfileByIdsCache = utils.NewLruWithParams(USER_PROFILE_BY_ID_CACHE_SIZE, "UserProfileByIds", USER_PROFILE_BY_ID_SEC, model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_PROFILE_BY_IDS) localCacheStore.user = LocalCacheUserStore{UserStore: baseStore.User(), rootStore: &localCacheStore} + localCacheStore.teamAllTeamIdsForUserCache = utils.NewLruWithParams(TEAM_CACHE_SIZE, "Team", TEAM_CACHE_SEC, model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_TEAMS) + localCacheStore.team = LocalCacheTeamStore{TeamStore: baseStore.Team(), rootStore: &localCacheStore} if cluster != nil { cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_REACTIONS, localCacheStore.reaction.handleClusterInvalidateReaction) @@ -107,6 +114,7 @@ func NewLocalCacheLayer(baseStore store.Store, metrics einterfaces.MetricsInterf 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) + cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_TEAMS, localCacheStore.team.handleClusterInvalidateTeam) } return localCacheStore } @@ -143,6 +151,10 @@ func (s LocalCacheStore) User() store.UserStore { return s.user } +func (s LocalCacheStore) Team() store.TeamStore { + return s.team +} + func (s LocalCacheStore) DropAllTables() { s.Invalidate() s.Store.DropAllTables() @@ -201,4 +213,5 @@ func (s *LocalCacheStore) Invalidate() { s.doClearCacheCluster(s.channelGuestCountCache) s.doClearCacheCluster(s.postLastPostsCache) s.doClearCacheCluster(s.userProfileByIdsCache) + s.doClearCacheCluster(s.teamAllTeamIdsForUserCache) } diff --git a/store/localcachelayer/main_test.go b/store/localcachelayer/main_test.go index 395b80b140f9..3bfc1c37319d 100644 --- a/store/localcachelayer/main_test.go +++ b/store/localcachelayer/main_test.go @@ -85,6 +85,12 @@ func getMockStore() *mocks.Store { mockUserStore.On("GetProfileByIds", []string{"123"}, &store.UserGetByIdsOpts{}, false).Return(fakeUser, nil) mockStore.On("User").Return(&mockUserStore) + fakeUserTeamIds := []string{"1", "2", "3"} + mockTeamStore := mocks.TeamStore{} + mockTeamStore.On("GetUserTeamIds", "123", true).Return(fakeUserTeamIds, nil) + mockTeamStore.On("GetUserTeamIds", "123", false).Return(fakeUserTeamIds, nil) + mockStore.On("Team").Return(&mockTeamStore) + return &mockStore } diff --git a/store/localcachelayer/team_layer.go b/store/localcachelayer/team_layer.go new file mode 100644 index 000000000000..e41a35989cf0 --- /dev/null +++ b/store/localcachelayer/team_layer.go @@ -0,0 +1,79 @@ +// Copyright (c) 2017-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 LocalCacheTeamStore struct { + store.TeamStore + rootStore *LocalCacheStore +} + +func (s *LocalCacheTeamStore) handleClusterInvalidateTeam(msg *model.ClusterMessage) { + if msg.Data == CLEAR_CACHE_MESSAGE_DATA { + s.rootStore.teamAllTeamIdsForUserCache.Purge() + } else { + s.rootStore.teamAllTeamIdsForUserCache.Remove(msg.Data) + } +} + +func (s LocalCacheTeamStore) ClearCaches() { + s.rootStore.teamAllTeamIdsForUserCache.Purge() + if s.rootStore.metrics != nil { + s.rootStore.metrics.IncrementMemCacheInvalidationCounter("All Team Ids for User - Purge") + } +} + +func (s LocalCacheTeamStore) InvalidateAllTeamIdsForUser(userId string) { + s.rootStore.doInvalidateCacheCluster(s.rootStore.teamAllTeamIdsForUserCache, userId) + if s.rootStore.metrics != nil { + s.rootStore.metrics.IncrementMemCacheInvalidationCounter("All Team Ids for User - Remove by UserId") + } +} + +func (s LocalCacheTeamStore) GetUserTeamIds(userID string, allowFromCache bool) ([]string, *model.AppError) { + if !allowFromCache { + return s.TeamStore.GetUserTeamIds(userID, allowFromCache) + } + + if userTeamIds := s.rootStore.doStandardReadCache(s.rootStore.teamAllTeamIdsForUserCache, userID); userTeamIds != nil { + return userTeamIds.([]string), nil + } + + userTeamIds, err := s.TeamStore.GetUserTeamIds(userID, allowFromCache) + if err != nil { + return nil, err + } + + if len(userTeamIds) > 0 { + s.rootStore.doStandardAddToCache(s.rootStore.teamAllTeamIdsForUserCache, userID, userTeamIds) + } + + return userTeamIds, nil +} + +func (s LocalCacheTeamStore) Update(team *model.Team) (*model.Team, *model.AppError) { + var oldTeam *model.Team + var err *model.AppError + if team.DeleteAt != 0 { + oldTeam, err = s.TeamStore.Get(team.Id) + if err != nil { + return nil, err + } + } + + tm, err := s.TeamStore.Update(team) + if err != nil { + return nil, err + } + + if oldTeam != nil && oldTeam.DeleteAt == 0 { + s.rootStore.doClearCacheCluster(s.rootStore.teamAllTeamIdsForUserCache) + } + + return tm, err +} diff --git a/store/localcachelayer/team_layer_test.go b/store/localcachelayer/team_layer_test.go new file mode 100644 index 000000000000..5ccd4ad4c7d1 --- /dev/null +++ b/store/localcachelayer/team_layer_test.go @@ -0,0 +1,70 @@ +// Copyright (c) 2017-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package localcachelayer + +import ( + "testing" + + "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 TestTeamStore(t *testing.T) { + StoreTest(t, storetest.TestTeamStore) +} + +func TestTeamStoreCache(t *testing.T) { + fakeUserId := "123" + fakeUserTeamIds := []string{"1", "2", "3"} + + t.Run("first call not cached, second cached and returning same data", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + gotUserTeamIds, err := cachedStore.Team().GetUserTeamIds(fakeUserId, true) + require.Nil(t, err) + assert.Equal(t, fakeUserTeamIds, gotUserTeamIds) + mockStore.Team().(*mocks.TeamStore).AssertNumberOfCalls(t, "GetUserTeamIds", 1) + + gotUserTeamIds, err = cachedStore.Team().GetUserTeamIds(fakeUserId, true) + require.Nil(t, err) + assert.Equal(t, fakeUserTeamIds, gotUserTeamIds) + mockStore.Team().(*mocks.TeamStore).AssertNumberOfCalls(t, "GetUserTeamIds", 1) + }) + + t.Run("first call not cached, second force no cached", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + gotUserTeamIds, err := cachedStore.Team().GetUserTeamIds(fakeUserId, true) + require.Nil(t, err) + assert.Equal(t, fakeUserTeamIds, gotUserTeamIds) + mockStore.Team().(*mocks.TeamStore).AssertNumberOfCalls(t, "GetUserTeamIds", 1) + + gotUserTeamIds, err = cachedStore.Team().GetUserTeamIds(fakeUserId, false) + require.Nil(t, err) + assert.Equal(t, fakeUserTeamIds, gotUserTeamIds) + mockStore.Team().(*mocks.TeamStore).AssertNumberOfCalls(t, "GetUserTeamIds", 2) + }) + + t.Run("first call not cached, invalidate, and then not cached again", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + gotUserTeamIds, err := cachedStore.Team().GetUserTeamIds(fakeUserId, true) + require.Nil(t, err) + assert.Equal(t, fakeUserTeamIds, gotUserTeamIds) + mockStore.Team().(*mocks.TeamStore).AssertNumberOfCalls(t, "GetUserTeamIds", 1) + + cachedStore.Team().InvalidateAllTeamIdsForUser(fakeUserId) + + gotUserTeamIds, err = cachedStore.Team().GetUserTeamIds(fakeUserId, true) + require.Nil(t, err) + assert.Equal(t, fakeUserTeamIds, gotUserTeamIds) + mockStore.Team().(*mocks.TeamStore).AssertNumberOfCalls(t, "GetUserTeamIds", 2) + }) + +} diff --git a/store/sqlstore/supplier.go b/store/sqlstore/supplier.go index 93e3679526f8..813f25a9095d 100644 --- a/store/sqlstore/supplier.go +++ b/store/sqlstore/supplier.go @@ -123,7 +123,7 @@ func NewSqlSupplier(settings model.SqlSettings, metrics einterfaces.MetricsInter supplier.initConnection() - supplier.stores.team = NewSqlTeamStore(supplier, metrics) + supplier.stores.team = NewSqlTeamStore(supplier) supplier.stores.channel = NewSqlChannelStore(supplier, metrics) supplier.stores.post = NewSqlPostStore(supplier, metrics) supplier.stores.user = NewSqlUserStore(supplier, metrics) diff --git a/store/sqlstore/team_store.go b/store/sqlstore/team_store.go index e06d0c8a9b52..81188d8c7da7 100644 --- a/store/sqlstore/team_store.go +++ b/store/sqlstore/team_store.go @@ -11,21 +11,16 @@ import ( sq "github.com/Masterminds/squirrel" "github.com/mattermost/gorp" - "github.com/mattermost/mattermost-server/v5/einterfaces" "github.com/mattermost/mattermost-server/v5/model" "github.com/mattermost/mattermost-server/v5/store" - "github.com/mattermost/mattermost-server/v5/utils" ) const ( - TEAM_MEMBER_EXISTS_ERROR = "store.sql_team.save_member.exists.app_error" - ALL_TEAM_IDS_FOR_USER_CACHE_SIZE = model.SESSION_CACHE_SIZE - ALL_TEAM_IDS_FOR_USER_CACHE_SEC = 1800 // 30 mins + TEAM_MEMBER_EXISTS_ERROR = "store.sql_team.save_member.exists.app_error" ) type SqlTeamStore struct { SqlStore - metrics einterfaces.MetricsInterface } type teamMember struct { @@ -155,10 +150,9 @@ func (db teamMemberWithSchemeRolesList) ToModel() []*model.TeamMember { return tms } -func NewSqlTeamStore(sqlStore SqlStore, metrics einterfaces.MetricsInterface) store.TeamStore { +func NewSqlTeamStore(sqlStore SqlStore) store.TeamStore { s := &SqlTeamStore{ sqlStore, - metrics, } for _, db := range sqlStore.GetAllConns() { @@ -245,11 +239,6 @@ func (s SqlTeamStore) Update(team *model.Team) (*model.Team, *model.AppError) { return nil, model.NewAppError("SqlTeamStore.Update", "store.sql_team.update.app_error", nil, "id="+team.Id, http.StatusInternalServerError) } - if oldTeam.DeleteAt == 0 && team.DeleteAt != 0 { - // Invalidate this cache after any team deletion - allTeamIdsForUserCache.Purge() - } - return team, nil } @@ -933,21 +922,9 @@ func (s SqlTeamStore) ResetAllTeamSchemes() *model.AppError { return nil } -var allTeamIdsForUserCache = utils.NewLru(ALL_TEAM_IDS_FOR_USER_CACHE_SIZE) +func (s SqlTeamStore) ClearCaches() {} -func (s SqlTeamStore) ClearCaches() { - allTeamIdsForUserCache.Purge() - if s.metrics != nil { - s.metrics.IncrementMemCacheInvalidationCounter("All Team Ids for User - Purge") - } -} - -func (s SqlTeamStore) InvalidateAllTeamIdsForUser(userId string) { - allTeamIdsForUserCache.Remove(userId) - if s.metrics != nil { - s.metrics.IncrementMemCacheInvalidationCounter("All Team Ids for User - Remove by UserId") - } -} +func (s SqlTeamStore) InvalidateAllTeamIdsForUser(userId string) {} func (s SqlTeamStore) ClearAllCustomRoleAssignments() *model.AppError { @@ -1035,21 +1012,8 @@ func (s SqlTeamStore) GetAllForExportAfter(limit int, afterId string) ([]*model. return data, nil } -// GetUserTeamIds get the team ids to which the user belongs to +// GetUserTeamIds get the team ids to which the user belongs to. allowFromCache parameter does not have any effect in this Store func (s SqlTeamStore) GetUserTeamIds(userID string, allowFromCache bool) ([]string, *model.AppError) { - if allowFromCache { - if cacheItem, ok := allTeamIdsForUserCache.Get(userID); ok { - if s.metrics != nil { - s.metrics.IncrementMemCacheHitCounter("All Team Ids for User") - } - return cacheItem.([]string), nil - } - } - - if s.metrics != nil { - s.metrics.IncrementMemCacheMissCounter("All Team Ids for User") - } - var teamIds []string _, err := s.GetReplica().Select(&teamIds, `SELECT @@ -1067,9 +1031,6 @@ func (s SqlTeamStore) GetUserTeamIds(userID string, allowFromCache bool) ([]stri return []string{}, model.NewAppError("SqlTeamStore.GetUserTeamIds", "store.sql_team.get_user_team_ids.app_error", nil, "userID="+userID+" "+err.Error(), http.StatusInternalServerError) } - if allowFromCache { - allTeamIdsForUserCache.AddWithExpiresInSecs(userID, teamIds, ALL_TEAM_IDS_FOR_USER_CACHE_SEC) - } return teamIds, nil }