You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@aurora.apache.org by Zameer Manji <zm...@apache.org> on 2017/03/29 19:21:46 UTC

Review Request 58036: Ensure enum tables are complete afer a snapshot restore.

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

Review request for Aurora, Santhosh Kumar Shanmugham and Stephan Erb.

Bugs: AURORA-1912
https://issues.apache.org/jira/browse/AURORA-1912

Repository: aurora

Description
-------

In our in memory database, we model enums as two column tables. The two columns
would be `id` which corresponds to the integer value in the thrift enum and
`name` which is the all caps string name of the enum. For example to model the
`JobUpdateStatus` enum we have a table called `job_update_statuses`. In there
the `ROLLING_FORWARD` enum is modeled as a row `(0, "ROLLING_FORWARD")`. Other
tables reference the enum table via the id.

When we prepare storage on startup the `DbStorage` starts up. It does two
things:
1. Load in the schema.
2. Populate the enum tables.

This ensures that when we insert values into the database, the enum refernces
will be valid.

However, after we restore from a Snapshot with the `dbScript` field, we blow all
of that data away and restore what was in the snapshot:
````
try (Connection c = ((DataSource) store.getUnsafeStoreAccess()).getConnection()) {
LOG.info("Dropping all tables");
try (PreparedStatement drop = c.prepareStatement("DROP ALL OBJECTS")) \
drop.executeUpdate();
}
````

This means that if we add a new enum value, and then restore from a snapshot,
that enum value will not exist in the table any more. We could address this by
saying that every enum value addition requires a migration. However instead I
propose not blowing away the work done by `DbStorage` instead and re-hydrating
the enum tables.

To do this I extracted the logic into a new class `EnumBackfill`. Restoring from
a snapshot calls this after the migrations are done. The underlying SQL was
changed from `INSERT` to `MERGE` to make this work.

Diffs
-----

src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java f0b148cd158d61cd89cc51dca9f3fa4c6feb1b49
src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 36a1bd5c784ed0febebccfd22e5064f0b2e3106f
src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java d1a196419b67108ee2bb778f83a2993e2e5ee83b
src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 923e904f396724b9dde4a330ef312a6aae2c02a6
src/main/java/org/apache/aurora/scheduler/storage/db/EnumBackfill.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 81a8cca6974e33c774473a4990e0e981cf6ddee6
src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml 153fd26c27275c46b190e71d8a5736153f2c2d18
src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 42615da54096d5b06c7989cb30fc3cfbe59bc1b9
src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java f26529c76214c8f22563f04a197798c82d341b49
src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java ca9525665805a33b4a322a72022ff037f0dd2a94

Diff: https://reviews.apache.org/r/58036/diff/1/

Testing
-------

existing tests and e2e tests

I also added a new enum value to `JobUpdateStatus` and observed it was correctly loaded in.

Thanks,

Zameer Manji

Re: Review Request 58036: Ensure enum tables are complete afer a snapshot restore.

Posted by Zameer Manji <zm...@apache.org>.


> On March 29, 2017, 12:34 p.m., Joshua Cohen wrote:
> > This may be an existing problem with the current impl as well, but what happens if we drop an enum value? Are we just assuming some other migration will have removed it (since presumably other tables will need to be updated to have their FK relations fixed)?

Yes, exactly. Dropping an enum value requires removing other tables. I presume we will just never use enums instead of actually deleting them.


- Zameer


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


On March 29, 2017, 12:21 p.m., Zameer Manji wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58036/
> -----------------------------------------------------------
> 
> (Updated March 29, 2017, 12:21 p.m.)
> 
> 
> Review request for Aurora, Santhosh Kumar Shanmugham and Stephan Erb.
> 
> 
> Bugs: AURORA-1912
>     https://issues.apache.org/jira/browse/AURORA-1912
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> In our in memory database, we model enums as two column tables. The two columns
> would be `id` which corresponds to the integer value in the thrift enum and
> `name` which is the all caps string name of the enum. For example to model the
> `JobUpdateStatus` enum we have a table called `job_update_statuses`. In there
> the `ROLLING_FORWARD` enum is modeled as a row `(0, "ROLLING_FORWARD")`. Other
> tables reference the enum table via the id.
> 
> When we prepare storage on startup the `DbStorage` starts up. It does two
> things:
> 1. Load in the schema.
> 2. Populate the enum tables.
> 
> This ensures that when we insert values into the database, the enum refernces
> will be valid.
> 
> However, after we restore from a Snapshot with the `dbScript` field, we blow all
> of that data away and restore what was in the snapshot:
> ````
> try (Connection c = ((DataSource) store.getUnsafeStoreAccess()).getConnection()) {
>   LOG.info("Dropping all tables");
>   try (PreparedStatement drop = c.prepareStatement("DROP ALL OBJECTS")) \
>     drop.executeUpdate();
>   }
> ````
> 
> This means that if we add a new enum value, and then restore from a snapshot,
> that enum value will not exist in the table any more. We could address this by
> saying that every enum value addition requires a migration. However instead I
> propose not blowing away the work done by `DbStorage` instead and re-hydrating
> the enum tables.
> 
> To do this I extracted the logic into a new class `EnumBackfill`. Restoring from
> a snapshot calls this after the migrations are done. The underlying SQL was
> changed from `INSERT` to `MERGE` to make this work.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java f0b148cd158d61cd89cc51dca9f3fa4c6feb1b49 
>   src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 36a1bd5c784ed0febebccfd22e5064f0b2e3106f 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java d1a196419b67108ee2bb778f83a2993e2e5ee83b 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 923e904f396724b9dde4a330ef312a6aae2c02a6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/EnumBackfill.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 81a8cca6974e33c774473a4990e0e981cf6ddee6 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml 153fd26c27275c46b190e71d8a5736153f2c2d18 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 42615da54096d5b06c7989cb30fc3cfbe59bc1b9 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java f26529c76214c8f22563f04a197798c82d341b49 
>   src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java ca9525665805a33b4a322a72022ff037f0dd2a94 
> 
> 
> Diff: https://reviews.apache.org/r/58036/diff/1/
> 
> 
> Testing
> -------
> 
> existing tests and e2e tests
> 
> I also added a new enum value to `JobUpdateStatus` and observed it was correctly loaded in.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 58036: Ensure enum tables are complete afer a snapshot restore.

Posted by Joshua Cohen <jc...@apache.org>.


> On March 29, 2017, 7:34 p.m., Joshua Cohen wrote:
> > This may be an existing problem with the current impl as well, but what happens if we drop an enum value? Are we just assuming some other migration will have removed it (since presumably other tables will need to be updated to have their FK relations fixed)?
> 
> Zameer Manji wrote:
>     Yes, exactly. Dropping an enum value requires removing other tables. I presume we will just never use enums instead of actually deleting them.

Might want to document that somewhere (even if just a comment on the backfill that it's only for additive changes). Otherwise the general approach looks good to me.


- Joshua


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


On March 29, 2017, 7:21 p.m., Zameer Manji wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58036/
> -----------------------------------------------------------
> 
> (Updated March 29, 2017, 7:21 p.m.)
> 
> 
> Review request for Aurora, Santhosh Kumar Shanmugham and Stephan Erb.
> 
> 
> Bugs: AURORA-1912
>     https://issues.apache.org/jira/browse/AURORA-1912
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> In our in memory database, we model enums as two column tables. The two columns
> would be `id` which corresponds to the integer value in the thrift enum and
> `name` which is the all caps string name of the enum. For example to model the
> `JobUpdateStatus` enum we have a table called `job_update_statuses`. In there
> the `ROLLING_FORWARD` enum is modeled as a row `(0, "ROLLING_FORWARD")`. Other
> tables reference the enum table via the id.
> 
> When we prepare storage on startup the `DbStorage` starts up. It does two
> things:
> 1. Load in the schema.
> 2. Populate the enum tables.
> 
> This ensures that when we insert values into the database, the enum refernces
> will be valid.
> 
> However, after we restore from a Snapshot with the `dbScript` field, we blow all
> of that data away and restore what was in the snapshot:
> ````
> try (Connection c = ((DataSource) store.getUnsafeStoreAccess()).getConnection()) {
>   LOG.info("Dropping all tables");
>   try (PreparedStatement drop = c.prepareStatement("DROP ALL OBJECTS")) \
>     drop.executeUpdate();
>   }
> ````
> 
> This means that if we add a new enum value, and then restore from a snapshot,
> that enum value will not exist in the table any more. We could address this by
> saying that every enum value addition requires a migration. However instead I
> propose not blowing away the work done by `DbStorage` instead and re-hydrating
> the enum tables.
> 
> To do this I extracted the logic into a new class `EnumBackfill`. Restoring from
> a snapshot calls this after the migrations are done. The underlying SQL was
> changed from `INSERT` to `MERGE` to make this work.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java f0b148cd158d61cd89cc51dca9f3fa4c6feb1b49 
>   src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 36a1bd5c784ed0febebccfd22e5064f0b2e3106f 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java d1a196419b67108ee2bb778f83a2993e2e5ee83b 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 923e904f396724b9dde4a330ef312a6aae2c02a6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/EnumBackfill.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 81a8cca6974e33c774473a4990e0e981cf6ddee6 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml 153fd26c27275c46b190e71d8a5736153f2c2d18 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 42615da54096d5b06c7989cb30fc3cfbe59bc1b9 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java f26529c76214c8f22563f04a197798c82d341b49 
>   src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java ca9525665805a33b4a322a72022ff037f0dd2a94 
> 
> 
> Diff: https://reviews.apache.org/r/58036/diff/1/
> 
> 
> Testing
> -------
> 
> existing tests and e2e tests
> 
> I also added a new enum value to `JobUpdateStatus` and observed it was correctly loaded in.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 58036: Ensure enum tables are complete afer a snapshot restore.

Posted by Zameer Manji <zm...@apache.org>.


> On March 29, 2017, 12:34 p.m., Joshua Cohen wrote:
> > This may be an existing problem with the current impl as well, but what happens if we drop an enum value? Are we just assuming some other migration will have removed it (since presumably other tables will need to be updated to have their FK relations fixed)?
> 
> Zameer Manji wrote:
>     Yes, exactly. Dropping an enum value requires removing other tables. I presume we will just never use enums instead of actually deleting them.
> 
> Joshua Cohen wrote:
>     Might want to document that somewhere (even if just a comment on the backfill that it's only for additive changes). Otherwise the general approach looks good to me.

Done.


- Zameer


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


On March 29, 2017, 12:51 p.m., Zameer Manji wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58036/
> -----------------------------------------------------------
> 
> (Updated March 29, 2017, 12:51 p.m.)
> 
> 
> Review request for Aurora, Santhosh Kumar Shanmugham and Stephan Erb.
> 
> 
> Bugs: AURORA-1912
>     https://issues.apache.org/jira/browse/AURORA-1912
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> In our in memory database, we model enums as two column tables. The two columns
> would be `id` which corresponds to the integer value in the thrift enum and
> `name` which is the all caps string name of the enum. For example to model the
> `JobUpdateStatus` enum we have a table called `job_update_statuses`. In there
> the `ROLLING_FORWARD` enum is modeled as a row `(0, "ROLLING_FORWARD")`. Other
> tables reference the enum table via the id.
> 
> When we prepare storage on startup the `DbStorage` starts up. It does two
> things:
> 1. Load in the schema.
> 2. Populate the enum tables.
> 
> This ensures that when we insert values into the database, the enum refernces
> will be valid.
> 
> However, after we restore from a Snapshot with the `dbScript` field, we blow all
> of that data away and restore what was in the snapshot:
> ````
> try (Connection c = ((DataSource) store.getUnsafeStoreAccess()).getConnection()) {
>   LOG.info("Dropping all tables");
>   try (PreparedStatement drop = c.prepareStatement("DROP ALL OBJECTS")) \
>     drop.executeUpdate();
>   }
> ````
> 
> This means that if we add a new enum value, and then restore from a snapshot,
> that enum value will not exist in the table any more. We could address this by
> saying that every enum value addition requires a migration. However instead I
> propose not blowing away the work done by `DbStorage` instead and re-hydrating
> the enum tables.
> 
> To do this I extracted the logic into a new class `EnumBackfill`. Restoring from
> a snapshot calls this after the migrations are done. The underlying SQL was
> changed from `INSERT` to `MERGE` to make this work.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 36a1bd5c784ed0febebccfd22e5064f0b2e3106f 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java d1a196419b67108ee2bb778f83a2993e2e5ee83b 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 923e904f396724b9dde4a330ef312a6aae2c02a6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/EnumBackfill.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 81a8cca6974e33c774473a4990e0e981cf6ddee6 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml 153fd26c27275c46b190e71d8a5736153f2c2d18 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 42615da54096d5b06c7989cb30fc3cfbe59bc1b9 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java f26529c76214c8f22563f04a197798c82d341b49 
>   src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java ca9525665805a33b4a322a72022ff037f0dd2a94 
> 
> 
> Diff: https://reviews.apache.org/r/58036/diff/2/
> 
> 
> Testing
> -------
> 
> existing tests and e2e tests
> 
> I also added a new enum value to `JobUpdateStatus` and observed it was correctly loaded in.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 58036: Ensure enum tables are complete afer a snapshot restore.

Posted by Joshua Cohen <jc...@apache.org>.

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



This may be an existing problem with the current impl as well, but what happens if we drop an enum value? Are we just assuming some other migration will have removed it (since presumably other tables will need to be updated to have their FK relations fixed)?

- Joshua Cohen


On March 29, 2017, 7:21 p.m., Zameer Manji wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58036/
> -----------------------------------------------------------
> 
> (Updated March 29, 2017, 7:21 p.m.)
> 
> 
> Review request for Aurora, Santhosh Kumar Shanmugham and Stephan Erb.
> 
> 
> Bugs: AURORA-1912
>     https://issues.apache.org/jira/browse/AURORA-1912
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> In our in memory database, we model enums as two column tables. The two columns
> would be `id` which corresponds to the integer value in the thrift enum and
> `name` which is the all caps string name of the enum. For example to model the
> `JobUpdateStatus` enum we have a table called `job_update_statuses`. In there
> the `ROLLING_FORWARD` enum is modeled as a row `(0, "ROLLING_FORWARD")`. Other
> tables reference the enum table via the id.
> 
> When we prepare storage on startup the `DbStorage` starts up. It does two
> things:
> 1. Load in the schema.
> 2. Populate the enum tables.
> 
> This ensures that when we insert values into the database, the enum refernces
> will be valid.
> 
> However, after we restore from a Snapshot with the `dbScript` field, we blow all
> of that data away and restore what was in the snapshot:
> ````
> try (Connection c = ((DataSource) store.getUnsafeStoreAccess()).getConnection()) {
>   LOG.info("Dropping all tables");
>   try (PreparedStatement drop = c.prepareStatement("DROP ALL OBJECTS")) \
>     drop.executeUpdate();
>   }
> ````
> 
> This means that if we add a new enum value, and then restore from a snapshot,
> that enum value will not exist in the table any more. We could address this by
> saying that every enum value addition requires a migration. However instead I
> propose not blowing away the work done by `DbStorage` instead and re-hydrating
> the enum tables.
> 
> To do this I extracted the logic into a new class `EnumBackfill`. Restoring from
> a snapshot calls this after the migrations are done. The underlying SQL was
> changed from `INSERT` to `MERGE` to make this work.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java f0b148cd158d61cd89cc51dca9f3fa4c6feb1b49 
>   src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 36a1bd5c784ed0febebccfd22e5064f0b2e3106f 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java d1a196419b67108ee2bb778f83a2993e2e5ee83b 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 923e904f396724b9dde4a330ef312a6aae2c02a6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/EnumBackfill.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 81a8cca6974e33c774473a4990e0e981cf6ddee6 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml 153fd26c27275c46b190e71d8a5736153f2c2d18 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 42615da54096d5b06c7989cb30fc3cfbe59bc1b9 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java f26529c76214c8f22563f04a197798c82d341b49 
>   src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java ca9525665805a33b4a322a72022ff037f0dd2a94 
> 
> 
> Diff: https://reviews.apache.org/r/58036/diff/1/
> 
> 
> Testing
> -------
> 
> existing tests and e2e tests
> 
> I also added a new enum value to `JobUpdateStatus` and observed it was correctly loaded in.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 58036: Ensure enum tables are complete afer a snapshot restore.

Posted by Aurora ReviewBot <wf...@apache.org>.

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

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

:processResources
:classes
:jar
:startScripts
:distTar
:distZip
:assemble
:compileJmhJavaNote: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.

:processJmhResources UP-TO-DATE
:jmhClasses
:checkstyleJmh
:jsHint
:checkstyleMain[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java:49:8: Unused import - org.apache.aurora.scheduler.storage.db.EnumBackfill. [UnusedImports]
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java:29:8: Unused import - org.apache.aurora.gen.CronCollisionPolicy. [UnusedImports]
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java:30:8: Unused import - org.apache.aurora.gen.JobUpdateAction. [UnusedImports]
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java:31:8: Unused import - org.apache.aurora.gen.JobUpdateStatus. [UnusedImports]
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java:32:8: Unused import - org.apache.aurora.gen.MaintenanceMode. [UnusedImports]
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java:33:8: Unused import - org.apache.aurora.gen.Mode. [UnusedImports]
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java:34:8: Unused import - org.apache.aurora.gen.ScheduleStatus. [UnusedImports]
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java:38:8: Unused import - org.apache.aurora.scheduler.resources.ResourceType. [UnusedImports]
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/EnumBackfill.java:1: Line does not match expected header line of '^\/\*\*$'. [RegexpHeader]
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/EnumBackfill.java:17: First sentence should end with a period. [JavadocStyle]
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/EnumBackfill.java:46: Line is longer than 100 characters (found 102). [LineLength]
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/EnumBackfill.java:54: Line is longer than 100 characters (found 101). [LineLength]
 FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':checkstyleMain'.
> Checkstyle rule violations were found. See the report at: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/checkstyle/main.html

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.

BUILD FAILED

Total time: 1 mins 24.435 secs

I will refresh this build result if you post a review containing "@ReviewBot retry"

- Aurora ReviewBot

On March 29, 2017, 7:21 p.m., Zameer Manji wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58036/
> -----------------------------------------------------------
> 
> (Updated March 29, 2017, 7:21 p.m.)
> 
> 
> Review request for Aurora, Santhosh Kumar Shanmugham and Stephan Erb.
> 
> 
> Bugs: AURORA-1912
>     https://issues.apache.org/jira/browse/AURORA-1912
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> In our in memory database, we model enums as two column tables. The two columns
> would be `id` which corresponds to the integer value in the thrift enum and
> `name` which is the all caps string name of the enum. For example to model the
> `JobUpdateStatus` enum we have a table called `job_update_statuses`. In there
> the `ROLLING_FORWARD` enum is modeled as a row `(0, "ROLLING_FORWARD")`. Other
> tables reference the enum table via the id.
> 
> When we prepare storage on startup the `DbStorage` starts up. It does two
> things:
> 1. Load in the schema.
> 2. Populate the enum tables.
> 
> This ensures that when we insert values into the database, the enum refernces
> will be valid.
> 
> However, after we restore from a Snapshot with the `dbScript` field, we blow all
> of that data away and restore what was in the snapshot:
> ````
> try (Connection c = ((DataSource) store.getUnsafeStoreAccess()).getConnection()) {
>   LOG.info("Dropping all tables");
>   try (PreparedStatement drop = c.prepareStatement("DROP ALL OBJECTS")) \
>     drop.executeUpdate();
>   }
> ````
> 
> This means that if we add a new enum value, and then restore from a snapshot,
> that enum value will not exist in the table any more. We could address this by
> saying that every enum value addition requires a migration. However instead I
> propose not blowing away the work done by `DbStorage` instead and re-hydrating
> the enum tables.
> 
> To do this I extracted the logic into a new class `EnumBackfill`. Restoring from
> a snapshot calls this after the migrations are done. The underlying SQL was
> changed from `INSERT` to `MERGE` to make this work.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java f0b148cd158d61cd89cc51dca9f3fa4c6feb1b49 
>   src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 36a1bd5c784ed0febebccfd22e5064f0b2e3106f 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java d1a196419b67108ee2bb778f83a2993e2e5ee83b 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 923e904f396724b9dde4a330ef312a6aae2c02a6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/EnumBackfill.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 81a8cca6974e33c774473a4990e0e981cf6ddee6 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml 153fd26c27275c46b190e71d8a5736153f2c2d18 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 42615da54096d5b06c7989cb30fc3cfbe59bc1b9 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java f26529c76214c8f22563f04a197798c82d341b49 
>   src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java ca9525665805a33b4a322a72022ff037f0dd2a94 
> 
> 
> Diff: https://reviews.apache.org/r/58036/diff/1/
> 
> 
> Testing
> -------
> 
> existing tests and e2e tests
> 
> I also added a new enum value to `JobUpdateStatus` and observed it was correctly loaded in.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 58036: Ensure enum tables are complete afer a snapshot restore.

Posted by Aurora ReviewBot <wf...@apache.org>.

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


Ship it!




Master (c32f14c) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot retry"

- Aurora ReviewBot


On March 29, 2017, 7:51 p.m., Zameer Manji wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58036/
> -----------------------------------------------------------
> 
> (Updated March 29, 2017, 7:51 p.m.)
> 
> 
> Review request for Aurora, Santhosh Kumar Shanmugham and Stephan Erb.
> 
> 
> Bugs: AURORA-1912
>     https://issues.apache.org/jira/browse/AURORA-1912
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> In our in memory database, we model enums as two column tables. The two columns
> would be `id` which corresponds to the integer value in the thrift enum and
> `name` which is the all caps string name of the enum. For example to model the
> `JobUpdateStatus` enum we have a table called `job_update_statuses`. In there
> the `ROLLING_FORWARD` enum is modeled as a row `(0, "ROLLING_FORWARD")`. Other
> tables reference the enum table via the id.
> 
> When we prepare storage on startup the `DbStorage` starts up. It does two
> things:
> 1. Load in the schema.
> 2. Populate the enum tables.
> 
> This ensures that when we insert values into the database, the enum refernces
> will be valid.
> 
> However, after we restore from a Snapshot with the `dbScript` field, we blow all
> of that data away and restore what was in the snapshot:
> ````
> try (Connection c = ((DataSource) store.getUnsafeStoreAccess()).getConnection()) {
>   LOG.info("Dropping all tables");
>   try (PreparedStatement drop = c.prepareStatement("DROP ALL OBJECTS")) \
>     drop.executeUpdate();
>   }
> ````
> 
> This means that if we add a new enum value, and then restore from a snapshot,
> that enum value will not exist in the table any more. We could address this by
> saying that every enum value addition requires a migration. However instead I
> propose not blowing away the work done by `DbStorage` instead and re-hydrating
> the enum tables.
> 
> To do this I extracted the logic into a new class `EnumBackfill`. Restoring from
> a snapshot calls this after the migrations are done. The underlying SQL was
> changed from `INSERT` to `MERGE` to make this work.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 36a1bd5c784ed0febebccfd22e5064f0b2e3106f 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java d1a196419b67108ee2bb778f83a2993e2e5ee83b 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 923e904f396724b9dde4a330ef312a6aae2c02a6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/EnumBackfill.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 81a8cca6974e33c774473a4990e0e981cf6ddee6 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml 153fd26c27275c46b190e71d8a5736153f2c2d18 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 42615da54096d5b06c7989cb30fc3cfbe59bc1b9 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java f26529c76214c8f22563f04a197798c82d341b49 
>   src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java ca9525665805a33b4a322a72022ff037f0dd2a94 
> 
> 
> Diff: https://reviews.apache.org/r/58036/diff/2/
> 
> 
> Testing
> -------
> 
> existing tests and e2e tests
> 
> I also added a new enum value to `JobUpdateStatus` and observed it was correctly loaded in.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 58036: Ensure enum tables are complete after a snapshot restore.

Posted by Zameer Manji <zm...@apache.org>.

(Updated March 30, 2017, 10:36 a.m.)

Review request for Aurora, Santhosh Kumar Shanmugham and Stephan Erb.

Summary (updated)
-----------------

Ensure enum tables are complete after a snapshot restore.

Bugs: AURORA-1912
https://issues.apache.org/jira/browse/AURORA-1912

Repository: aurora

Description (updated)
-------

When we prepare storage on startup the `DbStorage` starts up. It does two
things:
1. Load in the schema.
2. Populate the enum tables.

This ensures that when we insert values into the database, the enum refernces
will be valid.

However, before we restore from a Snapshot with the `dbScript` field, we blow all
of that data away and restore what was in the snapshot:
````
try (Connection c = ((DataSource) store.getUnsafeStoreAccess()).getConnection()) {
LOG.info("Dropping all tables");
try (PreparedStatement drop = c.prepareStatement("DROP ALL OBJECTS"))
drop.executeUpdate();
}
````

Diffs
-----

src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 36a1bd5c784ed0febebccfd22e5064f0b2e3106f
src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java d1a196419b67108ee2bb778f83a2993e2e5ee83b
src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 923e904f396724b9dde4a330ef312a6aae2c02a6
src/main/java/org/apache/aurora/scheduler/storage/db/EnumBackfill.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 81a8cca6974e33c774473a4990e0e981cf6ddee6
src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml 153fd26c27275c46b190e71d8a5736153f2c2d18
src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 42615da54096d5b06c7989cb30fc3cfbe59bc1b9
src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java f26529c76214c8f22563f04a197798c82d341b49
src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java ca9525665805a33b4a322a72022ff037f0dd2a94

Diff: https://reviews.apache.org/r/58036/diff/2/

Testing
-------

existing tests and e2e tests

I also added a new enum value to `JobUpdateStatus` and observed it was correctly loaded in.

Thanks,

Zameer Manji

Re: Review Request 58036: Ensure enum tables are complete afer a snapshot restore.

Posted by Stephan Erb <se...@apache.org>.

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


Ship it!




Ship It!

- Stephan Erb


On March 30, 2017, 1:42 a.m., Zameer Manji wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58036/
> -----------------------------------------------------------
> 
> (Updated March 30, 2017, 1:42 a.m.)
> 
> 
> Review request for Aurora, Santhosh Kumar Shanmugham and Stephan Erb.
> 
> 
> Bugs: AURORA-1912
>     https://issues.apache.org/jira/browse/AURORA-1912
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> In our in memory database, we model enums as two column tables. The two columns
> would be `id` which corresponds to the integer value in the thrift enum and
> `name` which is the all caps string name of the enum. For example to model the
> `JobUpdateStatus` enum we have a table called `job_update_statuses`. In there
> the `ROLLING_FORWARD` enum is modeled as a row `(0, "ROLLING_FORWARD")`. Other
> tables reference the enum table via the id.
> 
> When we prepare storage on startup the `DbStorage` starts up. It does two
> things:
> 1. Load in the schema.
> 2. Populate the enum tables.
> 
> This ensures that when we insert values into the database, the enum refernces
> will be valid.
> 
> However, after we restore from a Snapshot with the `dbScript` field, we blow all
> of that data away and restore what was in the snapshot:
> ````
> try (Connection c = ((DataSource) store.getUnsafeStoreAccess()).getConnection()) {
>   LOG.info("Dropping all tables");
>   try (PreparedStatement drop = c.prepareStatement("DROP ALL OBJECTS"))
>     drop.executeUpdate();
>   }
> ````
> 
> This means that if we add a new enum value, and then restore from a snapshot,
> that enum value will not exist in the table any more. We could address this by
> saying that every enum value addition requires a migration. However instead I
> propose not blowing away the work done by `DbStorage` instead and re-hydrating
> the enum tables.
> 
> To do this I extracted the logic into a new class `EnumBackfill`. Restoring from
> a snapshot calls this after the migrations are done. The underlying SQL was
> changed from `INSERT` to `MERGE` to make this work.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 36a1bd5c784ed0febebccfd22e5064f0b2e3106f 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java d1a196419b67108ee2bb778f83a2993e2e5ee83b 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 923e904f396724b9dde4a330ef312a6aae2c02a6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/EnumBackfill.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 81a8cca6974e33c774473a4990e0e981cf6ddee6 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml 153fd26c27275c46b190e71d8a5736153f2c2d18 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 42615da54096d5b06c7989cb30fc3cfbe59bc1b9 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java f26529c76214c8f22563f04a197798c82d341b49 
>   src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java ca9525665805a33b4a322a72022ff037f0dd2a94 
> 
> 
> Diff: https://reviews.apache.org/r/58036/diff/2/
> 
> 
> Testing
> -------
> 
> existing tests and e2e tests
> 
> I also added a new enum value to `JobUpdateStatus` and observed it was correctly loaded in.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 58036: Ensure enum tables are complete afer a snapshot restore.

Posted by Zameer Manji <zm...@apache.org>.


> On March 29, 2017, 9:41 p.m., Santhosh Kumar Shanmugham wrote:
> > In the commit message - s/after we restroe
> 
> Santhosh Kumar Shanmugham wrote:
>     s/after we restore from a Snapshot/before we restore from a Snapshot/
>     
>     Slight -1 for forking special-casing the enum table migration, but I can see some benefit.

I agree with the sentiment. However enums are a special case. Most columns originate in the schema, but enum values originate in api.thrift. As a result, we need to handle them in a special manner.


- Zameer


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


On March 29, 2017, 4:42 p.m., Zameer Manji wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58036/
> -----------------------------------------------------------
> 
> (Updated March 29, 2017, 4:42 p.m.)
> 
> 
> Review request for Aurora, Santhosh Kumar Shanmugham and Stephan Erb.
> 
> 
> Bugs: AURORA-1912
>     https://issues.apache.org/jira/browse/AURORA-1912
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> In our in memory database, we model enums as two column tables. The two columns
> would be `id` which corresponds to the integer value in the thrift enum and
> `name` which is the all caps string name of the enum. For example to model the
> `JobUpdateStatus` enum we have a table called `job_update_statuses`. In there
> the `ROLLING_FORWARD` enum is modeled as a row `(0, "ROLLING_FORWARD")`. Other
> tables reference the enum table via the id.
> 
> When we prepare storage on startup the `DbStorage` starts up. It does two
> things:
> 1. Load in the schema.
> 2. Populate the enum tables.
> 
> This ensures that when we insert values into the database, the enum refernces
> will be valid.
> 
> However, after we restore from a Snapshot with the `dbScript` field, we blow all
> of that data away and restore what was in the snapshot:
> ````
> try (Connection c = ((DataSource) store.getUnsafeStoreAccess()).getConnection()) {
>   LOG.info("Dropping all tables");
>   try (PreparedStatement drop = c.prepareStatement("DROP ALL OBJECTS"))
>     drop.executeUpdate();
>   }
> ````
> 
> This means that if we add a new enum value, and then restore from a snapshot,
> that enum value will not exist in the table any more. We could address this by
> saying that every enum value addition requires a migration. However instead I
> propose not blowing away the work done by `DbStorage` instead and re-hydrating
> the enum tables.
> 
> To do this I extracted the logic into a new class `EnumBackfill`. Restoring from
> a snapshot calls this after the migrations are done. The underlying SQL was
> changed from `INSERT` to `MERGE` to make this work.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 36a1bd5c784ed0febebccfd22e5064f0b2e3106f 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java d1a196419b67108ee2bb778f83a2993e2e5ee83b 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 923e904f396724b9dde4a330ef312a6aae2c02a6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/EnumBackfill.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 81a8cca6974e33c774473a4990e0e981cf6ddee6 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml 153fd26c27275c46b190e71d8a5736153f2c2d18 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 42615da54096d5b06c7989cb30fc3cfbe59bc1b9 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java f26529c76214c8f22563f04a197798c82d341b49 
>   src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java ca9525665805a33b4a322a72022ff037f0dd2a94 
> 
> 
> Diff: https://reviews.apache.org/r/58036/diff/2/
> 
> 
> Testing
> -------
> 
> existing tests and e2e tests
> 
> I also added a new enum value to `JobUpdateStatus` and observed it was correctly loaded in.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 58036: Ensure enum tables are complete afer a snapshot restore.

Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.


> On March 29, 2017, 9:41 p.m., Santhosh Kumar Shanmugham wrote:
> > In the commit message - s/after we restroe

s/after we restore from a Snapshot/before we restore from a Snapshot/

Slight -1 for forking special-casing the enum table migration, but I can see some benefit.


- Santhosh Kumar


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


On March 29, 2017, 4:42 p.m., Zameer Manji wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58036/
> -----------------------------------------------------------
> 
> (Updated March 29, 2017, 4:42 p.m.)
> 
> 
> Review request for Aurora, Santhosh Kumar Shanmugham and Stephan Erb.
> 
> 
> Bugs: AURORA-1912
>     https://issues.apache.org/jira/browse/AURORA-1912
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> In our in memory database, we model enums as two column tables. The two columns
> would be `id` which corresponds to the integer value in the thrift enum and
> `name` which is the all caps string name of the enum. For example to model the
> `JobUpdateStatus` enum we have a table called `job_update_statuses`. In there
> the `ROLLING_FORWARD` enum is modeled as a row `(0, "ROLLING_FORWARD")`. Other
> tables reference the enum table via the id.
> 
> When we prepare storage on startup the `DbStorage` starts up. It does two
> things:
> 1. Load in the schema.
> 2. Populate the enum tables.
> 
> This ensures that when we insert values into the database, the enum refernces
> will be valid.
> 
> However, after we restore from a Snapshot with the `dbScript` field, we blow all
> of that data away and restore what was in the snapshot:
> ````
> try (Connection c = ((DataSource) store.getUnsafeStoreAccess()).getConnection()) {
>   LOG.info("Dropping all tables");
>   try (PreparedStatement drop = c.prepareStatement("DROP ALL OBJECTS"))
>     drop.executeUpdate();
>   }
> ````
> 
> This means that if we add a new enum value, and then restore from a snapshot,
> that enum value will not exist in the table any more. We could address this by
> saying that every enum value addition requires a migration. However instead I
> propose not blowing away the work done by `DbStorage` instead and re-hydrating
> the enum tables.
> 
> To do this I extracted the logic into a new class `EnumBackfill`. Restoring from
> a snapshot calls this after the migrations are done. The underlying SQL was
> changed from `INSERT` to `MERGE` to make this work.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 36a1bd5c784ed0febebccfd22e5064f0b2e3106f 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java d1a196419b67108ee2bb778f83a2993e2e5ee83b 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 923e904f396724b9dde4a330ef312a6aae2c02a6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/EnumBackfill.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 81a8cca6974e33c774473a4990e0e981cf6ddee6 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml 153fd26c27275c46b190e71d8a5736153f2c2d18 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 42615da54096d5b06c7989cb30fc3cfbe59bc1b9 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java f26529c76214c8f22563f04a197798c82d341b49 
>   src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java ca9525665805a33b4a322a72022ff037f0dd2a94 
> 
> 
> Diff: https://reviews.apache.org/r/58036/diff/2/
> 
> 
> Testing
> -------
> 
> existing tests and e2e tests
> 
> I also added a new enum value to `JobUpdateStatus` and observed it was correctly loaded in.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 58036: Ensure enum tables are complete afer a snapshot restore.

Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.

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


Ship it!




In the commit message - s/after we restroe

- Santhosh Kumar Shanmugham


On March 29, 2017, 4:42 p.m., Zameer Manji wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58036/
> -----------------------------------------------------------
> 
> (Updated March 29, 2017, 4:42 p.m.)
> 
> 
> Review request for Aurora, Santhosh Kumar Shanmugham and Stephan Erb.
> 
> 
> Bugs: AURORA-1912
>     https://issues.apache.org/jira/browse/AURORA-1912
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> In our in memory database, we model enums as two column tables. The two columns
> would be `id` which corresponds to the integer value in the thrift enum and
> `name` which is the all caps string name of the enum. For example to model the
> `JobUpdateStatus` enum we have a table called `job_update_statuses`. In there
> the `ROLLING_FORWARD` enum is modeled as a row `(0, "ROLLING_FORWARD")`. Other
> tables reference the enum table via the id.
> 
> When we prepare storage on startup the `DbStorage` starts up. It does two
> things:
> 1. Load in the schema.
> 2. Populate the enum tables.
> 
> This ensures that when we insert values into the database, the enum refernces
> will be valid.
> 
> However, after we restore from a Snapshot with the `dbScript` field, we blow all
> of that data away and restore what was in the snapshot:
> ````
> try (Connection c = ((DataSource) store.getUnsafeStoreAccess()).getConnection()) {
>   LOG.info("Dropping all tables");
>   try (PreparedStatement drop = c.prepareStatement("DROP ALL OBJECTS"))
>     drop.executeUpdate();
>   }
> ````
> 
> This means that if we add a new enum value, and then restore from a snapshot,
> that enum value will not exist in the table any more. We could address this by
> saying that every enum value addition requires a migration. However instead I
> propose not blowing away the work done by `DbStorage` instead and re-hydrating
> the enum tables.
> 
> To do this I extracted the logic into a new class `EnumBackfill`. Restoring from
> a snapshot calls this after the migrations are done. The underlying SQL was
> changed from `INSERT` to `MERGE` to make this work.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 36a1bd5c784ed0febebccfd22e5064f0b2e3106f 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java d1a196419b67108ee2bb778f83a2993e2e5ee83b 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 923e904f396724b9dde4a330ef312a6aae2c02a6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/EnumBackfill.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 81a8cca6974e33c774473a4990e0e981cf6ddee6 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml 153fd26c27275c46b190e71d8a5736153f2c2d18 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 42615da54096d5b06c7989cb30fc3cfbe59bc1b9 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java f26529c76214c8f22563f04a197798c82d341b49 
>   src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java ca9525665805a33b4a322a72022ff037f0dd2a94 
> 
> 
> Diff: https://reviews.apache.org/r/58036/diff/2/
> 
> 
> Testing
> -------
> 
> existing tests and e2e tests
> 
> I also added a new enum value to `JobUpdateStatus` and observed it was correctly loaded in.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 58036: Ensure enum tables are complete afer a snapshot restore.

Posted by Zameer Manji <zm...@apache.org>.

(Updated March 29, 2017, 4:42 p.m.)

Review request for Aurora, Santhosh Kumar Shanmugham and Stephan Erb.

Bugs: AURORA-1912
https://issues.apache.org/jira/browse/AURORA-1912

Repository: aurora

Description (updated)
-------

When we prepare storage on startup the `DbStorage` starts up. It does two
things:
1. Load in the schema.
2. Populate the enum tables.

This ensures that when we insert values into the database, the enum refernces
will be valid.

Diffs
-----

Diff: https://reviews.apache.org/r/58036/diff/2/

Testing
-------

existing tests and e2e tests

I also added a new enum value to `JobUpdateStatus` and observed it was correctly loaded in.

Thanks,

Zameer Manji

Re: Review Request 58036: Ensure enum tables are complete afer a snapshot restore.

Posted by Joshua Cohen <jc...@apache.org>.

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


Ship it!




Ship It!

- Joshua Cohen


On March 29, 2017, 7:51 p.m., Zameer Manji wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58036/
> -----------------------------------------------------------
> 
> (Updated March 29, 2017, 7:51 p.m.)
> 
> 
> Review request for Aurora, Santhosh Kumar Shanmugham and Stephan Erb.
> 
> 
> Bugs: AURORA-1912
>     https://issues.apache.org/jira/browse/AURORA-1912
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> In our in memory database, we model enums as two column tables. The two columns
> would be `id` which corresponds to the integer value in the thrift enum and
> `name` which is the all caps string name of the enum. For example to model the
> `JobUpdateStatus` enum we have a table called `job_update_statuses`. In there
> the `ROLLING_FORWARD` enum is modeled as a row `(0, "ROLLING_FORWARD")`. Other
> tables reference the enum table via the id.
> 
> When we prepare storage on startup the `DbStorage` starts up. It does two
> things:
> 1. Load in the schema.
> 2. Populate the enum tables.
> 
> This ensures that when we insert values into the database, the enum refernces
> will be valid.
> 
> However, after we restore from a Snapshot with the `dbScript` field, we blow all
> of that data away and restore what was in the snapshot:
> ````
> try (Connection c = ((DataSource) store.getUnsafeStoreAccess()).getConnection()) {
>   LOG.info("Dropping all tables");
>   try (PreparedStatement drop = c.prepareStatement("DROP ALL OBJECTS")) \
>     drop.executeUpdate();
>   }
> ````
> 
> This means that if we add a new enum value, and then restore from a snapshot,
> that enum value will not exist in the table any more. We could address this by
> saying that every enum value addition requires a migration. However instead I
> propose not blowing away the work done by `DbStorage` instead and re-hydrating
> the enum tables.
> 
> To do this I extracted the logic into a new class `EnumBackfill`. Restoring from
> a snapshot calls this after the migrations are done. The underlying SQL was
> changed from `INSERT` to `MERGE` to make this work.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 36a1bd5c784ed0febebccfd22e5064f0b2e3106f 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java d1a196419b67108ee2bb778f83a2993e2e5ee83b 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 923e904f396724b9dde4a330ef312a6aae2c02a6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/EnumBackfill.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 81a8cca6974e33c774473a4990e0e981cf6ddee6 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml 153fd26c27275c46b190e71d8a5736153f2c2d18 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 42615da54096d5b06c7989cb30fc3cfbe59bc1b9 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java f26529c76214c8f22563f04a197798c82d341b49 
>   src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java ca9525665805a33b4a322a72022ff037f0dd2a94 
> 
> 
> Diff: https://reviews.apache.org/r/58036/diff/2/
> 
> 
> Testing
> -------
> 
> existing tests and e2e tests
> 
> I also added a new enum value to `JobUpdateStatus` and observed it was correctly loaded in.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 58036: Ensure enum tables are complete afer a snapshot restore.

Posted by Zameer Manji <zm...@apache.org>.

(Updated March 29, 2017, 12:51 p.m.)

Review request for Aurora, Santhosh Kumar Shanmugham and Stephan Erb.

Changes
-------

Style fixes.

Bugs: AURORA-1912
https://issues.apache.org/jira/browse/AURORA-1912

Repository: aurora

Description
-------

When we prepare storage on startup the `DbStorage` starts up. It does two
things:
1. Load in the schema.
2. Populate the enum tables.

This ensures that when we insert values into the database, the enum refernces
will be valid.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/58036/diff/2/

Changes: https://reviews.apache.org/r/58036/diff/1-2/

Testing
-------

existing tests and e2e tests

I also added a new enum value to `JobUpdateStatus` and observed it was correctly loaded in.

Thanks,

Zameer Manji