mirrors/mostlymatter
1
0
Форкнуть 0
Код Задачи Запросы на слияние Действия Пакеты Проекты Релизы Вики Активность

MM-40302: CRT, fix performance of MarkAllAsReadInChannels (#19566)

Просмотр исходного кода 
* deadcode: remove UpdateChannelLastViewedAt

* deadcode: remove ThreadStore.(Save(Multiple)|Update|Delete)

* deadcode: followThead in App.MarkChannelAsUnreadFromPost

* document ThreadMembership, Thread structs

* maintain LastUpdated consistently

Whenever we touch a `ThreadMembership` record, we should be setting `LastUpdated` to the current timestamp. The mobile client relies on this to detect changes to these records.

* simplify: never updateThreads from `App.MarkChannelAsUnreadFromPost`

Change all invocations of `ChannelStore.UpdateLastViewedAtPost` from `App.MarkChannelAsUnreadFromPost` to pass `updateThreads` as `false`. When `ChannelStore.UpdateLastViewedAtPost` was invoked with `updateThreads` as `true`, it would in turn call `ThreadStore.UpdateUnreadsByChannel` but pass `updateViewedTimestamp` as `false`. This effectively updated the `LastUpdated` field of the corresponding thread memberships but never touched any of the actual data (such as `LastViewed`).

The overall CRT feature continued to work, because `App.MarkChannelAsUnreadFromPost` directly updates the relevant thread memberships via `ThreadStore.MaintainMembership`.

* deadcode: updateThreads in ChannelStore.UpdateLastViewedAtPost

* simplify: never updateThreads from App.SendNotifications

Change all invocations of `ChannelStore.IncrementMentionCount` from
`App.SendNotifications` to pass `updateThreads` as `false`. When `ChannelStore.IncrementMentionCount` was invoked with `updateThreads` as `true`, it would in turn call `ThreadStore.UpdateUnreadsByChannel` but pass `updateViewedTimestamp` as `false`. This effectively updated the `LastUpdated` field of the corresponding thread memberships but never touched any of the actual data (such as `UnreadMentions`).

The overall CRT feature continued to work, because `App.SendNotifications` directly updates the relevant thread memberships mention counts via `ThreadStore.MaintainMembership`.

* deadcode: updateThreads in ChannelStore.IncrementMentionCount

* fix & rename ThreadStore.UpdateUnreadsByChannel

Rename `ThreadStore.UpdateUnreadsByChannel` to `ThreadStore.UpdateLastViewedByThreadIds`, making it unconditionally set the `LastViewed` for the given threads (as well as `LastUpdated`).

All previous invocations of this method that passed `updateViewedTimestamp` have been previously removed.

* unrelated gofmt -w -s changes to satisfy linter

* always set LastUpdated to model.GetMillis()

* deadcode: ThreadStore.SaveMembership

* fix TestMarkUnreadWithThreads

* MM-40302: CRT, use updateThreads param vs. MarkAllAsReadInChannels

`MarkAllAsReadInChannels` was the subject of a significant performance regression in v5.37 and is known to be very inefficient, by virtue of always writing to an ever increasing number of rows, and doing so on common events like simply viewing a channel.

Fortunately, `ChannelStore.UpdateLastViewedAt` already supported an `updateThreads` parameter that implemented the start of an improved algorithm: query the set of threads with newer posts, and then update only /those/. Missing was the need to reset the `UnreadMentions`, but thanks to the previous simplifications in #19523, we can make this change largely without impacting other semantics.

Fixes: https://mattermost.atlassian.net/browse/MM-40302

* fix MySQL

* remove another JOIN

* remove outdated comment

* unit tests
Этот коммит содержится в:
Jesse Hallam
2022-03-06 19:55:59 -04:00
коммит произвёл GitHub
родитель c114aba628
Коммит f5b5a3c746
13 изменённых файлов: 331 добавлений и 322 удалений
Скачать Patch файл Скачать Diff файл Показать все файлы Свернуть все файлы

6
store/storetest/channel_store.go
Просмотреть файл

@@ -4749,11 +4749,11 @@ func testChannelStoreUpdateLastViewedAt(t *testing.T, ss store.Store) {
require.NoError(t, err)
var times map[string]int64
times, err = ss.Channel().UpdateLastViewedAt([]string{m1.ChannelId}, m1.UserId, false)
times, err = ss.Channel().UpdateLastViewedAt([]string{m1.ChannelId}, m1.UserId)
require.NoError(t, err, "failed to update ", err)
require.Equal(t, o1.LastPostAt, times[o1.Id], "last viewed at time incorrect")
times, err = ss.Channel().UpdateLastViewedAt([]string{m1.ChannelId, m2.ChannelId}, m1.UserId, false)
times, err = ss.Channel().UpdateLastViewedAt([]string{m1.ChannelId, m2.ChannelId}, m1.UserId)
require.NoError(t, err, "failed to update ", err)
require.Equal(t, o2.LastPostAt, times[o2.Id], "last viewed at time incorrect")
@@ -4769,7 +4769,7 @@ func testChannelStoreUpdateLastViewedAt(t *testing.T, ss store.Store) {
assert.Equal(t, o2.LastPostAt, rm2.LastUpdateAt)
assert.Equal(t, o2.TotalMsgCount, rm2.MsgCount)
_, err = ss.Channel().UpdateLastViewedAt([]string{m1.ChannelId}, "missing id", false)
_, err = ss.Channel().UpdateLastViewedAt([]string{m1.ChannelId}, "missing id")
require.NoError(t, err, "failed to update")
}

14
store/storetest/mocks/ChannelStore.go
Просмотреть файл

@@ -2008,13 +2008,13 @@ func (_m *ChannelStore) Update(channel *model.Channel) (*model.Channel, error) {
return r0, r1
}
// UpdateLastViewedAt provides a mock function with given fields: channelIds, userID, updateThreads
func (_m *ChannelStore) UpdateLastViewedAt(channelIds []string, userID string, updateThreads bool) (map[string]int64, error) {
ret := _m.Called(channelIds, userID, updateThreads)
// UpdateLastViewedAt provides a mock function with given fields: channelIds, userID
func (_m *ChannelStore) UpdateLastViewedAt(channelIds []string, userID string) (map[string]int64, error) {
ret := _m.Called(channelIds, userID)
var r0 map[string]int64
if rf, ok := ret.Get(0).(func([]string, string, bool) map[string]int64); ok {
r0 = rf(channelIds, userID, updateThreads)
if rf, ok := ret.Get(0).(func([]string, string) map[string]int64); ok {
r0 = rf(channelIds, userID)
} else {
if ret.Get(0) != nil {
r0 = ret.Get(0).(map[string]int64)
@@ -2022,8 +2022,8 @@ func (_m *ChannelStore) UpdateLastViewedAt(channelIds []string, userID string, u
}
var r1 error
if rf, ok := ret.Get(1).(func([]string, string, bool) error); ok {
r1 = rf(channelIds, userID, updateThreads)
if rf, ok := ret.Get(1).(func([]string, string) error); ok {
r1 = rf(channelIds, userID)
} else {
r1 = ret.Error(1)
}

65
store/storetest/mocks/ThreadStore.go
Просмотреть файл

@@ -15,29 +15,6 @@ type ThreadStore struct {
mock.Mock
}
// CollectThreadsWithNewerReplies provides a mock function with given fields: userId, channelIds, timestamp
func (_m *ThreadStore) CollectThreadsWithNewerReplies(userId string, channelIds []string, timestamp int64) ([]string, error) {
ret := _m.Called(userId, channelIds, timestamp)
var r0 []string
if rf, ok := ret.Get(0).(func(string, []string, int64) []string); ok {
r0 = rf(userId, channelIds, timestamp)
} else {
if ret.Get(0) != nil {
r0 = ret.Get(0).([]string)
}
}
var r1 error
if rf, ok := ret.Get(1).(func(string, []string, int64) error); ok {
r1 = rf(userId, channelIds, timestamp)
} else {
r1 = ret.Error(1)
}
return r0, r1
}
// DeleteMembershipForUser provides a mock function with given fields: userId, postID
func (_m *ThreadStore) DeleteMembershipForUser(userId string, postID string) error {
ret := _m.Called(userId, postID)
@@ -301,13 +278,13 @@ func (_m *ThreadStore) MaintainMembership(userID string, postID string, opts sto
return r0, r1
}
// MarkAllAsRead provides a mock function with given fields: userID, teamID
func (_m *ThreadStore) MarkAllAsRead(userID string, teamID string) error {
ret := _m.Called(userID, teamID)
// MarkAllAsRead provides a mock function with given fields: userID, threadIds
func (_m *ThreadStore) MarkAllAsRead(userID string, threadIds []string) error {
ret := _m.Called(userID, threadIds)
var r0 error
if rf, ok := ret.Get(0).(func(string, string) error); ok {
r0 = rf(userID, teamID)
if rf, ok := ret.Get(0).(func(string, []string) error); ok {
r0 = rf(userID, threadIds)
} else {
r0 = ret.Error(0)
}
@@ -315,8 +292,8 @@ func (_m *ThreadStore) MarkAllAsRead(userID string, teamID string) error {
return r0
}
// MarkAllAsReadInChannels provides a mock function with given fields: userID, channelIDs
func (_m *ThreadStore) MarkAllAsReadInChannels(userID string, channelIDs []string) error {
// MarkAllAsReadByChannels provides a mock function with given fields: userID, channelIDs
func (_m *ThreadStore) MarkAllAsReadByChannels(userID string, channelIDs []string) error {
ret := _m.Called(userID, channelIDs)
var r0 error
@@ -329,6 +306,20 @@ func (_m *ThreadStore) MarkAllAsReadInChannels(userID string, channelIDs []strin
return r0
}
// MarkAllAsReadByTeam provides a mock function with given fields: userID, teamID
func (_m *ThreadStore) MarkAllAsReadByTeam(userID string, teamID string) error {
ret := _m.Called(userID, teamID)
var r0 error
if rf, ok := ret.Get(0).(func(string, string) error); ok {
r0 = rf(userID, teamID)
} else {
r0 = ret.Error(0)
}
return r0
}
// MarkAsRead provides a mock function with given fields: userID, threadID, timestamp
func (_m *ThreadStore) MarkAsRead(userID string, threadID string, timestamp int64) error {
ret := _m.Called(userID, threadID, timestamp)
@@ -399,20 +390,6 @@ func (_m *ThreadStore) PermanentDeleteBatchThreadMembershipsForRetentionPolicies
return r0, r1, r2
}
// UpdateLastViewedByThreadIds provides a mock function with given fields: userId, threadIds, timestamp
func (_m *ThreadStore) UpdateLastViewedByThreadIds(userId string, threadIds []string, timestamp int64) error {
ret := _m.Called(userId, threadIds, timestamp)
var r0 error
if rf, ok := ret.Get(0).(func(string, []string, int64) error); ok {
r0 = rf(userId, threadIds, timestamp)
} else {
r0 = ret.Error(0)
}
return r0
}
// UpdateMembership provides a mock function with given fields: membership
func (_m *ThreadStore) UpdateMembership(membership *model.ThreadMembership) (*model.ThreadMembership, error) {
ret := _m.Called(membership)

175
store/storetest/thread_store.go
Просмотреть файл

@@ -6,7 +6,6 @@ package storetest
import (
"context"
"testing"
"time"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
@@ -24,6 +23,7 @@ func TestThreadStore(t *testing.T, ss store.Store, s SqlStore) {
testThreadStorePermanentDeleteBatchThreadMembershipsForRetentionPolicies(t, ss, s)
})
t.Run("GetTeamsUnreadForUser", func(t *testing.T) { testGetTeamsUnreadForUser(t, ss) })
t.Run("MarkAllAsReadByChannels", func(t *testing.T) { testMarkAllAsReadByChannels(t, ss) })
}
func testThreadStorePopulation(t *testing.T, ss store.Store) {
@@ -262,33 +262,6 @@ func testThreadStorePopulation(t *testing.T, ss store.Store) {
require.Nil(t, thread2)
})
t.Run("Thread last updated is changed when channel is updated after UpdateLastViewedAt", func(t *testing.T) {
newPosts := makeSomePosts()
opts := store.ThreadMembershipOpts{
Following: true,
IncrementMentions: false,
UpdateFollowing: true,
UpdateViewedTimestamp: false,
UpdateParticipants: false,
}
_, e := ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, opts)
require.NoError(t, e)
m, err1 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id)
require.NoError(t, err1)
m.LastUpdated -= 1000
_, err := ss.Thread().UpdateMembership(m)
require.NoError(t, err)
_, err = ss.Channel().UpdateLastViewedAt([]string{newPosts[0].ChannelId}, newPosts[0].UserId, true)
require.NoError(t, err)
assert.Eventually(t, func() bool {
m2, err2 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id)
require.NoError(t, err2)
return m2.LastUpdated > m.LastUpdated
}, time.Second, 10*time.Millisecond)
})
t.Run("Thread membership 'viewed' timestamp is updated properly", func(t *testing.T) {
newPosts := makeSomePosts()
@@ -421,6 +394,8 @@ func testThreadStorePopulation(t *testing.T, ss store.Store) {
}
func threadStoreCreateReply(t *testing.T, ss store.Store, channelID, postID, userID string, createAt int64) *model.Post {
t.Helper()
reply, err := ss.Post().Save(&model.Post{
ChannelId: channelID,
UserId: userID,
@@ -702,3 +677,147 @@ func testGetTeamsUnreadForUser(t *testing.T, ss store.Store) {
assert.Equal(t, int64(1), teamsUnread[team2.Id].ThreadCount)
assert.Equal(t, int64(1), teamsUnread[team2.Id].ThreadMentionCount)
}
func testMarkAllAsReadByChannels(t *testing.T, ss store.Store) {
postingUserId := model.NewId()
userAID := model.NewId()
userBID := model.NewId()
team1, err := ss.Team().Save(&model.Team{
DisplayName: "Team1",
Name: "team" + model.NewId(),
Email: MakeEmail(),
Type: model.TeamOpen,
})
require.NoError(t, err)
channel1, err := ss.Channel().Save(&model.Channel{
TeamId: team1.Id,
DisplayName: "Channel1",
Name: "channel1" + model.NewId(),
Type: model.ChannelTypeOpen,
}, -1)
require.NoError(t, err)
channel2, err := ss.Channel().Save(&model.Channel{
TeamId: team1.Id,
DisplayName: "Channel2",
Name: "channel2" + model.NewId(),
Type: model.ChannelTypeOpen,
}, -1)
require.NoError(t, err)
createThreadMembership := func(userID, postID string) {
t.Helper()
opts := store.ThreadMembershipOpts{
Following: true,
IncrementMentions: false,
UpdateFollowing: true,
UpdateViewedTimestamp: false,
UpdateParticipants: false,
}
_, err := ss.Thread().MaintainMembership(userID, postID, opts)
require.NoError(t, err)
}
assertThreadReplyCount := func(t *testing.T, userID string, count int64) {
t.Helper()
teamsUnread, err := ss.Thread().GetTeamsUnreadForUser(userID, []string{team1.Id})
require.NoError(t, err)
require.Len(t, teamsUnread, 1, "unexpected unread teams count")
assert.Equal(t, count, teamsUnread[team1.Id].ThreadCount, "unexpected thread count")
}
t.Run("empty set of channels", func(t *testing.T) {
err := ss.Thread().MarkAllAsReadByChannels(model.NewId(), []string{})
require.NoError(t, err)
})
t.Run("single channel", func(t *testing.T) {
post, err := ss.Post().Save(&model.Post{
ChannelId: channel1.Id,
UserId: postingUserId,
Message: "Root",
})
require.NoError(t, err)
_, err = ss.Post().Save(&model.Post{
ChannelId: channel1.Id,
UserId: postingUserId,
RootId: post.Id,
Message: "Reply",
})
require.NoError(t, err)
createThreadMembership(userAID, post.Id)
createThreadMembership(userBID, post.Id)
assertThreadReplyCount(t, userAID, 1)
assertThreadReplyCount(t, userBID, 1)
err = ss.Thread().MarkAllAsReadByChannels(userAID, []string{channel1.Id})
require.NoError(t, err)
assertThreadReplyCount(t, userAID, 0)
assertThreadReplyCount(t, userBID, 1)
err = ss.Thread().MarkAllAsReadByChannels(userBID, []string{channel1.Id})
require.NoError(t, err)
assertThreadReplyCount(t, userAID, 0)
assertThreadReplyCount(t, userBID, 0)
})
t.Run("multiple channels", func(t *testing.T) {
post1, err := ss.Post().Save(&model.Post{
ChannelId: channel1.Id,
UserId: postingUserId,
Message: "Root",
})
require.NoError(t, err)
_, err = ss.Post().Save(&model.Post{
ChannelId: channel1.Id,
UserId: postingUserId,
RootId: post1.Id,
Message: "Reply",
})
require.NoError(t, err)
post2, err := ss.Post().Save(&model.Post{
ChannelId: channel2.Id,
UserId: postingUserId,
Message: "Root",
})
require.NoError(t, err)
_, err = ss.Post().Save(&model.Post{
ChannelId: channel2.Id,
UserId: postingUserId,
RootId: post2.Id,
Message: "Reply",
})
require.NoError(t, err)
createThreadMembership(userAID, post1.Id)
createThreadMembership(userBID, post1.Id)
createThreadMembership(userAID, post2.Id)
createThreadMembership(userBID, post2.Id)
assertThreadReplyCount(t, userAID, 2)
assertThreadReplyCount(t, userBID, 2)
err = ss.Thread().MarkAllAsReadByChannels(userAID, []string{channel1.Id, channel2.Id})
require.NoError(t, err)
assertThreadReplyCount(t, userAID, 0)
assertThreadReplyCount(t, userBID, 2)
err = ss.Thread().MarkAllAsReadByChannels(userBID, []string{channel1.Id, channel2.Id})
require.NoError(t, err)
assertThreadReplyCount(t, userAID, 0)
assertThreadReplyCount(t, userBID, 0)
})
}