Improvements for migration table locking #109
Conversation
rPraml
left a comment
rPraml
left a comment
There was a problem hiding this comment.
For MySql/MariaDb it would be possible to obtain the lock BEFORE trying to create the table, but I didn't want to change thant now
| private boolean obtainNamedLock(Connection connection) throws SQLException { | ||
| try (PreparedStatement query = connection.prepareStatement("select get_lock('ebean_migration', 10)")) { | ||
| String hash = Integer.toHexString(connection.getMetaData().getURL().hashCode()); | ||
| try (PreparedStatement query = connection.prepareStatement("select get_lock('ebean_migration-" + hash + "', 10)")) { |
There was a problem hiding this comment.
this makes an application wide lock on the server. This means, if one ebean application runs a migration, no other ebean app (in different db) can run its migration.
I would suggest only to lock the database.
We have a multi tenat setup and we plan to run migration in different tenants parallel.
| try { | ||
| obtainLockWithWait(); | ||
| } catch (RuntimeException re) { | ||
| // catch "failed to obtain row locks" |
There was a problem hiding this comment.
This fixes an issue, when createTableDdl succeeds, but createInitMetaRow will fail. It will give you the source of the error.
(while writing the tests, I forgot to set the sql-platform and I got the "failed to obtan row locks", because the init-sql had the wrong syntax.)
| } | ||
|
|
||
| @Disabled | ||
| //@Disabled |
There was a problem hiding this comment.
We can disable this, if build takes too long
| dropTable("m3"); | ||
|
|
||
| if (withExisting) { | ||
| // create empty migration table |
There was a problem hiding this comment.
I added tests where it checks, if the migration will also work (so, if table exists, but contains no run migrations, yet)
| try (Connection conn = dataSource.getConnection()) { | ||
| String derivedPlatformName = DbNameUtil.normalise(conn); | ||
|
|
||
| config.setPlatform(derivedPlatformName); |
There was a problem hiding this comment.
I forgot this line, and got wrong script for sql server (see other comment with exception handling)
| } | ||
| } | ||
| assertThat(rawVersions).containsExactly("0", "hello", "1.1", "1.2", "1.2.1", "m2_view"); | ||
| table.unlockMigrationTable(); |
There was a problem hiding this comment.
just for completeness, I added unlockMigrationTable in all tests
|
Nice !! |
2 participants
Hello Rob, thanks for fixing #107
I did some tests and I think it is working as expected.
I have some suggestions and ideas, how to further improve the current procedure.
(they are non-critical)