From cee1e3685968cbf84b8b655bf438fb6d34a612e5 Mon Sep 17 00:00:00 2001 From: Evan do Carmo <34245571+carmo-evan@users.noreply.github.com> Date: Thu, 30 May 2019 10:44:33 -0400 Subject: [PATCH] MM-15846 migrating get posts to sync by default (#10994) * migrating get posts to sync by default * flow control and style changes * style changes, error checking * pulling master down * counting missed cache, not hit * fixing bad conflict resolution * forcing rebuild --- app/channel_test.go | 4 +- app/file.go | 2 +- app/post.go | 12 +--- store/sqlstore/post_store.go | 90 ++++++++++++++---------------- store/store.go | 2 +- store/storetest/mocks/PostStore.go | 19 +++++-- store/storetest/post_store.go | 14 +++-- 7 files changed, 73 insertions(+), 70 deletions(-) diff --git a/app/channel_test.go b/app/channel_test.go index 114493bc52..9f6f673506 100644 --- a/app/channel_test.go +++ b/app/channel_test.go @@ -452,7 +452,9 @@ func TestAddChannelMemberNoUserRequestor(t *testing.T) { } assert.Equal(t, groupUserIds, channelMemberHistoryUserIds) - postList := store.Must(th.App.Srv.Store.Post().GetPosts(channel.Id, 0, 1, false)).(*model.PostList) + postList, err := th.App.Srv.Store.Post().GetPosts(channel.Id, 0, 1, false) + require.Nil(t,err) + if assert.Len(t, postList.Order, 1) { post := postList.Posts[postList.Order[0]] diff --git a/app/file.go b/app/file.go index a2157eb53b..4dc438e7f9 100644 --- a/app/file.go +++ b/app/file.go @@ -239,7 +239,7 @@ func (a *App) MigrateFilenamesToFileInfos(post *model.Post) []*model.FileInfo { // Find the team that was used to make this post since its part of the file path that isn't saved in the Filename var teamId string if channel.TeamId == "" { - // This post was made in a cross-team DM channel so we need to find where its files were saved + // This post was made in a cross-team DM channel, so we need to find where its files were saved teamId = a.FindTeamIdForFilename(post, filenames[0]) } else { teamId = channel.TeamId diff --git a/app/post.go b/app/post.go index 2f795167de..1d46a1b706 100644 --- a/app/post.go +++ b/app/post.go @@ -599,19 +599,11 @@ func (a *App) PatchPost(postId string, patch *model.PostPatch) (*model.Post, *mo } func (a *App) GetPostsPage(channelId string, page int, perPage int) (*model.PostList, *model.AppError) { - result := <-a.Srv.Store.Post().GetPosts(channelId, page*perPage, perPage, true) - if result.Err != nil { - return nil, result.Err - } - return result.Data.(*model.PostList), nil + return a.Srv.Store.Post().GetPosts(channelId, page*perPage, perPage, true) } func (a *App) GetPosts(channelId string, offset int, limit int) (*model.PostList, *model.AppError) { - result := <-a.Srv.Store.Post().GetPosts(channelId, offset, limit, true) - if result.Err != nil { - return nil, result.Err - } - return result.Data.(*model.PostList), nil + return a.Srv.Store.Post().GetPosts(channelId, offset, limit, true) } func (a *App) GetPostsEtag(channelId string) string { diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index 4b62c20b8b..0f2f8d5a5a 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -450,65 +450,61 @@ func (s *SqlPostStore) PermanentDeleteByChannel(channelId string) *model.AppErro return nil } -func (s *SqlPostStore) GetPosts(channelId string, offset int, limit int, allowFromCache bool) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - if limit > 1000 { - result.Err = model.NewAppError("SqlPostStore.GetLinearPosts", "store.sql_post.get_posts.app_error", nil, "channelId="+channelId, http.StatusBadRequest) - return - } +func (s *SqlPostStore) GetPosts(channelId string, offset int, limit int, allowFromCache bool) (*model.PostList, *model.AppError) { + if limit > 1000 { + return nil, model.NewAppError("SqlPostStore.GetLinearPosts", "store.sql_post.get_posts.app_error", nil, "channelId="+channelId, http.StatusBadRequest) + } - // Caching only occurs on limits of 30 and 60, the common limits requested by MM clients - if allowFromCache && offset == 0 && (limit == 60 || limit == 30) { - if cacheItem, ok := s.lastPostsCache.Get(fmt.Sprintf("%s%v", channelId, limit)); ok { - if s.metrics != nil { - s.metrics.IncrementMemCacheHitCounter("Last Posts Cache") - } - - result.Data = cacheItem.(*model.PostList) - return - } else { - if s.metrics != nil { - s.metrics.IncrementMemCacheMissCounter("Last Posts Cache") - } - } - } else { + // Caching only occurs on limits of 30 and 60, the common limits requested by MM clients + if allowFromCache && offset == 0 && (limit == 60 || limit == 30) { + if cacheItem, ok := s.lastPostsCache.Get(fmt.Sprintf("%s%v", channelId, limit)); ok { if s.metrics != nil { - s.metrics.IncrementMemCacheMissCounter("Last Posts Cache") + s.metrics.IncrementMemCacheHitCounter("Last Posts Cache") } + return cacheItem.(*model.PostList), nil } + } - rpc := s.getRootPosts(channelId, offset, limit) - cpc := s.getParentsPosts(channelId, offset, limit) + if s.metrics != nil { + s.metrics.IncrementMemCacheMissCounter("Last Posts Cache") + } - if rpr := <-rpc; rpr.Err != nil { - result.Err = rpr.Err - } else if cpr := <-cpc; cpr.Err != nil { - result.Err = cpr.Err - } else { - posts := rpr.Data.([]*model.Post) - parents := cpr.Data.([]*model.Post) + rpc := s.getRootPosts(channelId, offset, limit) + cpc := s.getParentsPosts(channelId, offset, limit) - list := model.NewPostList() + var err *model.AppError + list := model.NewPostList() - for _, p := range posts { - list.AddPost(p) - list.AddOrder(p.Id) - } + rpr := <-rpc + if rpr.Err != nil { + return nil, rpr.Err + } - for _, p := range parents { - list.AddPost(p) - } + cpr := <-cpc + if cpr.Err != nil { + return nil, cpr.Err + } - list.MakeNonNil() + posts := rpr.Data.([]*model.Post) + parents := cpr.Data.([]*model.Post) - // Caching only occurs on limits of 30 and 60, the common limits requested by MM clients - if offset == 0 && (limit == 60 || limit == 30) { - s.lastPostsCache.AddWithExpiresInSecs(fmt.Sprintf("%s%v", channelId, limit), list, LAST_POSTS_CACHE_SEC) - } + for _, p := range posts { + list.AddPost(p) + list.AddOrder(p.Id) + } - result.Data = list - } - }) + for _, p := range parents { + list.AddPost(p) + } + + list.MakeNonNil() + + // Caching only occurs on limits of 30 and 60, the common limits requested by MM clients + if offset == 0 && (limit == 60 || limit == 30) { + s.lastPostsCache.AddWithExpiresInSecs(fmt.Sprintf("%s%v", channelId, limit), list, LAST_POSTS_CACHE_SEC) + } + + return list, err } func (s *SqlPostStore) GetPostsSince(channelId string, time int64, allowFromCache bool) store.StoreChannel { diff --git a/store/store.go b/store/store.go index d1a6873eb5..38bb012dbc 100644 --- a/store/store.go +++ b/store/store.go @@ -217,7 +217,7 @@ type PostStore interface { Delete(postId string, time int64, deleteByID string) *model.AppError PermanentDeleteByUser(userId string) StoreChannel PermanentDeleteByChannel(channelId string) *model.AppError - GetPosts(channelId string, offset int, limit int, allowFromCache bool) StoreChannel + GetPosts(channelId string, offset int, limit int, allowFromCache bool) (*model.PostList, *model.AppError) GetFlaggedPosts(userId string, offset int, limit int) (*model.PostList, *model.AppError) GetFlaggedPostsForTeam(userId, teamId string, offset int, limit int) (*model.PostList, *model.AppError) GetFlaggedPostsForChannel(userId, channelId string, offset int, limit int) StoreChannel diff --git a/store/storetest/mocks/PostStore.go b/store/storetest/mocks/PostStore.go index 6c76081090..b9921e3158 100644 --- a/store/storetest/mocks/PostStore.go +++ b/store/storetest/mocks/PostStore.go @@ -252,19 +252,28 @@ func (_m *PostStore) GetParentsForExportAfter(limit int, afterId string) store.S } // GetPosts provides a mock function with given fields: channelId, offset, limit, allowFromCache -func (_m *PostStore) GetPosts(channelId string, offset int, limit int, allowFromCache bool) store.StoreChannel { +func (_m *PostStore) GetPosts(channelId string, offset int, limit int, allowFromCache bool) (*model.PostList, *model.AppError) { ret := _m.Called(channelId, offset, limit, allowFromCache) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string, int, int, bool) store.StoreChannel); ok { + var r0 *model.PostList + if rf, ok := ret.Get(0).(func(string, int, int, bool) *model.PostList); ok { r0 = rf(channelId, offset, limit, allowFromCache) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(*model.PostList) } } - return r0 + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string, int, int, bool) *model.AppError); ok { + r1 = rf(channelId, offset, limit, allowFromCache) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } // GetPostsAfter provides a mock function with given fields: channelId, postId, numPosts, offset diff --git a/store/storetest/post_store.go b/store/storetest/post_store.go index d11a411d1e..4cc2016f8f 100644 --- a/store/storetest/post_store.go +++ b/store/storetest/post_store.go @@ -636,7 +636,8 @@ func testPostStoreGetPostsWithDetails(t *testing.T, ss store.Store) { o5.RootId = o4.Id o5 = (<-ss.Post().Save(o5)).Data.(*model.Post) - r1 := (<-ss.Post().GetPosts(o1.ChannelId, 0, 4, false)).Data.(*model.PostList) + r1, err := ss.Post().GetPosts(o1.ChannelId, 0, 4, false) + require.Nil(t, err) if r1.Order[0] != o5.Id { t.Fatal("invalid order") @@ -662,7 +663,8 @@ func testPostStoreGetPostsWithDetails(t *testing.T, ss store.Store) { t.Fatal("Missing parent") } - r2 := (<-ss.Post().GetPosts(o1.ChannelId, 0, 4, true)).Data.(*model.PostList) + r2, err := ss.Post().GetPosts(o1.ChannelId, 0, 4, true) + require.Nil(t, err) if r2.Order[0] != o5.Id { t.Fatal("invalid order") @@ -689,7 +691,7 @@ func testPostStoreGetPostsWithDetails(t *testing.T, ss store.Store) { } // Run once to fill cache - <-ss.Post().GetPosts(o1.ChannelId, 0, 30, true) + ss.Post().GetPosts(o1.ChannelId, 0, 30, true) o6 := &model.Post{} o6.ChannelId = o1.ChannelId @@ -698,13 +700,15 @@ func testPostStoreGetPostsWithDetails(t *testing.T, ss store.Store) { _ = (<-ss.Post().Save(o6)).Data.(*model.Post) // Should only be 6 since we hit the cache - r3 := (<-ss.Post().GetPosts(o1.ChannelId, 0, 30, true)).Data.(*model.PostList) + r3, err := ss.Post().GetPosts(o1.ChannelId, 0, 30, true) + require.Nil(t, err) assert.Equal(t, 6, len(r3.Order)) ss.Post().InvalidateLastPostTimeCache(o1.ChannelId) // Cache was invalidated, we should get all the posts - r4 := (<-ss.Post().GetPosts(o1.ChannelId, 0, 30, true)).Data.(*model.PostList) + r4, err := ss.Post().GetPosts(o1.ChannelId, 0, 30, true) + require.Nil(t, err) assert.Equal(t, 7, len(r4.Order)) }