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



src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java
<https://reviews.apache.org/r/24431/#comment87252>

    s/org.apache.aurora.gen.//



src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java
<https://reviews.apache.org/r/24431/#comment87253>

    ditto
    
    s/if/if it/



src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml
<https://reviews.apache.org/r/24431/#comment87254>

    -2 indent



src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml
<https://reviews.apache.org/r/24431/#comment87255>

    If we're going to generate our own IDs, we should really let those be the 
foreign key references.  However, i think now is a good time to revisit using 
the DB's generated ID.
    
    As far as i can tell, the current structure of JobUpdate works with this, 
since a JobUpdate is self-identifying.  
    
    For objects that are not self-identifying, it looks like this would work:
    
http://stackoverflow.com/questions/18507508/mybatis-how-to-get-the-auto-generated-key-of-an-insert-mysql
    
    In this case, insert() accepts Map<String, Object>, where one key points to 
the POJO, the other points to the returned autogenerated key.  You would need 
this snippet below the insert statement:
    
      <selectKey resultType="long" order="AFTER" keyProperty="returnedId">
        SELECT LAST_INSERT_ID()
      </selectKey>
    
    Not terribly elegant, but avoids broader exposure of this limitation.
    



src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml
<https://reviews.apache.org/r/24431/#comment87251>

    After this change, can you go back and update DbLockStore to not catch 
PersistenceException?



src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml
<https://reviews.apache.org/r/24431/#comment87256>

    If you set columnPrefix="j_", you might be able to let mybatis' POJO 
mapping take care of the individual fields.  I _think_ you'll still need to 
specify <id> though.  If this works here, the approach might apply to other 
resultMaps in here as well.



src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml
<https://reviews.apache.org/r/24431/#comment87257>

    notNullColumn is new for us, can you add a comment describing why it is 
used?



src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml
<https://reviews.apache.org/r/24431/#comment87258>

    columnPrefix on the <collection> should make fields auto-amp, with the 
exception of those that need special handling (id, typeHandler).



src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml
<https://reviews.apache.org/r/24431/#comment87259>

    Ditto re: columnPrefix



src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml
<https://reviews.apache.org/r/24431/#comment87261>

    Should we start leaning on the database and let 'ON DELETE CASCADE' handle 
child tables appropriately?  I realize now i did not do this in 
AttributeMapper.xml, but i think it's worth considering.



src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml
<https://reviews.apache.org/r/24431/#comment87260>

    remove newline



src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql
<https://reviews.apache.org/r/24431/#comment87262>

    i think you can s/BIGINT //, AFAICT h2 uses a long regardless.  Not a bad 
idea to do this without.



src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
<https://reviews.apache.org/r/24431/#comment87268>

    A test case for two updates with the full slew of child entries would be 
nice, to prove that they don't cross wires (lesson learned from my recent JOIN 
flub).



src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
<https://reviews.apache.org/r/24431/#comment87264>

    please check that these objects are valid, not just present (equality 
rather than size).
    
    this implicitly tests the ordering, so you can skip the checks below.



src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
<https://reviews.apache.org/r/24431/#comment87267>

    You could do this in an @After as a catch-all for every test case.



src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
<https://reviews.apache.org/r/24431/#comment87266>

    In general, favor:
    
      assertEquals(Optional.<Foo>absent(), actual);
    
    over:
    
      assertFalse(actual.isPresent());
    
    The difference is subtle, but the former produces test output that shows 
the incorrect value, and will point more directly towards the problem.


- Bill Farner


On Aug. 7, 2014, 12:01 a.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24431/
> -----------------------------------------------------------
> 
> (Updated Aug. 7, 2014, 12:01 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Bill Farner.
> 
> 
> Bugs: AURORA-612
>     https://issues.apache.org/jira/browse/AURORA-612
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Adding SQL support for saving updates and fetching update details. 
> 
> The configuration and updateOnlyTheseInstances option support will come 
> separately. 
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 
> 869590ed3c43c64936af42e60d82f5a6938475d3 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> 5d6521813e40eed5cf7e6e0285a7400624c4c5b2 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> 8a573f7b04ecc37a19e3886f334f626af577839a 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.java
>  PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> 844714c08cdaf280c8862e933a95ab009bd9e464 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.java
>  PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
> 618d5b72fd3078ef59571b5f2e33ecce3eb7386c 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java
>  f014123348e2b229b2bb9be0dce424327ec0dfbe 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml
>  PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> 3997f9833a7289a49e4f4972efc3f953674bd41b 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml
>  PRE-CREATION 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.xml
>  PRE-CREATION 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> eb2216ae22ae95370d0b5cd15dbb3d61fca79272 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/24431/diff/
> 
> 
> Testing
> -------
> 
> gradle -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Reply via email to