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

Dselans/concurrency support #58

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dselans
Copy link

@dselans dselans commented Feb 20, 2017

Hey folks,

So this is a semi-large PR for functionality that was requested in #45. We ran into the exact same need -- our applications run in docker containers on an ephemeral set of nodes and perform in-app migrations. The sql-migrate lib is fantastic, but when used concurrently, you are likely to run into migration collisions. Someone mentioned that this should be handled at a different layer and while that can be done, we have little control over the ops side of things and adding "cluster awareness" to each different service seems .. hacky and difficult to scale (if we're talking about microservices).

#45 suggested to use vendor-specific lock implementations -- while probably doable (and "proper"), it would be a pretty large undertaking. I went with the model of using a db-based mutex instead (ie. a lock record in a gorp_lock table), that all "migrators" attempt to insert, but only one wins. This ensures that it should work across most db's, without using any vendor-specific lock code (except for sqlite, more on that later).

Implementation and notes:

  • Introduced two new functions ExecWithLock() and ExecMaxWithLock() which will utilize a db mutex to ensure only one 'master' migrator is performing migrations
  • Any sql-migrate instances that were not able to acquire the lock go into a wait-state, periodically checking to see if the lock disappears.
    • If the lock disappears before waitTime is up - the 'wait state' migrators finish cleanly, with no errors (and 0 completed migrations)
    • If the lock does not get removed before waitTime is up - the additional sql-migrate instances will error with a Exceeded lock clearance wait time message. You can then tune the waitTime accordingly (it's a param passed to ExecWithLock())
  • In the process of updating tests, I realized that the lock implementation won't work with sqlite due to concurrent access causing database is locked errors. It's totally possible that I just missed something obvious, but the easiest route seemed to be to just enable MySQL based integration tests, so I introduced another test suite (MySQLMigrateSuite), which thoroughly tests the lock implementation.
    • MySQL based testing can be enabled via a new flag passed to go test called -enable-mysql
    • SQLite tests can now also be disabled via -disable-sqlite (default: false); this was done to speed up testing (as the sqlite tests seemed to be pretty slow on my machine)
  • I also went ahead and updated the .travis.yml to enable mysql based testing as well

With that said, please let me know if you've got any questions -- this functionality is really crucial for our team and I would much rather prefer running off of upstream than our little fork :-)

TIA!

- Introduced two new funcs: `ExecMaxWithLock()` and `ExecWithLock`
- Use a db-based mutex (to avoid getting locked into supporting vendor-specific lock implementations)
- I was getting 'database is locked' errors when writing tests using sqlite3 -- this appears to be normal (?) behavior due to sqlite and concurrent access; added an additional test suite specifically most 'mysql', which worked as expected.
- Exposed a test flags `-enable-mysql` (default: false) and `-disable-sqlite` (default: false)
@rubenv
Copy link
Owner

rubenv commented Mar 7, 2017

Hi, sorry for the late reply.

As mentioned in #45, I don't think this functionality has a place in sql-migrate, there's too many different ways to do this and I want to keep things simple.

There's nothing stopping you from putting this functionality in a class that decorates the functionality from sql-migrate, it can be perfectly separate.

@dselans
Copy link
Author

dselans commented Mar 7, 2017

Hi @rubenv, thanks for your update.

I suspect more and more folks are launching their apps in parallel -- it would be nice to support this sort of functionality out of the box (without requiring everyone to write their own, custom wrappers). The functionality I added does not modify any existing code - it adds two new functions, so folks don't have to use it if they don't want to.

With that said, this is your project, if you do not want to take on the potential tech debt, that's understandable. I still feel that others may benefit from this, so I'll keep the alternate fork running.

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.

2 participants