From 01fa648886d8bbb3871d3a8bd884e2d309b7638e Mon Sep 17 00:00:00 2001 From: Shota Gvinepadze Date: Thu, 20 Jun 2019 18:25:53 +0400 Subject: [PATCH] [MM-11222] Migrate 'Channel.GetMemberCount' to Sync by default (#11246) * Migrate 'Channel.GetMemberCount' to Sync by default * Rename member count chan * Fix variable shadowing --- app/channel.go | 22 ++++----- store/sqlstore/channel_store.go | 65 +++++++++++++-------------- store/store.go | 2 +- store/storetest/channel_store.go | 55 +++++++++++++---------- store/storetest/mocks/ChannelStore.go | 19 +++++--- 5 files changed, 88 insertions(+), 75 deletions(-) diff --git a/app/channel.go b/app/channel.go index 54543f4661..7ad2a8d670 100644 --- a/app/channel.go +++ b/app/channel.go @@ -1317,11 +1317,7 @@ func (a *App) GetChannelMembersForUserWithPagination(teamId, userId string, page } func (a *App) GetChannelMemberCount(channelId string) (int64, *model.AppError) { - result := <-a.Srv.Store.Channel().GetMemberCount(channelId, true) - if result.Err != nil { - return 0, result.Err - } - return result.Data.(int64), nil + return a.Srv.Store.Channel().GetMemberCount(channelId, true) } func (a *App) GetChannelCounts(teamId string, userId string) (*model.ChannelCounts, *model.AppError) { @@ -1452,7 +1448,13 @@ func (a *App) LeaveChannel(channelId string, userId string) *model.AppError { uc <- store.StoreResult{Data: user, Err: err} close(uc) }() - ccm := a.Srv.Store.Channel().GetMemberCount(channelId, false) + + mcc := make(chan store.StoreResult, 1) + go func() { + count, err := a.Srv.Store.Channel().GetMemberCount(channelId, false) + mcc <- store.StoreResult{Data: count, Err: err} + close(mcc) + }() cresult := <-sc if cresult.Err != nil { @@ -1462,14 +1464,14 @@ func (a *App) LeaveChannel(channelId string, userId string) *model.AppError { if uresult.Err != nil { return cresult.Err } - ccmresult := <-ccm - if ccmresult.Err != nil { - return ccmresult.Err + ccresult := <-mcc + if ccresult.Err != nil { + return ccresult.Err } channel := cresult.Data.(*model.Channel) user := uresult.Data.(*model.User) - membersCount := ccmresult.Data.(int64) + membersCount := ccresult.Data.(int64) if channel.IsGroupOrDirect() { err := model.NewAppError("LeaveChannel", "api.channel.leave.direct.app_error", nil, "", http.StatusBadRequest) diff --git a/store/sqlstore/channel_store.go b/store/sqlstore/channel_store.go index cf4e9e2489..10a541eedf 100644 --- a/store/sqlstore/channel_store.go +++ b/store/sqlstore/channel_store.go @@ -1618,50 +1618,47 @@ func (s SqlChannelStore) GetMemberCountFromCache(channelId string) int64 { s.metrics.IncrementMemCacheMissCounter("Channel Member Counts") } - result := <-s.GetMemberCount(channelId, true) - if result.Err != nil { + count, err := s.GetMemberCount(channelId, true) + if err != nil { return 0 } - return result.Data.(int64) + return count } -func (s SqlChannelStore) GetMemberCount(channelId string, allowFromCache bool) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - if allowFromCache { - if cacheItem, ok := channelMemberCountsCache.Get(channelId); ok { - if s.metrics != nil { - s.metrics.IncrementMemCacheHitCounter("Channel Member Counts") - } - result.Data = cacheItem.(int64) - return +func (s SqlChannelStore) GetMemberCount(channelId string, allowFromCache bool) (int64, *model.AppError) { + if allowFromCache { + if cacheItem, ok := channelMemberCountsCache.Get(channelId); ok { + if s.metrics != nil { + s.metrics.IncrementMemCacheHitCounter("Channel Member Counts") } + return cacheItem.(int64), nil } + } - if s.metrics != nil { - s.metrics.IncrementMemCacheMissCounter("Channel Member Counts") - } + if s.metrics != nil { + s.metrics.IncrementMemCacheMissCounter("Channel Member Counts") + } - count, err := s.GetReplica().SelectInt(` - SELECT - count(*) - FROM - ChannelMembers, - Users - WHERE - ChannelMembers.UserId = Users.Id - AND ChannelMembers.ChannelId = :ChannelId - AND Users.DeleteAt = 0`, map[string]interface{}{"ChannelId": channelId}) - if err != nil { - result.Err = model.NewAppError("SqlChannelStore.GetMemberCount", "store.sql_channel.get_member_count.app_error", nil, "channel_id="+channelId+", "+err.Error(), http.StatusInternalServerError) - return - } - result.Data = count + count, err := s.GetReplica().SelectInt(` + SELECT + count(*) + FROM + ChannelMembers, + Users + WHERE + ChannelMembers.UserId = Users.Id + AND ChannelMembers.ChannelId = :ChannelId + AND Users.DeleteAt = 0`, map[string]interface{}{"ChannelId": channelId}) + if err != nil { + return 0, model.NewAppError("SqlChannelStore.GetMemberCount", "store.sql_channel.get_member_count.app_error", nil, "channel_id="+channelId+", "+err.Error(), http.StatusInternalServerError) + } - if allowFromCache { - channelMemberCountsCache.AddWithExpiresInSecs(channelId, count, CHANNEL_MEMBERS_COUNTS_CACHE_SEC) - } - }) + if allowFromCache { + channelMemberCountsCache.AddWithExpiresInSecs(channelId, count, CHANNEL_MEMBERS_COUNTS_CACHE_SEC) + } + + return count, nil } func (s SqlChannelStore) RemoveMember(channelId string, userId string) *model.AppError { diff --git a/store/store.go b/store/store.go index 6313c67f7a..19bbc14e55 100644 --- a/store/store.go +++ b/store/store.go @@ -170,7 +170,7 @@ type ChannelStore interface { GetMemberForPost(postId string, userId string) (*model.ChannelMember, *model.AppError) InvalidateMemberCount(channelId string) GetMemberCountFromCache(channelId string) int64 - GetMemberCount(channelId string, allowFromCache bool) StoreChannel + GetMemberCount(channelId string, allowFromCache bool) (int64, *model.AppError) GetPinnedPosts(channelId string) StoreChannel RemoveMember(channelId string, userId string) *model.AppError PermanentDeleteMembersByUser(userId string) StoreChannel diff --git a/store/storetest/channel_store.go b/store/storetest/channel_store.go index 254e34ebbe..4d99339534 100644 --- a/store/storetest/channel_store.go +++ b/store/storetest/channel_store.go @@ -888,12 +888,14 @@ func testChannelMemberStore(t *testing.T, ss store.Store) { c1t2, _ := ss.Channel().Get(c1.Id, false) assert.EqualValues(t, 0, c1t2.ExtraUpdateAt, "ExtraUpdateAt should be 0") - count := (<-ss.Channel().GetMemberCount(o1.ChannelId, true)).Data.(int64) + count, err := ss.Channel().GetMemberCount(o1.ChannelId, true) + require.Nil(t, err) if count != 2 { t.Fatal("should have saved 2 members") } - count = (<-ss.Channel().GetMemberCount(o1.ChannelId, true)).Data.(int64) + count, err = ss.Channel().GetMemberCount(o1.ChannelId, true) + require.Nil(t, err) if count != 2 { t.Fatal("should have saved 2 members") } @@ -906,7 +908,8 @@ func testChannelMemberStore(t *testing.T, ss store.Store) { t.Fatal("should have saved 0 members") } - count = (<-ss.Channel().GetMemberCount(o1.ChannelId, false)).Data.(int64) + count, err = ss.Channel().GetMemberCount(o1.ChannelId, false) + require.Nil(t, err) if count != 2 { t.Fatal("should have saved 2 members") } @@ -914,7 +917,8 @@ func testChannelMemberStore(t *testing.T, ss store.Store) { err = ss.Channel().RemoveMember(o2.ChannelId, o2.UserId) require.Nil(t, err) - count = (<-ss.Channel().GetMemberCount(o1.ChannelId, false)).Data.(int64) + count, err = ss.Channel().GetMemberCount(o1.ChannelId, false) + require.Nil(t, err) if count != 1 { t.Fatal("should have removed 1 member") } @@ -974,23 +978,26 @@ func testChannelDeleteMemberStore(t *testing.T, ss store.Store) { c1t2, _ := ss.Channel().Get(c1.Id, false) assert.EqualValues(t, 0, c1t2.ExtraUpdateAt, "ExtraUpdateAt should be 0") - count := (<-ss.Channel().GetMemberCount(o1.ChannelId, false)).Data.(int64) + count, err := ss.Channel().GetMemberCount(o1.ChannelId, false) + require.Nil(t, err) if count != 2 { t.Fatal("should have saved 2 members") } store.Must(ss.Channel().PermanentDeleteMembersByUser(o2.UserId)) - count = (<-ss.Channel().GetMemberCount(o1.ChannelId, false)).Data.(int64) + count, err = ss.Channel().GetMemberCount(o1.ChannelId, false) + require.Nil(t, err) if count != 1 { t.Fatal("should have removed 1 member") } - if err := ss.Channel().PermanentDeleteMembersByChannel(o1.ChannelId); err != nil { + if err = ss.Channel().PermanentDeleteMembersByChannel(o1.ChannelId); err != nil { t.Fatal(err) } - count = (<-ss.Channel().GetMemberCount(o1.ChannelId, false)).Data.(int64) + count, err = ss.Channel().GetMemberCount(o1.ChannelId, false) + require.Nil(t, err) if count != 0 { t.Fatal("should have removed all members") } @@ -1911,10 +1918,10 @@ func testGetMemberCount(t *testing.T, ss store.Store) { } store.Must(ss.Channel().SaveMember(&m1)) - if result := <-ss.Channel().GetMemberCount(c1.Id, false); result.Err != nil { - t.Fatalf("failed to get member count: %v", result.Err) - } else if result.Data.(int64) != 1 { - t.Fatalf("got incorrect member count %v", result.Data) + if count, err := ss.Channel().GetMemberCount(c1.Id, false); err != nil { + t.Fatalf("failed to get member count: %v", err) + } else if count != 1 { + t.Fatalf("got incorrect member count %v", count) } u2 := model.User{ @@ -1931,10 +1938,10 @@ func testGetMemberCount(t *testing.T, ss store.Store) { } store.Must(ss.Channel().SaveMember(&m2)) - if result := <-ss.Channel().GetMemberCount(c1.Id, false); result.Err != nil { - t.Fatalf("failed to get member count: %v", result.Err) - } else if result.Data.(int64) != 2 { - t.Fatalf("got incorrect member count %v", result.Data) + if count, err := ss.Channel().GetMemberCount(c1.Id, false); err != nil { + t.Fatalf("failed to get member count: %v", err) + } else if count != 2 { + t.Fatalf("got incorrect member count %v", count) } // make sure members of other channels aren't counted @@ -1952,10 +1959,10 @@ func testGetMemberCount(t *testing.T, ss store.Store) { } store.Must(ss.Channel().SaveMember(&m3)) - if result := <-ss.Channel().GetMemberCount(c1.Id, false); result.Err != nil { - t.Fatalf("failed to get member count: %v", result.Err) - } else if result.Data.(int64) != 2 { - t.Fatalf("got incorrect member count %v", result.Data) + if count, err := ss.Channel().GetMemberCount(c1.Id, false); err != nil { + t.Fatalf("failed to get member count: %v", err) + } else if count != 2 { + t.Fatalf("got incorrect member count %v", count) } // make sure inactive users aren't counted @@ -1973,10 +1980,10 @@ func testGetMemberCount(t *testing.T, ss store.Store) { } store.Must(ss.Channel().SaveMember(&m4)) - if result := <-ss.Channel().GetMemberCount(c1.Id, false); result.Err != nil { - t.Fatalf("failed to get member count: %v", result.Err) - } else if result.Data.(int64) != 2 { - t.Fatalf("got incorrect member count %v", result.Data) + if count, err := ss.Channel().GetMemberCount(c1.Id, false); err != nil { + t.Fatalf("failed to get member count: %v", err) + } else if count != 2 { + t.Fatalf("got incorrect member count %v", count) } } diff --git a/store/storetest/mocks/ChannelStore.go b/store/storetest/mocks/ChannelStore.go index c488e0eeaa..8d30327645 100644 --- a/store/storetest/mocks/ChannelStore.go +++ b/store/storetest/mocks/ChannelStore.go @@ -693,19 +693,26 @@ func (_m *ChannelStore) GetMember(channelId string, userId string) (*model.Chann } // GetMemberCount provides a mock function with given fields: channelId, allowFromCache -func (_m *ChannelStore) GetMemberCount(channelId string, allowFromCache bool) store.StoreChannel { +func (_m *ChannelStore) GetMemberCount(channelId string, allowFromCache bool) (int64, *model.AppError) { ret := _m.Called(channelId, allowFromCache) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string, bool) store.StoreChannel); ok { + var r0 int64 + if rf, ok := ret.Get(0).(func(string, bool) int64); ok { r0 = rf(channelId, allowFromCache) } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(int64) + } + + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string, bool) *model.AppError); ok { + r1 = rf(channelId, allowFromCache) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) } } - return r0 + return r0, r1 } // GetMemberCountFromCache provides a mock function with given fields: channelId