> On July 1, 2014, 4:55 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml,
> >  lines 6-14
> > <https://reviews.apache.org/r/23105/diff/1/?file=619259#file619259line6>
> >
> >     Do you really need this block? I thought EnumValueMapper takes care of 
> > this already.

Thank you.  This is pre-refactor cruft.  Removed.


> On July 1, 2014, 4:55 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml,
> >  line 18
> > <https://reviews.apache.org/r/23105/diff/1/?file=619259#file619259line18>
> >
> >     Pretty sure you can omit id column here.

Right you are, removed.


> On July 1, 2014, 4:55 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml,
> >  line 7
> > <https://reviews.apache.org/r/23105/diff/1/?file=619260#file619260line7>
> >
> >     Should this be a MERGE instead to simplify re-population case? 
> > Otherwise, I am not sure how a re-run of the population loop in DBStorage 
> > is going to be handled here.

The re-population case crossed my mind, but i figured it would better for it to 
fail early if that happens.  Otherwise you run the risk of crufty deleted 
values in the database.  In other words, if this code is used for a persistent 
database, i want it to break so that it must be re-thought.


> On July 1, 2014, 4:55 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql, lines 
> > 48-49
> > <https://reviews.apache.org/r/23105/diff/1/?file=619262#file619262line48>
> >
> >     You probably want UNIQUE(name) here as well.

Good call, done.


> On July 1, 2014, 4:55 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql, lines 
> > 58-59
> > <https://reviews.apache.org/r/23105/diff/1/?file=619262#file619262line58>
> >
> >     Is there a legitimate case when UNIQUE(host, slave_id) would not 
> > suffice here?
> 
> Kevin Sweeney wrote:
>     I think you need to drop the UNIQUE(host) constraint - only slave_id is 
> necessarily unique. multiple mesos slaves could have the same hostname, and 
> slave_id is persistent across slave restarts that don't result in reboots / 
> recovery info corruption (at which point you want to invalidate attribute 
> storage anyway since they'll potentially have changed)

I wanted to avoid changing existing API semantics here, but position internally 
for it.  That's why i've made the foreign key use slave_id rather than host.  
However, i don't want to risk having a semantic change throw a wrench in this 
refactor.  For that reason, i used UNIQUE(host) for matching behavior.


- Bill


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


On June 27, 2014, 3:39 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23105/
> -----------------------------------------------------------
> 
> (Updated June 27, 2014, 3:39 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-557
>     https://issues.apache.org/jira/browse/AURORA-557
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> There are a few different characteristics of this mapper compared to others 
> so far:
> - custom type handler (see AbstractTEnumTypeHandler and 
> MaintenanceModeTypeHandler)
> - outer join (to allow a HostAttributes with an empty Attributes set)
> - batch insert via foreach
> - collection, and nested collection result mapping
> 
> You may find this page helpful to explain the features used: 
> http://mybatis.github.io/mybatis-3/sqlmap-xml.html#Result_Maps
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/AttributeMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbAttributeStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> 1738b95cd67cf990bd8aad8c744a1febe2d87f15 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> c683e398640c7ebf2047ef308a701cb4897c58dc 
>   src/main/java/org/apache/aurora/scheduler/storage/db/EnumValueMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> 505c94d6800c1453b1b1f696ef774f5943973f19 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/AbstractTEnumTypeHandler.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/MaintenanceModeTypeHandler.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/mem/MemAttributeStore.java 
> 4bb807c04e47a091c83a575850ebfc3b244bfa73 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 65750b61b864f0e830513039a7c9d727ac9d493d 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 23555c2483d7fe716243847f8478898e98fb5ac4 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml 
> PRE-CREATION 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/EnumValueMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
> 31b98cb3107a88756694922de01fa0ba267f3e9d 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> 3298eb38644b6fa7096801a69f8b88d0331ce4a7 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DbAttributeStoreTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23105/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew run -Pq
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Reply via email to