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

Replace refinery with our own backwards compatible migration code #932

Closed
wants to merge 2 commits into from

Conversation

serprex
Copy link
Contributor

@serprex serprex commented Dec 29, 2023

Currently server_tests must be run with --test-threads=1 because of two things:

  1. port collision
  2. concurrent migrations failing

I explored fixing these in #926. Ports are easily solved, but migrations are a pain

This migration code works around concurrency with two changes from refinery:

  1. ignore unique constraint violation from CREATE TABLE IF NOT EXISTS
  2. lock migration table so concurrent migrations are serialized

Considered submitting a PR to refinery with these two fixes,
but this simple change was non trivial since they support multiple async/sync database drivers

@serprex
Copy link
Contributor Author

serprex commented Dec 29, 2023

Two aspects are implemented more loosely here than in refinery:

  1. making sure versions are contiguous (we only check that the applied migrations have matching order)
  2. comparing checksum, version/name should be enough

@serprex serprex force-pushed the remove-refinery branch 2 times, most recently from f9abbc7 to d2915f1 Compare December 29, 2023 16:55
@serprex serprex marked this pull request as ready for review December 29, 2023 17:01
@serprex serprex requested a review from iskakaushik December 29, 2023 17:01
@serprex
Copy link
Contributor Author

serprex commented Dec 29, 2023

@iskakaushik is refinery-cli being used to manage customer migrations? if so, I can look to get this upstreamed

@serprex serprex force-pushed the remove-refinery branch 5 times, most recently from 78fb996 to 0cb4e98 Compare December 29, 2023 18:05
Currently server_tests must be run with --test-threads=1 because of two things:
1. port collision
2. concurrent migrations failing

I explored fixing these in #926. Ports are easily solved, but migrations are a pain

This migration code works around concurrency with two changes from refinery:
1. ignore unique constraint violation from CREATE TABLE IF NOT EXISTS
2. lock migration table so concurrent migrations are serialized

Considered submitting a PR to refinery with these two fixes,
but this simple change was non trivial since they support multiple async/sync database drivers
@serprex
Copy link
Contributor Author

serprex commented Dec 31, 2023

#926 passes after being rebased onto this PR

@serprex
Copy link
Contributor Author

serprex commented Feb 5, 2024

Spoke with Kaushik, rolling our own database migration code is too far into yak shaving, so will have to get this moved upstream

@serprex serprex closed this Feb 5, 2024
@serprex serprex deleted the remove-refinery branch July 19, 2024 15:25
@serprex serprex restored the remove-refinery branch July 19, 2024 15:25
@serprex serprex deleted the remove-refinery branch July 19, 2024 15:25
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.

1 participant