> On March 21, 2016, 7:35 p.m., Maxim Khutornenko wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift, line 218
> > <https://reviews.apache.org/r/45112/diff/2/?file=1308743#file1308743line218>
> >
> >     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
> 
>

Reply via email to