search

mattermost / mattermost-plugin-msteams

MS Teams plugin for Mattermost
Other
13 stars 11 forks source link

MM-59466 remove unused migrations #711

Closed lieut-data closed 4 months ago

lieut-data commented 4 months ago

Summary

This pull request strips out unneeded migrations (keeping their keys as legacy strings so we avoid accidentally reusing them.

Ticket Link

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

lieut-data commented 4 months ago

Looks good, just wondering what kind of impact this would have on a big deployment like SNOW?

Good call out, @JulienTant! SNOW (and Hub) are specifically in mind here to allow us to transition away from the existing data model and "clean up" all those synthetic users, but you're right that this is likely to take some time. I'm aiming to coordinate explicitly with SRE to manage this upgrade.

lieut-data commented 4 months ago

Instead of looping and calling the pluginAPI for DeleteUser, which could be a performance issue, why not just handle via SQL Queries.

I understand that if these were "normal" users why you would do this. But all of the additional things that this function handles, such as invalidating caches and sending web socket events, revoking sessions, calling other plugins, are performance hits that I don't believe are necessary for these users.

I do agree we might be able to get away with a more manual deletion, but I'm uncertain of the enduser side effects. For example, these users might be in client-side caches, but won't get "refreshed" without the websocket events. I'm hoping that if we just do this in Cloud during downtime, we won't need to deal with unexpected fallout of direct database manipulation.

lieut-data commented 4 months ago

Raising this with SRE over at https://hub.mattermost.com/private-core/pl/t94auqbwojbqbezzqd455exrra.

lieut-data commented 4 months ago

@sbishel & @JulienTant, with the plan to manually transition Cloud customers, I'm leaning towards possibly just removing unneeded migrations and documenting how to achieve this same effect instead of running a migration at all. Thoughts?

sbishel commented 4 months ago

@sbishel & @JulienTant, with the plan to manually transition Cloud customers, I'm leaning towards possibly just removing unneeded migrations and documenting how to achieve this same effect instead of running a migration at all. Thoughts?

Yeah. Might be better, allows cleaning up the database without installing the new plugin. We don't have too many that we know about either.