chore(popx): improve migration performance by removing unnecessary queries#730
chore(popx): improve migration performance by removing unnecessary queries#730
Conversation
alnr
left a comment
alnr
left a comment
There was a problem hiding this comment.
LGTM. Will it actually make a difference, tho? We'll see I guess 😁
| } | ||
|
|
||
| if exists { | ||
| if _, exists := appliedMigrationsMap[mi.Version]; exists { |
There was a problem hiding this comment.
Theoretically we could end up with an outdated view of already applied migrations, for example a migration that is applied 2 times (for whatever reason) or because there are more than one migration workers. Are we OK accepting that risk for the benefit it brings?
There was a problem hiding this comment.
Yes, theoretically this could be an outdated view.
The benefit will probably be a few hundred ms up to maybe 2s from my testing. This happens a couple of times per test run.
If you think this is a possible source of issues it is probably worth the extra few hundred ms in tests though.
There was a problem hiding this comment.
Sorry, I missed replying to this. I'm not sure to be honest. Migrations should be run as singletons but I guess the question is how do we deal with it if it isn't - so in the case of an operational mistake. Not sure to be honest...
|
I like this PR. Why was it closed? @zepatrik |
3 participants
This is most relevant during tests, but also Kratos has so many migrations it will significantly speedup it's migration command.