From eb29750f19b2252f9f76993f97c252b1b4e5188e Mon Sep 17 00:00:00 2001 From: Joram Wilander Date: Wed, 5 Feb 2020 09:42:36 -0500 Subject: [PATCH] Revert "MM-21209: Use the LRU cache for UserStore.Get call too (#13456)" (#13824) This reverts commit 1e28ad4a73abdb367cda36836586788301c4492c. --- store/localcachelayer/main_test.go | 1 - store/localcachelayer/user_layer.go | 25 +-------- store/localcachelayer/user_layer_test.go | 68 +----------------------- store/storetest/user_store.go | 2 - 4 files changed, 2 insertions(+), 94 deletions(-) diff --git a/store/localcachelayer/main_test.go b/store/localcachelayer/main_test.go index 2f75014c4d..a745935be2 100644 --- a/store/localcachelayer/main_test.go +++ b/store/localcachelayer/main_test.go @@ -223,7 +223,6 @@ func getMockStore() *mocks.Store { mockUserStore.On("GetAllProfilesInChannel", "123", true).Return(fakeProfilesInChannelMap, nil) mockUserStore.On("GetAllProfilesInChannel", "123", false).Return(fakeProfilesInChannelMap, 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 a913dc36b4..2224f56a7f 100644 --- a/store/localcachelayer/user_layer.go +++ b/store/localcachelayer/user_layer.go @@ -130,31 +130,8 @@ func (s LocalCacheUserStore) GetProfileByIds(userIds []string, options *store.Us users = append(users, user.DeepCopy()) 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.DeepCopy(), 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.DeepCopy(), nil -} diff --git a/store/localcachelayer/user_layer_test.go b/store/localcachelayer/user_layer_test.go index 8507acf3c6..90cd8e5d3c 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 TestUserStoreGetProfileByIdsCache(t *testing.T) { +func TestUserStoreCache(t *testing.T) { fakeUserIds := []string{"123"} fakeUser := []*model.User{{Id: "123"}} @@ -179,69 +179,3 @@ func TestUserStoreProfilesInChannelCache(t *testing.T) { mockStore.User().(*mocks.UserStore).AssertNumberOfCalls(t, "GetAllProfilesInChannel", 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() - mockCacheProvider := getMockCacheProvider() - cachedStore := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider) - - 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() - mockCacheProvider := getMockCacheProvider() - cachedStore := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider) - - 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().InvalidateProfileCacheForUser("123") - - _, _ = cachedStore.User().Get(fakeUserId) - mockStore.User().(*mocks.UserStore).AssertNumberOfCalls(t, "Get", 2) - }) - - t.Run("should always return a copy of the stored data", func(t *testing.T) { - mockStore := getMockStore() - mockCacheProvider := getMockCacheProvider() - cachedStore := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider) - - storedUser, err := mockStore.User().Get(fakeUserId) - require.Nil(t, err) - originalProps := storedUser.NotifyProps - - storedUser.NotifyProps = map[string]string{} - storedUser.NotifyProps["key"] = "somevalue" - - cachedUser, err := cachedStore.User().Get(fakeUserId) - require.Nil(t, err) - assert.Equal(t, storedUser, cachedUser) - if storedUser == cachedUser { - assert.Fail(t, "should be different pointers") - } - cachedUser.NotifyProps["key"] = "othervalue" - assert.NotEqual(t, storedUser, cachedUser) - - cachedUser, err = cachedStore.User().Get(fakeUserId) - require.Nil(t, err) - assert.Equal(t, storedUser, cachedUser) - if storedUser == cachedUser { - assert.Fail(t, "should be different pointers") - } - cachedUser.NotifyProps["key"] = "othervalue" - assert.NotEqual(t, storedUser, cachedUser) - - storedUser.NotifyProps = originalProps - }) -} diff --git a/store/storetest/user_store.go b/store/storetest/user_store.go index 7522eebfec..58a32883f5 100644 --- a/store/storetest/user_store.go +++ b/store/storetest/user_store.go @@ -4949,8 +4949,6 @@ func testUserStoreResetLastPictureUpdate(t *testing.T, ss store.Store) { err = ss.User().ResetLastPictureUpdate(u1.Id) require.Nil(t, err) - ss.User().InvalidateProfileCacheForUser(u1.Id) - user2, err := ss.User().Get(u1.Id) require.Nil(t, err)