> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java, 
> > line 75
> > <https://reviews.apache.org/r/24521/diff/4/?file=658554#file658554line75>
> >
> >     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).

Addressed in previous diff.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java, 
> > line 88
> > <https://reviews.apache.org/r/24521/diff/4/?file=658554#file658554line88>
> >
> >     instanceOverrides.isEmpty() rather than Iterables.isEmpty

Done.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 93
> > <https://reviews.apache.org/r/24521/diff/4/?file=658555#file658555line93>
> >
> >     Should be able to revert this since DBJobUpdateStore will no longer 
> > require a clock

Addressed in previous diff.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/InsertResult.java, 
> > line 17
> > <https://reviews.apache.org/r/24521/diff/4/?file=658556#file658556line17>
> >
> >     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."

Done.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/InsertResult.java, 
> > line 20
> > <https://reviews.apache.org/r/24521/diff/4/?file=658556#file658556line20>
> >
> >     remove extra newline

Done.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java,
> >  line 48
> > <https://reviews.apache.org/r/24521/diff/4/?file=658557#file658557line48>
> >
> >     {@code false}, {@code true}

Done.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java,
> >  line 49
> > <https://reviews.apache.org/r/24521/diff/4/?file=658557#file658557line49>
> >
> >     "Container for auto-generated ID of the inserted job update row."

Done.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java,
> >  line 54
> > <https://reviews.apache.org/r/24521/diff/4/?file=658557#file658557line54>
> >
> >     Can this be primitive boolean instead?

Yes it can.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java,
> >  line 61
> > <https://reviews.apache.org/r/24521/diff/4/?file=658557#file658557line61>
> >
> >     "Instance ID ranges to associate with a task configuration."

Done.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java,
> >  line 64
> > <https://reviews.apache.org/r/24521/diff/4/?file=658557#file658557line64>
> >
> >     s/Long/long/?

Done.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java,
> >  line 104
> > <https://reviews.apache.org/r/24521/diff/4/?file=658557#file658557line104>
> >
> >     @return All stored job update details.

Done.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java,
> >  line 88
> > <https://reviews.apache.org/r/24521/diff/4/?file=658557#file658557line88>
> >
> >     In general, avoid javadoc that re-states the signature in english.
> >     
> >     "@return Job update summaries matching the query."

Done.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/AbstractTBaseTypeHandler.java,
> >  line 29
> > <https://reviews.apache.org/r/24521/diff/4/?file=658558#file658558line29>
> >
> >     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.

Added.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/AbstractTBaseTypeHandler.java,
> >  line 33
> > <https://reviews.apache.org/r/24521/diff/4/?file=658558#file658558line33>
> >
> >     Unless there's a need right now, don't even provide an abstract base - 
> > this is technical debt we shouldn't encourage further.

Dropped.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TaskConfigTypeHandler.java,
> >  line 23
> > <https://reviews.apache.org/r/24521/diff/4/?file=658559#file658559line23>
> >
> >     s/  / /

Gone.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/AbstractTBaseTypeHandler.java,
> >  line 38
> > <https://reviews.apache.org/r/24521/diff/4/?file=658558#file658558line38>
> >
> >     Thrift binary-encoded byte array.

Gone.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml,
> >  line 263
> > <https://reviews.apache.org/r/24521/diff/4/?file=658562#file658562line263>
> >
> >     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.

Added.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml,
> >  line 313
> > <https://reviews.apache.org/r/24521/diff/4/?file=658562#file658562line313>
> >
> >     Why IN rather than =?  If this is legit, please drop in a comment.

No preference. Either works fine. 


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql, line 
> > 122
> > <https://reviews.apache.org/r/24521/diff/4/?file=658563#file658563line122>
> >
> >     s/BIGINT //

Ah, thanks, missed these two on rebase.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 566
> > <https://reviews.apache.org/r/24521/diff/4/?file=658564#file658564line566>
> >
> >     Instance IDs to act on. All instances will be affected if this is not 
> > set.

Added.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 697
> > <https://reviews.apache.org/r/24521/diff/4/?file=658564#file658564line697>
> >
> >     s/User/User who/

Done.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java,
> >  line 332
> > <https://reviews.apache.org/r/24521/diff/4/?file=658565#file658565line332>
> >
> >     Query for an empty set of statuses would be a nice corner case to try 
> > as well.

It feels like an empty value should be treated as invalid and result in no 
match. However, the TaskQuery seems to be treating null and empty statuses the 
same way, so making it consistent here as well.


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DbUtil.java, line 24
> > <https://reviews.apache.org/r/24521/diff/4/?file=658570#file658570line24>
> >
> >     AFAICT this can go away now as well.

There is still some repetition involved in every test file to initialize the 
storage. I'd rather keep it as it cleans up the setup methods a bit. 


- Maxim


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


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