Re: Review Request 45112: Add support for storing and fetching images as properties of task configs.

[Aurora ReviewBot]

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


Ship it!




Master (335cf88) 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 22, 2016, 4:46 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45112/
> ---
> 
> (Updated March 22, 2016, 4:46 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1635
> https://issues.apache.org/jira/browse/AURORA-1635
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add support for storing and fetching images as properties of task configs.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> d4b8904031e6671a8083cac9b82d934377797fe2 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  b3b8ccf868c2a2f18f720a837e90d763072dd3eb 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 364026a8ef2b47cf1beafa3990691d3375516fe6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 12ca16b79a062d9ea15c206ef963fb077ad7ad98 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/DbImage.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
> eb848add00fba6d3571657bb9080be0599b2756a 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  fd272ccf9b1cfccd9198d1e5e0db37d23f546afa 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> be60c3be0cd51d06ba0ca7f56384281ee71d8c55 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  1a520b3cb035a9afc25406d2f313c9f861eee4d6 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java
>  c316e497a34a45c7ada2ca83a1115e826c0f572f 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> e56fed2e6c0cdb47737cf1a9b637c44c5e5b9815 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java
>  08530397ff75081bde6f07f9d53317b5486e0da4 
> 
> Diff: https://reviews.apache.org/r/45112/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> 
> ran e2e tests.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 45112: Add support for storing and fetching images as properties of task configs.

[Joshua Cohen]

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

(Updated March 22, 2016, 4:25 p.m.)


Review request for Aurora, Maxim Khutornenko and Bill Farner.


Changes
---

checkstyle.


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


Repository: aurora


Description
---

Add support for storing and fetching images as properties of task configs.


Diffs (updated)
-

  api/src/main/thrift/org/apache/aurora/gen/api.thrift 
d4b8904031e6671a8083cac9b82d934377797fe2 
  
src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
 b3b8ccf868c2a2f18f720a837e90d763072dd3eb 
  src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
364026a8ef2b47cf1beafa3990691d3375516fe6 
  src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
12ca16b79a062d9ea15c206ef963fb077ad7ad98 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/DbImage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
eb848add00fba6d3571657bb9080be0599b2756a 
  
src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml 
fd272ccf9b1cfccd9198d1e5e0db37d23f546afa 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
be60c3be0cd51d06ba0ca7f56384281ee71d8c55 
  
src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
 1a520b3cb035a9afc25406d2f313c9f861eee4d6 
  
src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java 
c316e497a34a45c7ada2ca83a1115e826c0f572f 
  src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
e56fed2e6c0cdb47737cf1a9b637c44c5e5b9815 
  
src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java 
08530397ff75081bde6f07f9d53317b5486e0da4 

Diff: https://reviews.apache.org/r/45112/diff/


Testing
---

./gradlew build -Pq

ran e2e tests.


Thanks,

Joshua Cohen

Re: Review Request 45112: Add support for storing and fetching images as properties of task configs.

[Stephan Erb]

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




src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
 (line 254)


Shouldn't that rather be formulated as container != `MESOS`? As the Image 
field will only work together with the Mesos containerizer.


- Stephan Erb


On March 22, 2016, 4:49 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45112/
> ---
> 
> (Updated March 22, 2016, 4:49 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1635
> https://issues.apache.org/jira/browse/AURORA-1635
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add support for storing and fetching images as properties of task configs.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> d4b8904031e6671a8083cac9b82d934377797fe2 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  b3b8ccf868c2a2f18f720a837e90d763072dd3eb 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 364026a8ef2b47cf1beafa3990691d3375516fe6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 12ca16b79a062d9ea15c206ef963fb077ad7ad98 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/DbImage.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
> eb848add00fba6d3571657bb9080be0599b2756a 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  fd272ccf9b1cfccd9198d1e5e0db37d23f546afa 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> be60c3be0cd51d06ba0ca7f56384281ee71d8c55 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  1a520b3cb035a9afc25406d2f313c9f861eee4d6 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java
>  c316e497a34a45c7ada2ca83a1115e826c0f572f 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> e56fed2e6c0cdb47737cf1a9b637c44c5e5b9815 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java
>  08530397ff75081bde6f07f9d53317b5486e0da4 
> 
> Diff: https://reviews.apache.org/r/45112/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> 
> ran e2e tests.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 45112: Add support for storing and fetching images as properties of task configs.

[Aurora ReviewBot]

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



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

:commons:generateThriftResources
:commons:processResources
:commons:classes
:commons:jar
:compileJava/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java:74:
 Note: Wrote forwarder 
org.apache.aurora.scheduler.storage.log.WriteAheadStorageForwarder
@Forward({
^
Note: Writing 
file:/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/classes/main/org/apache/aurora/common/args/apt/cmdline.arg.info.txt.2
Note: Writing 
file:/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/classes/main/META-INF/compiler/resource-mappings/org.apache.aurora.common.args.apt.CmdLineProcessor

:generateBuildProperties
: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] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/db/views/DbImage.java:33:
 error: 'else' child have incorrect indentation level 8, expected level should 
be 6.
 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: 2 mins 16.503 secs


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

- Aurora ReviewBot


On March 22, 2016, 3:49 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45112/
> ---
> 
> (Updated March 22, 2016, 3:49 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1635
> https://issues.apache.org/jira/browse/AURORA-1635
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add support for storing and fetching images as properties of task configs.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> d4b8904031e6671a8083cac9b82d934377797fe2 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  b3b8ccf868c2a2f18f720a837e90d763072dd3eb 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 364026a8ef2b47cf1beafa3990691d3375516fe6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 12ca16b79a062d9ea15c206ef963fb077ad7ad98 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/DbImage.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
> eb848add00fba6d3571657bb9080be0599b2756a 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  fd272ccf9b1cfccd9198d1e5e0db37d23f546afa 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> be60c3be0cd51d06ba0ca7f56384281ee71d8c55 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  1a520b3cb035a9afc25406d2f313c9f861eee4d6 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java
>  c316e497a34a45c7ada2ca83a1115e826c0f572f 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> e56fed2e6c0cdb47737cf1a9b637c44c5e5b9815 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java
>  08530397ff75081bde6f07f9d53317b5486e0da4 
> 
> Diff: https://reviews.apache.org/r/45112/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> 
> ran e2e tests.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 45112: Add support for storing and fetching images as properties of task configs.

[Maxim Khutornenko]

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


Ship it!




Thanks for following up!

- Maxim Khutornenko


On March 22, 2016, 3:49 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45112/
> ---
> 
> (Updated March 22, 2016, 3:49 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1635
> https://issues.apache.org/jira/browse/AURORA-1635
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add support for storing and fetching images as properties of task configs.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> d4b8904031e6671a8083cac9b82d934377797fe2 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  b3b8ccf868c2a2f18f720a837e90d763072dd3eb 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 364026a8ef2b47cf1beafa3990691d3375516fe6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 12ca16b79a062d9ea15c206ef963fb077ad7ad98 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/DbImage.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
> eb848add00fba6d3571657bb9080be0599b2756a 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  fd272ccf9b1cfccd9198d1e5e0db37d23f546afa 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> be60c3be0cd51d06ba0ca7f56384281ee71d8c55 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  1a520b3cb035a9afc25406d2f313c9f861eee4d6 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java
>  c316e497a34a45c7ada2ca83a1115e826c0f572f 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> e56fed2e6c0cdb47737cf1a9b637c44c5e5b9815 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java
>  08530397ff75081bde6f07f9d53317b5486e0da4 
> 
> Diff: https://reviews.apache.org/r/45112/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> 
> ran e2e tests.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 45112: Add support for storing and fetching images as properties of task configs.

[Joshua Cohen]

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

(Updated March 22, 2016, 3:49 p.m.)


Review request for Aurora, Maxim Khutornenko and Bill Farner.


Changes
---

Switch to separate tables for docker and appc images.


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


Repository: aurora


Description
---

Add support for storing and fetching images as properties of task configs.


Diffs (updated)
-

  api/src/main/thrift/org/apache/aurora/gen/api.thrift 
d4b8904031e6671a8083cac9b82d934377797fe2 
  
src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
 b3b8ccf868c2a2f18f720a837e90d763072dd3eb 
  src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
364026a8ef2b47cf1beafa3990691d3375516fe6 
  src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
12ca16b79a062d9ea15c206ef963fb077ad7ad98 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/DbImage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
eb848add00fba6d3571657bb9080be0599b2756a 
  
src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml 
fd272ccf9b1cfccd9198d1e5e0db37d23f546afa 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
be60c3be0cd51d06ba0ca7f56384281ee71d8c55 
  
src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
 1a520b3cb035a9afc25406d2f313c9f861eee4d6 
  
src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java 
c316e497a34a45c7ada2ca83a1115e826c0f572f 
  src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
e56fed2e6c0cdb47737cf1a9b637c44c5e5b9815 
  
src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java 
08530397ff75081bde6f07f9d53317b5486e0da4 

Diff: https://reviews.apache.org/r/45112/diff/


Testing
---

./gradlew build -Pq

ran e2e tests.


Thanks,

Joshua Cohen

Re: Review Request 45112: Add support for storing and fetching images as properties of task configs.

[Maxim Khutornenko]

> On March 21, 2016, 7:35 p.m., Maxim Khutornenko wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift, line 218
> > 
> >
> > Couldn't find what it maps to in DB, is it not saved?
> 
> Joshua Cohen wrote:
> It's done by `TaskConfigMananger` here: 
> https://reviews.apache.org/r/45112/diff/2#2
> 
> Maxim Khutornenko wrote:
> Ah, I guess `image_id` column name got me thinking it only supports appc. 
> On that note, shouldn't different image types be mapped to dedicated tables 
> to start with? With the move towards db-driven snapshots it may be easier to 
> reason about future schema migrations when there is a 1:1 denormalized 
> mapping between thrift and DB schema.
> 
> Joshua Cohen wrote:
> I did that based on a discussion you and I had when I was initially 
> investigating this. I had originally thought to store images using the same 
> structure as containers, and we discussed a flattened structure there, rather 
> than creating separate tables for each type of container. Given the 
> similarities between image definitions as things stand today it doesn't seem 
> necessary to have separate tables.
> 
> Do you feel differently now? I think this approach is reasonable given 
> what we know about the space, but I'm amenable to the argument that 
> individual tables buy us more flexibility/clarity in the future (it's really 
> just a YAGNI question for me... do we invision an explosion of supported 
> image formats? At most I'd imagine one more in the foreseeable future...).

Hmm, I certainly remember discussing the thrift side but I don't recall 
discussing the correspondent table structure. I admit if feels redundant to 
have separate tables at this point. My concerns are around how it may feel if 
we start adding specific fields into those structs. A single table may become 
harder to reason about, which would only get worse if we added yet another 
image type. If we are not normalizing the thrift side I think we should keep 
the SQL side consistent as well. Once this feature is out, any SQL schema 
changes will require forward/backward roll migration scripts. However, since 
the latter is unavoidable and we will have to eventually deal with it for other 
changes, I am ok shipping it as is. I still encourage you to consider splitting 
`task_config_tables` though.


- Maxim


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


On March 21, 2016, 6:20 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45112/
> ---
> 
> (Updated March 21, 2016, 6:20 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1635
> https://issues.apache.org/jira/browse/AURORA-1635
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add support for storing and fetching images as properties of task configs.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> d4b8904031e6671a8083cac9b82d934377797fe2 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  b3b8ccf868c2a2f18f720a837e90d763072dd3eb 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 364026a8ef2b47cf1beafa3990691d3375516fe6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 12ca16b79a062d9ea15c206ef963fb077ad7ad98 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/DbImage.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
> eb848add00fba6d3571657bb9080be0599b2756a 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  fd272ccf9b1cfccd9198d1e5e0db37d23f546afa 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> be60c3be0cd51d06ba0ca7f56384281ee71d8c55 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  1a520b3cb035a9afc25406d2f313c9f861eee4d6 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java
>  c316e497a34a45c7ada2ca83a1115e826c0f572f 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> e56fed2e6c0cdb47737cf1a9b637c44c5e5b9815 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java
>  08530397ff75081bde6f07f9d53317b5486e0da4 
> 
> Diff: https://reviews.apache.org/r/45112/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> 
> ran e2e tests.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 45112: Add support for storing and fetching images as properties of task configs.

[Maxim Khutornenko]

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


Ship it!




Ship It!

- Maxim Khutornenko


On March 21, 2016, 6:20 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45112/
> ---
> 
> (Updated March 21, 2016, 6:20 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1635
> https://issues.apache.org/jira/browse/AURORA-1635
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add support for storing and fetching images as properties of task configs.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> d4b8904031e6671a8083cac9b82d934377797fe2 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  b3b8ccf868c2a2f18f720a837e90d763072dd3eb 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 364026a8ef2b47cf1beafa3990691d3375516fe6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 12ca16b79a062d9ea15c206ef963fb077ad7ad98 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/DbImage.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
> eb848add00fba6d3571657bb9080be0599b2756a 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  fd272ccf9b1cfccd9198d1e5e0db37d23f546afa 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> be60c3be0cd51d06ba0ca7f56384281ee71d8c55 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  1a520b3cb035a9afc25406d2f313c9f861eee4d6 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java
>  c316e497a34a45c7ada2ca83a1115e826c0f572f 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> e56fed2e6c0cdb47737cf1a9b637c44c5e5b9815 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java
>  08530397ff75081bde6f07f9d53317b5486e0da4 
> 
> Diff: https://reviews.apache.org/r/45112/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> 
> ran e2e tests.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 45112: Add support for storing and fetching images as properties of task configs.

[Joshua Cohen]

> On March 21, 2016, 7:35 p.m., Maxim Khutornenko wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift, line 218
> > 
> >
> > Couldn't find what it maps to in DB, is it not saved?
> 
> Joshua Cohen wrote:
> It's done by `TaskConfigMananger` here: 
> https://reviews.apache.org/r/45112/diff/2#2
> 
> Maxim Khutornenko wrote:
> Ah, I guess `image_id` column name got me thinking it only supports appc. 
> On that note, shouldn't different image types be mapped to dedicated tables 
> to start with? With the move towards db-driven snapshots it may be easier to 
> reason about future schema migrations when there is a 1:1 denormalized 
> mapping between thrift and DB schema.

I did that based on a discussion you and I had when I was initially 
investigating this. I had originally thought to store images using the same 
structure as containers, and we discussed a flattened structure there, rather 
than creating separate tables for each type of container. Given the 
similarities between image definitions as things stand today it doesn't seem 
necessary to have separate tables.

Do you feel differently now? I think this approach is reasonable given what we 
know about the space, but I'm amenable to the argument that individual tables 
buy us more flexibility/clarity in the future (it's really just a YAGNI 
question for me... do we invision an explosion of supported image formats? At 
most I'd imagine one more in the foreseeable future...).


- Joshua


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


On March 21, 2016, 6:20 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45112/
> ---
> 
> (Updated March 21, 2016, 6:20 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1635
> https://issues.apache.org/jira/browse/AURORA-1635
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add support for storing and fetching images as properties of task configs.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> d4b8904031e6671a8083cac9b82d934377797fe2 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  b3b8ccf868c2a2f18f720a837e90d763072dd3eb 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 364026a8ef2b47cf1beafa3990691d3375516fe6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 12ca16b79a062d9ea15c206ef963fb077ad7ad98 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/DbImage.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
> eb848add00fba6d3571657bb9080be0599b2756a 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  fd272ccf9b1cfccd9198d1e5e0db37d23f546afa 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> be60c3be0cd51d06ba0ca7f56384281ee71d8c55 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  1a520b3cb035a9afc25406d2f313c9f861eee4d6 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java
>  c316e497a34a45c7ada2ca83a1115e826c0f572f 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> e56fed2e6c0cdb47737cf1a9b637c44c5e5b9815 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java
>  08530397ff75081bde6f07f9d53317b5486e0da4 
> 
> Diff: https://reviews.apache.org/r/45112/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> 
> ran e2e tests.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 45112: Add support for storing and fetching images as properties of task configs.

[Maxim Khutornenko]

> On March 21, 2016, 7:35 p.m., Maxim Khutornenko wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift, line 218
> > 
> >
> > Couldn't find what it maps to in DB, is it not saved?
> 
> Joshua Cohen wrote:
> It's done by `TaskConfigMananger` here: 
> https://reviews.apache.org/r/45112/diff/2#2

Ah, I guess `image_id` column name got me thinking it only supports appc. On 
that note, shouldn't different image types be mapped to dedicated tables to 
start with? With the move towards db-driven snapshots it may be easier to 
reason about future schema migrations when there is a 1:1 denormalized mapping 
between thrift and DB schema.


- Maxim


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


On March 21, 2016, 6:20 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45112/
> ---
> 
> (Updated March 21, 2016, 6:20 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1635
> https://issues.apache.org/jira/browse/AURORA-1635
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add support for storing and fetching images as properties of task configs.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> d4b8904031e6671a8083cac9b82d934377797fe2 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  b3b8ccf868c2a2f18f720a837e90d763072dd3eb 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 364026a8ef2b47cf1beafa3990691d3375516fe6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 12ca16b79a062d9ea15c206ef963fb077ad7ad98 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/DbImage.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
> eb848add00fba6d3571657bb9080be0599b2756a 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  fd272ccf9b1cfccd9198d1e5e0db37d23f546afa 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> be60c3be0cd51d06ba0ca7f56384281ee71d8c55 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  1a520b3cb035a9afc25406d2f313c9f861eee4d6 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java
>  c316e497a34a45c7ada2ca83a1115e826c0f572f 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> e56fed2e6c0cdb47737cf1a9b637c44c5e5b9815 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java
>  08530397ff75081bde6f07f9d53317b5486e0da4 
> 
> Diff: https://reviews.apache.org/r/45112/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> 
> ran e2e tests.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 45112: Add support for storing and fetching images as properties of task configs.

[Joshua Cohen]

> On March 21, 2016, 7:35 p.m., Maxim Khutornenko wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift, line 218
> > 
> >
> > Couldn't find what it maps to in DB, is it not saved?

It's done by `TaskConfigMananger` here: 
https://reviews.apache.org/r/45112/diff/2#2


- Joshua


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


On March 21, 2016, 6:20 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45112/
> ---
> 
> (Updated March 21, 2016, 6:20 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1635
> https://issues.apache.org/jira/browse/AURORA-1635
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add support for storing and fetching images as properties of task configs.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> d4b8904031e6671a8083cac9b82d934377797fe2 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  b3b8ccf868c2a2f18f720a837e90d763072dd3eb 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 364026a8ef2b47cf1beafa3990691d3375516fe6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 12ca16b79a062d9ea15c206ef963fb077ad7ad98 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/DbImage.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
> eb848add00fba6d3571657bb9080be0599b2756a 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  fd272ccf9b1cfccd9198d1e5e0db37d23f546afa 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> be60c3be0cd51d06ba0ca7f56384281ee71d8c55 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  1a520b3cb035a9afc25406d2f313c9f861eee4d6 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java
>  c316e497a34a45c7ada2ca83a1115e826c0f572f 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> e56fed2e6c0cdb47737cf1a9b637c44c5e5b9815 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java
>  08530397ff75081bde6f07f9d53317b5486e0da4 
> 
> Diff: https://reviews.apache.org/r/45112/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> 
> ran e2e tests.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 45112: Add support for storing and fetching images as properties of task configs.

[Maxim Khutornenko]

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




api/src/main/thrift/org/apache/aurora/gen/api.thrift (line 218)


Couldn't find what it maps to in DB, is it not saved?


- Maxim Khutornenko


On March 21, 2016, 6:20 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45112/
> ---
> 
> (Updated March 21, 2016, 6:20 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1635
> https://issues.apache.org/jira/browse/AURORA-1635
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add support for storing and fetching images as properties of task configs.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> d4b8904031e6671a8083cac9b82d934377797fe2 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  b3b8ccf868c2a2f18f720a837e90d763072dd3eb 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 364026a8ef2b47cf1beafa3990691d3375516fe6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 12ca16b79a062d9ea15c206ef963fb077ad7ad98 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/DbImage.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
> eb848add00fba6d3571657bb9080be0599b2756a 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  fd272ccf9b1cfccd9198d1e5e0db37d23f546afa 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> be60c3be0cd51d06ba0ca7f56384281ee71d8c55 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  1a520b3cb035a9afc25406d2f313c9f861eee4d6 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java
>  c316e497a34a45c7ada2ca83a1115e826c0f572f 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> e56fed2e6c0cdb47737cf1a9b637c44c5e5b9815 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java
>  08530397ff75081bde6f07f9d53317b5486e0da4 
> 
> Diff: https://reviews.apache.org/r/45112/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> 
> ran e2e tests.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 45112: Add support for storing and fetching images as properties of task configs.

[Aurora ReviewBot]

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


Ship it!




Master (b24619b) 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 21, 2016, 6:20 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45112/
> ---
> 
> (Updated March 21, 2016, 6:20 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1635
> https://issues.apache.org/jira/browse/AURORA-1635
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add support for storing and fetching images as properties of task configs.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> d4b8904031e6671a8083cac9b82d934377797fe2 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  b3b8ccf868c2a2f18f720a837e90d763072dd3eb 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 364026a8ef2b47cf1beafa3990691d3375516fe6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 12ca16b79a062d9ea15c206ef963fb077ad7ad98 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/DbImage.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
> eb848add00fba6d3571657bb9080be0599b2756a 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  fd272ccf9b1cfccd9198d1e5e0db37d23f546afa 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> be60c3be0cd51d06ba0ca7f56384281ee71d8c55 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  1a520b3cb035a9afc25406d2f313c9f861eee4d6 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java
>  c316e497a34a45c7ada2ca83a1115e826c0f572f 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> e56fed2e6c0cdb47737cf1a9b637c44c5e5b9815 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java
>  08530397ff75081bde6f07f9d53317b5486e0da4 
> 
> Diff: https://reviews.apache.org/r/45112/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> 
> ran e2e tests.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 45112: Add support for storing and fetching images as properties of task configs.

[Joshua Cohen]

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

(Updated March 21, 2016, 5:46 p.m.)


Review request for Aurora, Maxim Khutornenko and Bill Farner.


Changes
---

+ticket.


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


Repository: aurora


Description
---

Add support for storing and fetching images as properties of task configs.


Diffs
-

  api/src/main/thrift/org/apache/aurora/gen/api.thrift 
d4b8904031e6671a8083cac9b82d934377797fe2 
  
src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
 b3b8ccf868c2a2f18f720a837e90d763072dd3eb 
  src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
364026a8ef2b47cf1beafa3990691d3375516fe6 
  src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
12ca16b79a062d9ea15c206ef963fb077ad7ad98 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/DbImage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
eb848add00fba6d3571657bb9080be0599b2756a 
  
src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml 
fd272ccf9b1cfccd9198d1e5e0db37d23f546afa 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
be60c3be0cd51d06ba0ca7f56384281ee71d8c55 
  
src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
 1a520b3cb035a9afc25406d2f313c9f861eee4d6 
  
src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java 
c316e497a34a45c7ada2ca83a1115e826c0f572f 
  src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
e56fed2e6c0cdb47737cf1a9b637c44c5e5b9815 
  
src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java 
08530397ff75081bde6f07f9d53317b5486e0da4 

Diff: https://reviews.apache.org/r/45112/diff/


Testing
---

./gradlew build -Pq

ran e2e tests.


Thanks,

Joshua Cohen