Re: Review Request 45467: [PROTOTYPE] Add support for DB migrations and rollbacks.

[Bill Farner]

> On March 30, 2016, 10:59 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java,
> >  line 180
> > 
> >
> > As i read this, i found myself wishing for something that appears to be 
> > [`FileMigrationLoader`](https://github.com/mybatis/migrations/blob/master/src/main/java/org/apache/ibatis/migration/FileMigrationLoader.java)
> >  but with the ability to load resources from the classpath.
> > 
> > Have you given any thought to using that instead?
> 
> Joshua Cohen wrote:
> Are you saying you'd prefer migrations be stored as sql scripts rather 
> than classes? It think it would be possible to write something on top of 
> `FileMigrationLoader` that did that, but I actually kind of prefer the 
> class-based representation since we want to define both up and down scripts. 
> I don't love the way the file-based scripts represent both in the same file 
> with an `@UNDO` annotation; this seems like it could potentially be brittle.
> 
> Bill Farner wrote:
> Yes, my preference was to keep them in files.  I agree that the `@UNDO` 
> is a bit odd, but ultimately we should have a test plan for this effort.  
> Without proof of brittleness, i don't think the argument holds water.

To clarify more - i'm not firm on this position, more of a "if it's more 
concise and readable, i'd take it".  Unless this is obviously better from where 
you sit, i don't intend to get in the way.


- Bill


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45467/#review126151
---


On March 29, 2016, 8:41 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45467/
> ---
> 
> (Updated March 29, 2016, 8:41 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> THIS CODE IS NOT INTENDED TO BE COMMITTED.
> 
> This is just a spike to show a proof of concept for how we can effect 
> automatic migrations and rollbacks of the H2 schema. The code is very sloppy, 
> please use this to further the discussion on the mailing list about 
> migrations. If we agree this methodology is acceptable, I'll clean this up 
> and send out an actual review.
> 
> That said...
> 
> The general gist here is:
> 
> 1. Use MyBatis Migrations which has built in support for specifying an up and 
> a down script for db changes and also makes it easy to locate all existing 
> migrations on the classpath.
> 2. When applying a migration, write the downgrade script to the changelog 
> table in the database.
> 3. Before actually applying migrations, check all changes in the changelog 
> table. If the corresponding migration does not exist on the classpath, we 
> assume this is a rollback and run the downgrade script from the changelog 
> table and delete the corresponding changelog row.
> 
> 
> Diffs
> -
> 
>   build.gradle ad5ec5c4afe4dd5c2189d0680692a7409f0417a9 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 120b89a1dc10a259940cb9527eb2517f19d04471 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  e32862034a1ad47dae8fff89cb04deb34ccd90e2 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> baf460e987d0a6ba2810507695fe118b6b502f87 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/migration/V001_CreateUnifiedContainerTables.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
> 6fee2510d044515e0704cf20ec0ba77231050ec4 
> 
> Diff: https://reviews.apache.org/r/45467/diff/
> 
> 
> Testing
> ---
> 
> I manually verified in vagrant that this works as expected for upgrades with 
> migrations, upgrades without migrations and rollbacks.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 45467: [PROTOTYPE] Add support for DB migrations and rollbacks.

[Bill Farner]

> On March 30, 2016, 10:59 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java,
> >  line 180
> > 
> >
> > As i read this, i found myself wishing for something that appears to be 
> > [`FileMigrationLoader`](https://github.com/mybatis/migrations/blob/master/src/main/java/org/apache/ibatis/migration/FileMigrationLoader.java)
> >  but with the ability to load resources from the classpath.
> > 
> > Have you given any thought to using that instead?
> 
> Joshua Cohen wrote:
> Are you saying you'd prefer migrations be stored as sql scripts rather 
> than classes? It think it would be possible to write something on top of 
> `FileMigrationLoader` that did that, but I actually kind of prefer the 
> class-based representation since we want to define both up and down scripts. 
> I don't love the way the file-based scripts represent both in the same file 
> with an `@UNDO` annotation; this seems like it could potentially be brittle.

Yes, my preference was to keep them in files.  I agree that the `@UNDO` is a 
bit odd, but ultimately we should have a test plan for this effort.  Without 
proof of brittleness, i don't think the argument holds water.


- Bill


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45467/#review126151
---


On March 29, 2016, 8:41 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45467/
> ---
> 
> (Updated March 29, 2016, 8:41 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> THIS CODE IS NOT INTENDED TO BE COMMITTED.
> 
> This is just a spike to show a proof of concept for how we can effect 
> automatic migrations and rollbacks of the H2 schema. The code is very sloppy, 
> please use this to further the discussion on the mailing list about 
> migrations. If we agree this methodology is acceptable, I'll clean this up 
> and send out an actual review.
> 
> That said...
> 
> The general gist here is:
> 
> 1. Use MyBatis Migrations which has built in support for specifying an up and 
> a down script for db changes and also makes it easy to locate all existing 
> migrations on the classpath.
> 2. When applying a migration, write the downgrade script to the changelog 
> table in the database.
> 3. Before actually applying migrations, check all changes in the changelog 
> table. If the corresponding migration does not exist on the classpath, we 
> assume this is a rollback and run the downgrade script from the changelog 
> table and delete the corresponding changelog row.
> 
> 
> Diffs
> -
> 
>   build.gradle ad5ec5c4afe4dd5c2189d0680692a7409f0417a9 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 120b89a1dc10a259940cb9527eb2517f19d04471 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  e32862034a1ad47dae8fff89cb04deb34ccd90e2 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> baf460e987d0a6ba2810507695fe118b6b502f87 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/migration/V001_CreateUnifiedContainerTables.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
> 6fee2510d044515e0704cf20ec0ba77231050ec4 
> 
> Diff: https://reviews.apache.org/r/45467/diff/
> 
> 
> Testing
> ---
> 
> I manually verified in vagrant that this works as expected for upgrades with 
> migrations, upgrades without migrations and rollbacks.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 45467: [PROTOTYPE] Add support for DB migrations and rollbacks.

[Bill Farner]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45467/#review126151
---




src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
(line 179)


As i read this, i found myself wishing for something that appears to be 
[`FileMigrationLoader`](https://github.com/mybatis/migrations/blob/master/src/main/java/org/apache/ibatis/migration/FileMigrationLoader.java)
 but with the ability to load resources from the classpath.

Have you given any thought to using that instead?


- Bill Farner


On March 29, 2016, 8:41 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45467/
> ---
> 
> (Updated March 29, 2016, 8:41 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> THIS CODE IS NOT INTENDED TO BE COMMITTED.
> 
> This is just a spike to show a proof of concept for how we can effect 
> automatic migrations and rollbacks of the H2 schema. The code is very sloppy, 
> please use this to further the discussion on the mailing list about 
> migrations. If we agree this methodology is acceptable, I'll clean this up 
> and send out an actual review.
> 
> That said...
> 
> The general gist here is:
> 
> 1. Use MyBatis Migrations which has built in support for specifying an up and 
> a down script for db changes and also makes it easy to locate all existing 
> migrations on the classpath.
> 2. When applying a migration, write the downgrade script to the changelog 
> table in the database.
> 3. Before actually applying migrations, check all changes in the changelog 
> table. If the corresponding migration does not exist on the classpath, we 
> assume this is a rollback and run the downgrade script from the changelog 
> table and delete the corresponding changelog row.
> 
> 
> Diffs
> -
> 
>   build.gradle ad5ec5c4afe4dd5c2189d0680692a7409f0417a9 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 120b89a1dc10a259940cb9527eb2517f19d04471 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  e32862034a1ad47dae8fff89cb04deb34ccd90e2 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> baf460e987d0a6ba2810507695fe118b6b502f87 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/migration/V001_CreateUnifiedContainerTables.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
> 6fee2510d044515e0704cf20ec0ba77231050ec4 
> 
> Diff: https://reviews.apache.org/r/45467/diff/
> 
> 
> Testing
> ---
> 
> I manually verified in vagrant that this works as expected for upgrades with 
> migrations, upgrades without migrations and rollbacks.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 45467: [PROTOTYPE] Add support for DB migrations and rollbacks.

[Aurora ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45467/#review126033
---



Master (ec29ac1) is red with this patch.
  ./build-support/jenkins/build.sh

:checkstyleJmh
:jsHint
:checkstyleMain[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java:53:15:
 error: Unused import - 
org.apache.aurora.scheduler.http.H2ConsoleModule.H2_PERM.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/migration/V001_CreateUnifiedContainerTables.java:20:14:
 error: Name 'V001_CreateUnifiedContainerTables' must match pattern 
'^[A-Z][a-zA-Z0-9]*$'.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/migration/V001_CreateUnifiedContainerTables.java:33:52:
 error: '+' should be on a new line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/migration/V001_CreateUnifiedContainerTables.java:34:26:
 error: '+' should be on a new line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/migration/V001_CreateUnifiedContainerTables.java:35:91:
 error: '+' should be on a new line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/migration/V001_CreateUnifiedContainerTables.java:36:36:
 error: '+' should be on a new line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/migration/V001_CreateUnifiedContainerTables.java:37:40:
 error: '+' should be on a new line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/migration/V001_CreateUnifiedContainerTables.java:38:38:
 error: '+' should be on a new line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/migration/V001_CreateUnifiedContainerTables.java:39:14:
 error: '+' should be on a new line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/migration/V001_CreateUnifiedContainerTables.java:40:51:
 error: '+' should be on a new line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/migration/V001_CreateUnifiedContainerTables.java:41:26:
 error: '+' should be on a new line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/migration/V001_CreateUnifiedContainerTables.java:42:91:
 error: '+' should be on a new line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/migration/V001_CreateUnifiedContainerTables.java:43:36:
 error: '+' should be on a new line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/migration/V001_CreateUnifiedContainerTables.java:44:35:
 error: '+' should be on a new line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/migration/V001_CreateUnifiedContainerTables.java:45:36:
 error: '+' should be on a new line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/migration/V001_CreateUnifiedContainerTables.java:51:60:
 error: '+' should be on a new line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java:162:81:
 error: '+' should be on a new line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java:163:53:
 error: '+' should be on a new line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java:164:54:
 error: '+' should be on a new line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java:165:56:
 error: '+' should be on a new line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java:166:48:
 error: '+' should be on a new line.
[ant:checkstyle] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java:167:32:
 error: '+' should be on a new line.
[ant:checkstyle]