From 1e28ad4a73abdb367cda36836586788301c4492c Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Mon, 30 Dec 2019 22:52:31 +0530 Subject: [PATCH] MM-21209: Use the LRU cache for UserStore.Get call too (#13456) * MM-21209: Use the LRU cache for UserStore.Get call too We already have userProfileByIdsCache to store the user profiles by Id. It just wasn't being used for the (*UserStore).Get method. We add a wrapper method in LocalCacheUserStore to intercept that call and check for the presence of the user Id in the cache. There is no need to add any code for invalidation as all of that is already present. * Fix nil check for rootstore * Fix TestUserStore test Added an invalidate call. The invalidation was being done from the app level. Hence we have to do it manually here for the test. --- app/server_app_adapters.go | 6 +++- store/localcachelayer/main_test.go | 1 + store/localcachelayer/user_layer.go | 25 ++++++++++++++++- store/localcachelayer/user_layer_test.go | 35 +++++++++++++++++++++++- store/storetest/user_store.go | 2 ++ 5 files changed, 66 insertions(+), 3 deletions(-) diff --git a/app/server_app_adapters.go b/app/server_app_adapters.go index bfa2dcd449..32db15e7f1 100644 --- a/app/server_app_adapters.go +++ b/app/server_app_adapters.go @@ -61,7 +61,11 @@ func (s *Server) RunOldAppInitialization() error { if s.FakeApp().Srv.newStore == nil { s.FakeApp().Srv.newStore = func() store.Store { - return store.NewTimerLayer(localcachelayer.NewLocalCacheLayer(sqlstore.NewSqlSupplier(s.FakeApp().Config().SqlSettings, s.Metrics), s.Metrics, s.Cluster), s.Metrics) + return store.NewTimerLayer( + localcachelayer.NewLocalCacheLayer( + sqlstore.NewSqlSupplier(s.FakeApp().Config().SqlSettings, s.Metrics), + s.Metrics, s.Cluster), + s.Metrics) } } diff --git a/store/localcachelayer/main_test.go b/store/localcachelayer/main_test.go index 15514facec..e306fe50a1 100644 --- a/store/localcachelayer/main_test.go +++ b/store/localcachelayer/main_test.go @@ -112,6 +112,7 @@ func getMockStore() *mocks.Store { mockUserStore := mocks.UserStore{} mockUserStore.On("GetProfileByIds", []string{"123"}, &store.UserGetByIdsOpts{}, true).Return(fakeUser, nil) mockUserStore.On("GetProfileByIds", []string{"123"}, &store.UserGetByIdsOpts{}, false).Return(fakeUser, nil) + mockUserStore.On("Get", "123").Return(fakeUser[0], nil) mockStore.On("User").Return(&mockUserStore) fakeUserTeamIds := []string{"1", "2", "3"} diff --git a/store/localcachelayer/user_layer.go b/store/localcachelayer/user_layer.go index 7104602a9e..102df7e342 100644 --- a/store/localcachelayer/user_layer.go +++ b/store/localcachelayer/user_layer.go @@ -80,8 +80,31 @@ func (s LocalCacheUserStore) GetProfileByIds(userIds []string, options *store.Us for _, user := range remainingUsers { s.rootStore.doStandardAddToCache(s.rootStore.userProfileByIdsCache, user.Id, user) } - } return users, nil } + +// Get is a cache wrapper around the SqlStore method to get a user profile by id. +// It checks if the user entry is present in the cache, returning the entry from cache +// if it is present. Otherwise, it fetches the entry from the store and stores it in the +// cache. +func (s LocalCacheUserStore) Get(id string) (*model.User, *model.AppError) { + cacheItem := s.rootStore.doStandardReadCache(s.rootStore.userProfileByIdsCache, id) + if cacheItem != nil { + if s.rootStore.metrics != nil { + s.rootStore.metrics.AddMemCacheHitCounter("Profile By Id", float64(1)) + } + u := *cacheItem.(*model.User) + return &u, nil + } + if s.rootStore.metrics != nil { + s.rootStore.metrics.AddMemCacheMissCounter("Profile By Id", float64(1)) + } + user, err := s.UserStore.Get(id) + if err != nil { + return nil, model.NewAppError("SqlUserStore.Get", "store.sql_user.get.app_error", nil, err.Error(), http.StatusInternalServerError) + } + s.rootStore.doStandardAddToCache(s.rootStore.userProfileByIdsCache, id, user) + return user, nil +} diff --git a/store/localcachelayer/user_layer_test.go b/store/localcachelayer/user_layer_test.go index 257968791c..9b43be64f2 100644 --- a/store/localcachelayer/user_layer_test.go +++ b/store/localcachelayer/user_layer_test.go @@ -19,7 +19,7 @@ func TestUserStore(t *testing.T) { StoreTestWithSqlSupplier(t, storetest.TestUserStore) } -func TestUserStoreCache(t *testing.T) { +func TestUserStoreGetProfileByIdsCache(t *testing.T) { fakeUserIds := []string{"123"} fakeUser := []*model.User{{Id: "123"}} @@ -64,3 +64,36 @@ func TestUserStoreCache(t *testing.T) { mockStore.User().(*mocks.UserStore).AssertNumberOfCalls(t, "GetProfileByIds", 2) }) } + +func TestUserStoreGetCache(t *testing.T) { + fakeUserId := "123" + fakeUser := &model.User{Id: "123"} + + t.Run("first call not cached, second cached and returning same data", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + gotUser, err := cachedStore.User().Get(fakeUserId) + require.Nil(t, err) + assert.Equal(t, fakeUser, gotUser) + mockStore.User().(*mocks.UserStore).AssertNumberOfCalls(t, "Get", 1) + + _, _ = cachedStore.User().Get(fakeUserId) + mockStore.User().(*mocks.UserStore).AssertNumberOfCalls(t, "Get", 1) + }) + + t.Run("first call not cached, invalidate, and then not cached again", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + gotUser, err := cachedStore.User().Get(fakeUserId) + require.Nil(t, err) + assert.Equal(t, fakeUser, gotUser) + mockStore.User().(*mocks.UserStore).AssertNumberOfCalls(t, "Get", 1) + + cachedStore.User().InvalidatProfileCacheForUser("123") + + _, _ = cachedStore.User().Get(fakeUserId) + mockStore.User().(*mocks.UserStore).AssertNumberOfCalls(t, "Get", 2) + }) +} diff --git a/store/storetest/user_store.go b/store/storetest/user_store.go index 58a32883f5..dbb5fea27e 100644 --- a/store/storetest/user_store.go +++ b/store/storetest/user_store.go @@ -4949,6 +4949,8 @@ func testUserStoreResetLastPictureUpdate(t *testing.T, ss store.Store) { err = ss.User().ResetLastPictureUpdate(u1.Id) require.Nil(t, err) + ss.User().InvalidatProfileCacheForUser(u1.Id) + user2, err := ss.User().Get(u1.Id) require.Nil(t, err)