Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(popx): improve migration performance by removing unnecessary queries #730

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zepatrik
Copy link
Member

This is most relevant during tests, but also Kratos has so many migrations it will significantly speedup it's migration command.

Copy link
Contributor

@alnr alnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Will it actually make a difference, tho? We'll see I guess 😁

}

if exists {
if _, exists := appliedMigrationsMap[mi.Version]; exists {
Copy link
Member

@aeneasr aeneasr Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@zepatrik zepatrik Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants