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



src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java
<https://reviews.apache.org/r/24521/#comment88039>

    As we discussed offline - let's remove this and require the caller to 
follow up with saveJobUpdateEvent when they want to.  If the caller neglects to 
do this, i think it's fine for the subsequent selects to have trouble (for now, 
at least).



src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java
<https://reviews.apache.org/r/24521/#comment88040>

    instanceOverrides.isEmpty() rather than Iterables.isEmpty



src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java
<https://reviews.apache.org/r/24521/#comment88042>

    Should be able to revert this since DBJobUpdateStore will no longer require 
a clock



src/main/java/org/apache/aurora/scheduler/storage/db/InsertResult.java
<https://reviews.apache.org/r/24521/#comment88045>

    Nice.  Suggested rewording:
    
    "MyBatis returns auto-generated IDs through mutable fields in parameters.  
This class can be used as an additional {@link 
org.apache.ibatis.annotations.Param Param} to retrieve the ID when the inserted 
object is not self-identifying."



src/main/java/org/apache/aurora/scheduler/storage/db/InsertResult.java
<https://reviews.apache.org/r/24521/#comment88043>

    remove extra newline



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

    {@code false}, {@code true}



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

    "Container for auto-generated ID of the inserted job update row."



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

    Can this be primitive boolean instead?



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

    "Instance ID ranges to associate with a task configuration."



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

    s/Long/long/?



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

    In general, avoid javadoc that re-states the signature in english.
    
    "@return Job update summaries matching the query."



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

    @return All stored job update details.



src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/AbstractTBaseTypeHandler.java
<https://reviews.apache.org/r/24521/#comment88063>

    Please add a disclaimer here:
    
    <p>
    NOTE: We don't want store serialized thrift objects long-term, but instead 
plan to reference a canonical table of task configurations. This class will go 
away with AURORA-647.



src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/AbstractTBaseTypeHandler.java
<https://reviews.apache.org/r/24521/#comment88066>

    Unless there's a need right now, don't even provide an abstract base - this 
is technical debt we shouldn't encourage further.



src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/AbstractTBaseTypeHandler.java
<https://reviews.apache.org/r/24521/#comment88064>

    Thrift binary-encoded byte array.



src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TaskConfigTypeHandler.java
<https://reviews.apache.org/r/24521/#comment88065>

    s/  / /



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

    A comment explaining the "AS" table naming convention would be really nice. 
 Clearly there's a reason, so it's nice to give a brief explanation.



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

    Why IN rather than =?  If this is legit, please drop in a comment.



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

    s/BIGINT //



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

    s/BIGINT //



src/main/thrift/org/apache/aurora/gen/api.thrift
<https://reviews.apache.org/r/24521/#comment88075>

    Instance IDs to act on. All instances will be affected if this is not set.



src/main/thrift/org/apache/aurora/gen/api.thrift
<https://reviews.apache.org/r/24521/#comment88076>

    s/User/User who/



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

    Query for an empty set of statuses would be a nice corner case to try as 
well.



src/test/java/org/apache/aurora/scheduler/storage/db/DbUtil.java
<https://reviews.apache.org/r/24521/#comment88078>

    AFAICT this can go away now as well.


- Bill Farner


On Aug. 12, 2014, 6:58 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24521/
> -----------------------------------------------------------
> 
> (Updated Aug. 12, 2014, 6:58 p.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 support for storing task configs and updateOnlyTheseInstances option.
> Implementing the rest of defined JobUpdateStore APIs.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 
> a9a325b877898be8a265c45112757deba0c3583f 
>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
> 808595817c31f3ef3515b623bbeb575bbf1f73fe 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 
> 87f428be892204b8149170d39d3ace2962abc4bd 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> 2a75646867a5af246625f7cf3910fd9a1bbf4f03 
>   src/main/java/org/apache/aurora/scheduler/storage/db/InsertResult.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java
>  5b7a3df035e78832ba4e6b595202d06af5d664a5 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/AbstractTBaseTypeHandler.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TaskConfigTypeHandler.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java
>  c5468b16709e7bc4758a0597bbe14257b31686ab 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml 
> ce4912d70fe75ea97f3d1bfb7fc4d5ec437fc83b 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml
>  3c69bda9fcb473205a3e308b15ad18cd1ae3cba7 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> f0c8336cde4d262bcaf99921c556875ba819b05c 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> 4ea9ec2b96fc12429f33336b53e677e12662ec9a 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  c76ab5cbde907a352dce25da585951831733487d 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DbAttributeStoreTest.java
>  4c8238955464960e14ab858dcff7a53a64fcd72a 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
> 7ed6691f497ebaaaeb43d522af1358cc0b9491af 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbQuotaStoreTest.java 
> 1f0af8669b655f773faa13a69887c6b0eb57dc57 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DbSchedulerStoreTest.java
>  64e2fee337be597f6af4ffaa356872d34d316530 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbUtil.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 548322be029c6663a75187d9f341e53c5b7ca416 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java
>  cbb9c468bf64691e573762f3734528c7b2e16490 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 6bc6ccf37a34bfe6baf260034584be758a4c83f5 
> 
> Diff: https://reviews.apache.org/r/24521/diff/
> 
> 
> Testing
> -------
> 
> gradle -Pq build
> ./pants src/test/python/apache/aurora/client/api:scheduler_client
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Reply via email to