[MM-53428] Delete empty drafts on upsert (#24046)

* [MM-53428] Delete empty drafts on upsert

* Add migrations to fix existing drafts

* Fix CI

* Delete empty drafts entirely from the DB

* Fix lint

* Implement batch migration for deleting drafts

* Missing store layers

* Add updated mock

* Remove unnecessary test

* PR feedback

* Add check for cluster migration

* Fix MySQL

* Don't check for len<2

* Bit of PR feedback

* Use query builder for parameters

* PR feedback

* More PR feedback

* Merge'd

* unit test GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration

* simplified builder interface

* fix DeleteEmptyDraftsByCreateAtAndUserId for MySQL

* rework as batch migration worker

* fix typo

* log ip address on version mismatches too

* simplify reset semantics

* remove trace log in favour of low spam

* document parameters for clarity

---------

Co-authored-by: Mattermost Build <build@mattermost.com>
Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>
Этот коммит содержится в:
Devin Binnie
2023-10-12 10:52:10 -04:00
коммит произвёл GitHub
родитель 760dfe41f9
Коммит 89492a6a46
22 изменённых файлов: 1573 добавлений и 26 удалений

Просмотреть файл

@@ -3335,6 +3335,24 @@ func (s *OpenTracingLayerDraftStore) Delete(userID string, channelID string, roo
return err
}
func (s *OpenTracingLayerDraftStore) DeleteEmptyDraftsByCreateAtAndUserId(createAt int64, userId string) error {
origCtx := s.Root.Store.Context()
span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "DraftStore.DeleteEmptyDraftsByCreateAtAndUserId")
s.Root.Store.SetContext(newCtx)
defer func() {
s.Root.Store.SetContext(origCtx)
}()
defer span.Finish()
err := s.DraftStore.DeleteEmptyDraftsByCreateAtAndUserId(createAt, userId)
if err != nil {
span.LogFields(spanlog.Error(err))
ext.Error.Set(span, true)
}
return err
}
func (s *OpenTracingLayerDraftStore) Get(userID string, channelID string, rootID string, includeDeleted bool) (*model.Draft, error) {
origCtx := s.Root.Store.Context()
span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "DraftStore.Get")
@@ -3371,6 +3389,24 @@ func (s *OpenTracingLayerDraftStore) GetDraftsForUser(userID string, teamID stri
return result, err
}
func (s *OpenTracingLayerDraftStore) GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt int64, userId string) (int64, string, error) {
origCtx := s.Root.Store.Context()
span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "DraftStore.GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration")
s.Root.Store.SetContext(newCtx)
defer func() {
s.Root.Store.SetContext(origCtx)
}()
defer span.Finish()
result, resultVar1, err := s.DraftStore.GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
if err != nil {
span.LogFields(spanlog.Error(err))
ext.Error.Set(span, true)
}
return result, resultVar1, err
}
func (s *OpenTracingLayerDraftStore) Upsert(d *model.Draft) (*model.Draft, error) {
origCtx := s.Root.Store.Context()
span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "DraftStore.Upsert")

Просмотреть файл

@@ -3724,6 +3724,27 @@ func (s *RetryLayerDraftStore) Delete(userID string, channelID string, rootID st
}
func (s *RetryLayerDraftStore) DeleteEmptyDraftsByCreateAtAndUserId(createAt int64, userId string) error {
tries := 0
for {
err := s.DraftStore.DeleteEmptyDraftsByCreateAtAndUserId(createAt, userId)
if err == nil {
return nil
}
if !isRepeatableError(err) {
return err
}
tries++
if tries >= 3 {
err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures")
return err
}
timepkg.Sleep(100 * timepkg.Millisecond)
}
}
func (s *RetryLayerDraftStore) Get(userID string, channelID string, rootID string, includeDeleted bool) (*model.Draft, error) {
tries := 0
@@ -3766,6 +3787,27 @@ func (s *RetryLayerDraftStore) GetDraftsForUser(userID string, teamID string) ([
}
func (s *RetryLayerDraftStore) GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt int64, userId string) (int64, string, error) {
tries := 0
for {
result, resultVar1, err := s.DraftStore.GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
if err == nil {
return result, resultVar1, nil
}
if !isRepeatableError(err) {
return result, resultVar1, err
}
tries++
if tries >= 3 {
err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures")
return result, resultVar1, err
}
timepkg.Sleep(100 * timepkg.Millisecond)
}
}
func (s *RetryLayerDraftStore) Upsert(d *model.Draft) (*model.Draft, error) {
tries := 0

Просмотреть файл

@@ -159,11 +159,8 @@ func (s *SqlDraftStore) GetDraftsForUser(userID, teamID string) ([]*model.Draft,
}
func (s *SqlDraftStore) Delete(userID, channelID, rootID string) error {
time := model.GetMillis()
query := s.getQueryBuilder().
Update("Drafts").
Set("UpdateAt", time).
Set("DeleteAt", time).
Delete("Drafts").
Where(sq.Eq{
"UserId": userID,
"ChannelId": channelID,
@@ -236,3 +233,90 @@ func (s *SqlDraftStore) determineMaxDraftSize() int {
return maxDraftSize
}
func (s *SqlDraftStore) GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt int64, userId string) (int64, string, error) {
var drafts []struct {
CreateAt int64
UserId string
}
query := s.getQueryBuilder().
Select("CreateAt", "UserId").
From("Drafts").
Where(sq.Or{
sq.Gt{"CreateAt": createAt},
sq.And{
sq.Eq{"CreateAt": createAt},
sq.Gt{"UserId": userId},
},
}).
OrderBy("CreateAt", "UserId ASC").
Limit(100)
err := s.GetReplicaX().SelectBuilder(&drafts, query)
if err != nil {
return 0, "", errors.Wrap(err, "failed to get the list of drafts")
}
if len(drafts) == 0 {
return 0, "", nil
}
lastElement := drafts[len(drafts)-1]
return lastElement.CreateAt, lastElement.UserId, nil
}
func (s *SqlDraftStore) DeleteEmptyDraftsByCreateAtAndUserId(createAt int64, userId string) error {
var builder Builder
if s.DriverName() == model.DatabaseDriverPostgres {
builder = s.getQueryBuilder().
Delete("Drafts d").
PrefixExpr(s.getQueryBuilder().Select().
Prefix("WITH dd AS (").
Columns("UserId", "ChannelId", "RootId").
From("Drafts").
Where(sq.Or{
sq.Gt{"CreateAt": createAt},
sq.And{
sq.Eq{"CreateAt": createAt},
sq.Gt{"UserId": userId},
},
}).
OrderBy("CreateAt", "UserId").
Limit(100).
Suffix(")"),
).
Using("dd").
Where("d.UserId = dd.UserId").
Where("d.ChannelId = dd.ChannelId").
Where("d.RootId = dd.RootId").
Where("d.Message = ''")
} else if s.DriverName() == model.DatabaseDriverMysql {
builder = s.getQueryBuilder().
Delete("Drafts d").
What("d.*").
JoinClause(s.getQueryBuilder().Select().
Prefix("INNER JOIN (").
Columns("UserId, ChannelId, RootId").
From("Drafts").
Where(sq.And{
sq.Or{
sq.Gt{"CreateAt": createAt},
sq.And{
sq.Eq{"CreateAt": createAt},
sq.Gt{"UserId": userId},
},
},
}).
OrderBy("CreateAt", "UserId").
Limit(100).
Suffix(") dj ON (d.UserId = dj.UserId AND d.ChannelId = dj.ChannelId AND d.RootId = dj.RootId)"),
).Where(sq.Eq{"Message": ""})
}
if _, err := s.GetMasterX().ExecBuilder(builder); err != nil {
return errors.Wrapf(err, "failed to delete empty drafts")
}
return nil
}

Просмотреть файл

@@ -986,6 +986,8 @@ type DraftStore interface {
Get(userID, channelID, rootID string, includeDeleted bool) (*model.Draft, error)
Delete(userID, channelID, rootID string) error
GetDraftsForUser(userID, teamID string) ([]*model.Draft, error)
GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt int64, userId string) (int64, string, error)
DeleteEmptyDraftsByCreateAtAndUserId(createAt int64, userId string) error
}
type PostAcknowledgementStore interface {

Просмотреть файл

@@ -5,6 +5,7 @@ package storetest
import (
"testing"
"time"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
@@ -19,6 +20,8 @@ func TestDraftStore(t *testing.T, ss store.Store, s SqlStore) {
t.Run("DeleteDraft", func(t *testing.T) { testDeleteDraft(t, ss) })
t.Run("GetDraft", func(t *testing.T) { testGetDraft(t, ss) })
t.Run("GetDraftsForUser", func(t *testing.T) { testGetDraftsForUser(t, ss) })
t.Run("GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration", func(t *testing.T) { testGetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(t, ss) })
t.Run("DeleteEmptyDraftsByCreateAtAndUserId", func(t *testing.T) { testDeleteEmptyDraftsByCreateAtAndUserId(t, ss) })
}
func testSaveDraft(t *testing.T, ss store.Store) {
@@ -275,24 +278,6 @@ func testGetDraft(t *testing.T, ss store.Store) {
assert.Equal(t, draft2.Message, draftResp.Message)
assert.Equal(t, draft2.ChannelId, draftResp.ChannelId)
})
t.Run("get draft including deleted", func(t *testing.T) {
draftResp, err := ss.Draft().Get(user.Id, channel.Id, "", false)
assert.NoError(t, err)
assert.Equal(t, draft1.Message, draftResp.Message)
assert.Equal(t, draft1.ChannelId, draftResp.ChannelId)
err = ss.Draft().Delete(user.Id, channel.Id, "")
assert.NoError(t, err)
_, err = ss.Draft().Get(user.Id, channel.Id, "", false)
assert.Error(t, err)
assert.IsType(t, &store.ErrNotFound{}, err)
draftResp, err = ss.Draft().Get(user.Id, channel.Id, "", true)
assert.NoError(t, err)
assert.Equal(t, draft1.Message, draftResp.Message)
assert.Equal(t, draft1.ChannelId, draftResp.ChannelId)
})
}
func testGetDraftsForUser(t *testing.T, ss store.Store) {
@@ -351,3 +336,226 @@ func testGetDraftsForUser(t *testing.T, ss store.Store) {
assert.ElementsMatch(t, []*model.Draft{draft1, draft2}, draftResp)
})
}
func clearDrafts(t *testing.T, ss store.Store) {
t.Helper()
_, err := ss.GetInternalMasterDB().Exec("DELETE FROM Drafts")
require.NoError(t, err)
}
func makeDrafts(t *testing.T, ss store.Store, count int, message string) {
t.Helper()
var delay time.Duration
if count > 100 {
// When creating more than one page of drafts, improve the odds we get
// some results with different CreateAt timetsamps.
delay = 5 * time.Millisecond
}
for i := 1; i <= count; i++ {
_, err := ss.Draft().Upsert(&model.Draft{
CreateAt: model.GetMillis(),
UpdateAt: model.GetMillis(),
UserId: model.NewId(),
ChannelId: model.NewId(),
Message: message,
})
require.NoError(t, err)
if delay > 0 {
time.Sleep(delay)
}
}
}
func countDraftPages(t *testing.T, ss store.Store) int {
t.Helper()
pages := 0
createAt := int64(0)
userId := ""
for {
nextCreateAt, nextUserId, err := ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
if nextCreateAt == 0 && nextUserId == "" {
break
}
// Ensure we're always making progress.
if nextCreateAt == createAt {
require.Greater(t, nextUserId, userId)
} else {
require.Greater(t, nextCreateAt, createAt)
}
pages++
createAt = nextCreateAt
userId = nextUserId
}
return pages
}
func testGetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(t *testing.T, ss store.Store) {
t.Run("no drafts", func(t *testing.T) {
clearDrafts(t, ss)
createAt, userId, err := ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(0, "")
require.NoError(t, err)
assert.EqualValues(t, 0, createAt)
assert.Equal(t, "", userId)
assert.Equal(t, 0, countDraftPages(t, ss), "incorrect number of pages")
})
t.Run("single page", func(t *testing.T) {
clearDrafts(t, ss)
makeDrafts(t, ss, 100, model.NewRandomString(16))
assert.Equal(t, 1, countDraftPages(t, ss), "incorrect number of pages")
})
t.Run("multiple pages", func(t *testing.T) {
clearDrafts(t, ss)
makeDrafts(t, ss, 300, model.NewRandomString(16))
assert.Equal(t, 3, countDraftPages(t, ss), "incorrect number of pages")
})
}
func testDeleteEmptyDraftsByCreateAtAndUserId(t *testing.T, ss store.Store) {
t.Run("nil parameters", func(t *testing.T) {
clearDrafts(t, ss)
err := ss.Draft().DeleteEmptyDraftsByCreateAtAndUserId(0, "")
require.NoError(t, err)
})
t.Run("delete single page, all empty", func(t *testing.T) {
clearDrafts(t, ss)
makeDrafts(t, ss, 100, "")
createAt, userId := int64(0), ""
nextCreateAt, nextUserId, err := ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteEmptyDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
assert.Equal(t, 0, countDraftPages(t, ss), "incorrect number of pages")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
assert.EqualValues(t, 0, nextCreateAt, "should have finished iterating through drafts")
assert.Equal(t, "", nextUserId, "should have finished iterating through drafts")
})
t.Run("delete multiple pages, all empty", func(t *testing.T) {
clearDrafts(t, ss)
makeDrafts(t, ss, 300, "")
createAt, userId := int64(0), ""
nextCreateAt, nextUserId, err := ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteEmptyDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
assert.Equal(t, 2, countDraftPages(t, ss), "incorrect number of pages")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteEmptyDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
assert.Equal(t, 1, countDraftPages(t, ss), "incorrect number of pages")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteEmptyDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
assert.Equal(t, 0, countDraftPages(t, ss), "incorrect number of pages")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
assert.EqualValues(t, 0, nextCreateAt, "should have finished iterating through drafts")
assert.Equal(t, "", nextUserId, "should have finished iterating through drafts")
})
t.Run("delete multiple pages, some empty", func(t *testing.T) {
clearDrafts(t, ss)
makeDrafts(t, ss, 50, "")
makeDrafts(t, ss, 50, "message")
makeDrafts(t, ss, 50, "")
makeDrafts(t, ss, 50, "message")
makeDrafts(t, ss, 50, "")
makeDrafts(t, ss, 50, "message")
makeDrafts(t, ss, 50, "")
makeDrafts(t, ss, 50, "message")
makeDrafts(t, ss, 50, "message")
makeDrafts(t, ss, 50, "message")
// Verify initially 5 pages
assert.Equal(t, 5, countDraftPages(t, ss), "incorrect number of pages")
createAt, userId := int64(0), ""
nextCreateAt, nextUserId, err := ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteEmptyDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
// Only deleted 50, so still 5 pages
assert.Equal(t, 5, countDraftPages(t, ss), "incorrect number of pages")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteEmptyDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
// Now deleted 100, so down to 4 pages
assert.Equal(t, 4, countDraftPages(t, ss), "incorrect number of pages")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteEmptyDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
// Only deleted 150 now, so still 4 pages
assert.Equal(t, 4, countDraftPages(t, ss), "incorrect number of pages")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteEmptyDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
// Now deleted all 200 empty messages, so down to 3 pages
assert.Equal(t, 3, countDraftPages(t, ss), "incorrect number of pages")
// Keep going through all pages to verify nothing else gets deleted.
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteEmptyDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
// Verify we're done iterating
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
assert.EqualValues(t, 0, nextCreateAt, "should have finished iterating through drafts")
assert.Equal(t, "", nextUserId, "should have finished iterating through drafts")
})
}

Просмотреть файл

@@ -28,6 +28,20 @@ func (_m *DraftStore) Delete(userID string, channelID string, rootID string) err
return r0
}
// DeleteEmptyDraftsByCreateAtAndUserId provides a mock function with given fields: createAt, userId
func (_m *DraftStore) DeleteEmptyDraftsByCreateAtAndUserId(createAt int64, userId string) error {
ret := _m.Called(createAt, userId)
var r0 error
if rf, ok := ret.Get(0).(func(int64, string) error); ok {
r0 = rf(createAt, userId)
} else {
r0 = ret.Error(0)
}
return r0
}
// Get provides a mock function with given fields: userID, channelID, rootID, includeDeleted
func (_m *DraftStore) Get(userID string, channelID string, rootID string, includeDeleted bool) (*model.Draft, error) {
ret := _m.Called(userID, channelID, rootID, includeDeleted)
@@ -80,6 +94,37 @@ func (_m *DraftStore) GetDraftsForUser(userID string, teamID string) ([]*model.D
return r0, r1
}
// GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration provides a mock function with given fields: createAt, userId
func (_m *DraftStore) GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt int64, userId string) (int64, string, error) {
ret := _m.Called(createAt, userId)
var r0 int64
var r1 string
var r2 error
if rf, ok := ret.Get(0).(func(int64, string) (int64, string, error)); ok {
return rf(createAt, userId)
}
if rf, ok := ret.Get(0).(func(int64, string) int64); ok {
r0 = rf(createAt, userId)
} else {
r0 = ret.Get(0).(int64)
}
if rf, ok := ret.Get(1).(func(int64, string) string); ok {
r1 = rf(createAt, userId)
} else {
r1 = ret.Get(1).(string)
}
if rf, ok := ret.Get(2).(func(int64, string) error); ok {
r2 = rf(createAt, userId)
} else {
r2 = ret.Error(2)
}
return r0, r1, r2
}
// Upsert provides a mock function with given fields: d
func (_m *DraftStore) Upsert(d *model.Draft) (*model.Draft, error) {
ret := _m.Called(d)

Просмотреть файл

@@ -3059,6 +3059,22 @@ func (s *TimerLayerDraftStore) Delete(userID string, channelID string, rootID st
return err
}
func (s *TimerLayerDraftStore) DeleteEmptyDraftsByCreateAtAndUserId(createAt int64, userId string) error {
start := time.Now()
err := s.DraftStore.DeleteEmptyDraftsByCreateAtAndUserId(createAt, userId)
elapsed := float64(time.Since(start)) / float64(time.Second)
if s.Root.Metrics != nil {
success := "false"
if err == nil {
success = "true"
}
s.Root.Metrics.ObserveStoreMethodDuration("DraftStore.DeleteEmptyDraftsByCreateAtAndUserId", success, elapsed)
}
return err
}
func (s *TimerLayerDraftStore) Get(userID string, channelID string, rootID string, includeDeleted bool) (*model.Draft, error) {
start := time.Now()
@@ -3091,6 +3107,22 @@ func (s *TimerLayerDraftStore) GetDraftsForUser(userID string, teamID string) ([
return result, err
}
func (s *TimerLayerDraftStore) GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt int64, userId string) (int64, string, error) {
start := time.Now()
result, resultVar1, err := s.DraftStore.GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
elapsed := float64(time.Since(start)) / float64(time.Second)
if s.Root.Metrics != nil {
success := "false"
if err == nil {
success = "true"
}
s.Root.Metrics.ObserveStoreMethodDuration("DraftStore.GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration", success, elapsed)
}
return result, resultVar1, err
}
func (s *TimerLayerDraftStore) Upsert(d *model.Draft) (*model.Draft, error) {
start := time.Now()