Re: Review Request 24521: Finalizing DB mapper implementation.

2014-08-12 Thread Maxim Khutornenko 

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

(Updated Aug. 12, 2014, 10:01 p.m.)


Review request for Aurora, David McLaughlin and Bill Farner.


Changes
---

CR comments.


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 (updated)
-

  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/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/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 
4ea9ec2b96fc12429f6b53e677e12662ec9a 
  
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

Re: Review Request 24521: Finalizing DB mapper implementation.

2014-08-12 Thread Maxim Khutornenko 


> On Aug. 12, 2014, 9:11 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java, line 
> > 62
> > 
> >
> > just , since p is a non-void HTML tag

Done.


> On Aug. 12, 2014, 9:11 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 82
> > 
> >
> > revert

Done.


> On Aug. 12, 2014, 9:11 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/InsertResult.java, 
> > line 24
> > 
> >
> > You might consider building in a gate here and throwing 
> > IllegalStateException if setId() was not called (avoid spread of bad data).

Good idea. Done.


> On Aug. 12, 2014, 9:11 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml,
> >  line 324
> > 
> >
> > This was clearly intentional, please document your reasoning.

Done.


> On Aug. 12, 2014, 9:11 p.m., Bill Farner wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 600
> > 
> >
> > Better doc?  Also, this is identical to the doc for JobUpdateSummary, 
> > and they're definitely not the same thing.

Thanks, clarified.


> On Aug. 12, 2014, 9:11 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java,
> >  line 63
> > 
> >
> > This feels like a holdover from the previous diff.  How about 
> > s/DEFAULT/FIRST/, and s/INIT/ROLLING_FORWARD/?

Modified and dropped INIT status from JobUpdateStatus enum.


> On Aug. 12, 2014, 9:11 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java,
> >  line 119
> > 
> >
> > Doesn't this duplicate the test case above?

Not really. The above saves with empty set and this one saves with a null set. 
In both cases the result on select is an empty set.


> On Aug. 12, 2014, 9:11 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java,
> >  line 148
> > 
> >
> > This makes for a nicer error message on failures:
> > 
> > assertEquals(ImmutableList.of(DEFAULT_EVENT), getUpdateDetails(...));

Changed.


- Maxim


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


On Aug. 12, 2014, 8:26 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, 8:26 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/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/sche

Re: Review Request 24521: Finalizing DB mapper implementation.

2014-08-12 Thread Bill Farner 

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

Ship it!



src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java


just , since p is a non-void HTML tag



src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java


revert



src/main/java/org/apache/aurora/scheduler/storage/db/InsertResult.java


You might consider building in a gate here and throwing 
IllegalStateException if setId() was not called (avoid spread of bad data).



src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml


This was clearly intentional, please document your reasoning.



src/main/thrift/org/apache/aurora/gen/api.thrift


Better doc?  Also, this is identical to the doc for JobUpdateSummary, and 
they're definitely not the same thing.



src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java


This feels like a holdover from the previous diff.  How about 
s/DEFAULT/FIRST/, and s/INIT/ROLLING_FORWARD/?



src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java


Doesn't this duplicate the test case above?



src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java


This makes for a nicer error message on failures:

assertEquals(ImmutableList.of(DEFAULT_EVENT), getUpdateDetails(...));


- Bill Farner


On Aug. 12, 2014, 8:26 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, 8:26 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/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 
> 4ea9ec2b96fc12429f6b53e677e12662ec9a 
>   
> 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/SnapshotStoreI

Re: Review Request 24521: Finalizing DB mapper implementation.

2014-08-12 Thread Maxim Khutornenko 


> On Aug. 12, 2014, 7:27 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java, 
> > line 75
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > {@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
> > 
> >
> > "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
> > 
> >
> > 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
> > 
> >
> > "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
> > 
> >
> > 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
> > 
> >
> > @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
> > 
> >
> > 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
> > 
> >
> > Please add a disclaimer here:
> > 
> > 
> > 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
> > 
> >
> > 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, 20

Re: Review Request 24521: Finalizing DB mapper implementation.

2014-08-12 Thread Maxim Khutornenko 

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

(Updated Aug. 12, 2014, 8:26 p.m.)


Review request for Aurora, David McLaughlin and Bill Farner.


Changes
---

CR comments.


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 (updated)
-

  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/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 
4ea9ec2b96fc12429f6b53e677e12662ec9a 
  
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

Re: Review Request 24521: Finalizing DB mapper implementation.

2014-08-12 Thread Bill Farner 

---
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


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


instanceOverrides.isEmpty() rather than Iterables.isEmpty



src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java


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


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


remove extra newline



src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java


{@code false}, {@code true}



src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java


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



src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java


Can this be primitive boolean instead?



src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java


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



src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java


s/Long/long/?



src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java


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


@return All stored job update details.



src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/AbstractTBaseTypeHandler.java


Please add a disclaimer here:


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


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


Thrift binary-encoded byte array.



src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TaskConfigTypeHandler.java


s/  / /



src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml


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


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



src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql


s/BIGINT //



src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql


s/BIGINT //



src/main/thrift/org/apache/aurora/gen/api.thrift


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



src/main/thrift/org/apache/aurora/gen/api.thrift

Re: Review Request 24521: Finalizing DB mapper implementation.

2014-08-12 Thread Maxim Khutornenko 

---
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.


Changes
---

Dropping support for JobUpdateState in saveUpdate(). 

Any call to saveUpdate() must now be followed up with the saveJobEvent() in 
order for the fetch call to retrieve anything.


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 (updated)
-

  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 
4ea9ec2b96fc12429f6b53e677e12662ec9a 
  
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

Re: Review Request 24521: Finalizing DB mapper implementation.

2014-08-12 Thread Maxim Khutornenko 

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

(Updated Aug. 12, 2014, 4:28 p.m.)


Review request for Aurora, David McLaughlin and Bill Farner.


Changes
---

Converting status, created and lastModified timestamp fields to be 
auto-populated based off of job/instance events. This enables user-requested as 
well as log recovery saveUpdate calls.


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 (updated)
-

  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 
4ea9ec2b96fc12429f6b53e677e12662ec9a 
  
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

Re: Review Request 24521: Finalizing DB mapper implementation.

2014-08-11 Thread David McLaughlin 

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

Ship it!


lgtm.

- David McLaughlin


On Aug. 11, 2014, 5:45 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24521/
> ---
> 
> (Updated Aug. 11, 2014, 5:45 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/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 
> 4ea9ec2b96fc12429f6b53e677e12662ec9a 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  c76ab5cbde907a352dce25da585951831733487d 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 548322be029c6663a75187d9f341e53c5b7ca416 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java
>  cbb9c468bf64691e573762f3734528c7b2e16490 
> 
> 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
> 
>

Re: Review Request 24521: Finalizing DB mapper implementation.

2014-08-11 Thread David McLaughlin 


> On Aug. 9, 2014, 12:48 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java, line 
> > 40
> > 
> >
> > Drive-by partial review, but this seems like an API regression.  Do you 
> > expect duplicates in the result?
> 
> Maxim Khutornenko wrote:
> This is the only way to preserve ordering in the result set. I am happy 
> to revert it if we decide to sort on the client. It's a bit weird to support 
> paging with partial results coming out of order though.
> 
> David McLaughlin wrote:
> +1 for keeping it a list.
> 
> Bill Farner wrote:
> We can guarantee order with a Set (ImmutableSet.copyOf does this).  I do 
> think it's fair to force the client to sort if they want to guarantee sorted 
> order though (especially since we don't accept a sort directive).
> 
> Maxim Khutornenko wrote:
> The order will not be guaranteed with python client when data gets 
> through thrift serialization. We should either keep List and document the 
> order by lastModifiedTS or revert to Set and drop any guarantees. I am fine 
> either way.

I think it's wasteful to force the client to sort when any paginated API 
implies that the result sets are sorted on the server first. 


- David


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


On Aug. 11, 2014, 5:45 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24521/
> ---
> 
> (Updated Aug. 11, 2014, 5:45 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/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 
> 4ea9ec2b96fc12429f6b53e677e12662ec9a 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  c76ab5cbde907a352dce25da585951831733487d 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 548322be029c6663a75187d9f341e53c5b7ca416 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java
>  cbb9c468bf64691e573762f3734528c7b2e16490 
> 
> 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
> 
>

Re: Review Request 24521: Finalizing DB mapper implementation.

2014-08-11 Thread Maxim Khutornenko 


> On Aug. 9, 2014, 12:48 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java, line 
> > 40
> > 
> >
> > Drive-by partial review, but this seems like an API regression.  Do you 
> > expect duplicates in the result?
> 
> Maxim Khutornenko wrote:
> This is the only way to preserve ordering in the result set. I am happy 
> to revert it if we decide to sort on the client. It's a bit weird to support 
> paging with partial results coming out of order though.
> 
> David McLaughlin wrote:
> +1 for keeping it a list.
> 
> Bill Farner wrote:
> We can guarantee order with a Set (ImmutableSet.copyOf does this).  I do 
> think it's fair to force the client to sort if they want to guarantee sorted 
> order though (especially since we don't accept a sort directive).

The order will not be guaranteed with python client when data gets through 
thrift serialization. We should either keep List and document the order by 
lastModifiedTS or revert to Set and drop any guarantees. I am fine either way. 


- Maxim


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


On Aug. 11, 2014, 5:45 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24521/
> ---
> 
> (Updated Aug. 11, 2014, 5:45 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/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 
> 4ea9ec2b96fc12429f6b53e677e12662ec9a 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  c76ab5cbde907a352dce25da585951831733487d 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 548322be029c6663a75187d9f341e53c5b7ca416 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java
>  cbb9c468bf64691e573762f3734528c7b2e16490 
> 
> 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
> 
>

Re: Review Request 24521: Finalizing DB mapper implementation.

2014-08-11 Thread Bill Farner 


> On Aug. 9, 2014, 12:48 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java, line 
> > 40
> > 
> >
> > Drive-by partial review, but this seems like an API regression.  Do you 
> > expect duplicates in the result?
> 
> Maxim Khutornenko wrote:
> This is the only way to preserve ordering in the result set. I am happy 
> to revert it if we decide to sort on the client. It's a bit weird to support 
> paging with partial results coming out of order though.
> 
> David McLaughlin wrote:
> +1 for keeping it a list.

We can guarantee order with a Set (ImmutableSet.copyOf does this).  I do think 
it's fair to force the client to sort if they want to guarantee sorted order 
though (especially since we don't accept a sort directive).


- Bill


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


On Aug. 11, 2014, 5:45 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24521/
> ---
> 
> (Updated Aug. 11, 2014, 5:45 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/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 
> 4ea9ec2b96fc12429f6b53e677e12662ec9a 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  c76ab5cbde907a352dce25da585951831733487d 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 548322be029c6663a75187d9f341e53c5b7ca416 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java
>  cbb9c468bf64691e573762f3734528c7b2e16490 
> 
> 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
> 
>

Re: Review Request 24521: Finalizing DB mapper implementation.

2014-08-11 Thread David McLaughlin 


> On Aug. 9, 2014, 12:48 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java, line 
> > 40
> > 
> >
> > Drive-by partial review, but this seems like an API regression.  Do you 
> > expect duplicates in the result?
> 
> Maxim Khutornenko wrote:
> This is the only way to preserve ordering in the result set. I am happy 
> to revert it if we decide to sort on the client. It's a bit weird to support 
> paging with partial results coming out of order though.

+1 for keeping it a list. 


- David


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


On Aug. 11, 2014, 5:45 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24521/
> ---
> 
> (Updated Aug. 11, 2014, 5:45 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/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 
> 4ea9ec2b96fc12429f6b53e677e12662ec9a 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  c76ab5cbde907a352dce25da585951831733487d 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 548322be029c6663a75187d9f341e53c5b7ca416 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java
>  cbb9c468bf64691e573762f3734528c7b2e16490 
> 
> 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
> 
>

Re: Review Request 24521: Finalizing DB mapper implementation.

2014-08-11 Thread Maxim Khutornenko 

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

(Updated Aug. 11, 2014, 5:45 p.m.)


Review request for Aurora, David McLaughlin and Bill Farner.


Changes
---

Making updateOnlyTheseInstances truly optional.


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 (updated)
-

  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/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 
4ea9ec2b96fc12429f6b53e677e12662ec9a 
  
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 
c76ab5cbde907a352dce25da585951831733487d 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
548322be029c6663a75187d9f341e53c5b7ca416 
  
src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java
 cbb9c468bf64691e573762f3734528c7b2e16490 

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

Re: Review Request 24521: Finalizing DB mapper implementation.

2014-08-08 Thread Maxim Khutornenko 


> On Aug. 9, 2014, 12:48 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java, line 
> > 40
> > 
> >
> > Drive-by partial review, but this seems like an API regression.  Do you 
> > expect duplicates in the result?

This is the only way to preserve ordering in the result set. I am happy to 
revert it if we decide to sort on the client. It's a bit weird to support 
paging with partial results coming out of order though.


- Maxim


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


On Aug. 9, 2014, 12:15 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24521/
> ---
> 
> (Updated Aug. 9, 2014, 12:15 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 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/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 
> 4ea9ec2b96fc12429f6b53e677e12662ec9a 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  c76ab5cbde907a352dce25da585951831733487d 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 548322be029c6663a75187d9f341e53c5b7ca416 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java
>  cbb9c468bf64691e573762f3734528c7b2e16490 
> 
> 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
> 
>

Re: Review Request 24521: Finalizing DB mapper implementation.

2014-08-08 Thread Bill Farner 

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



src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java


Drive-by partial review, but this seems like an API regression.  Do you 
expect duplicates in the result?


- Bill Farner


On Aug. 9, 2014, 12:15 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24521/
> ---
> 
> (Updated Aug. 9, 2014, 12:15 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 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/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 
> 4ea9ec2b96fc12429f6b53e677e12662ec9a 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  c76ab5cbde907a352dce25da585951831733487d 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 548322be029c6663a75187d9f341e53c5b7ca416 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java
>  cbb9c468bf64691e573762f3734528c7b2e16490 
> 
> 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
> 
>

Re: Review Request 24521: Finalizing DB mapper implementation.

2014-08-08 Thread Maxim Khutornenko 

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

(Updated Aug. 9, 2014, 12:15 a.m.)


Review request for Aurora, David McLaughlin and Bill Farner.


Changes
---

Adding missing comments.


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 (updated)
-

  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/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 
4ea9ec2b96fc12429f6b53e677e12662ec9a 
  
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 
c76ab5cbde907a352dce25da585951831733487d 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
548322be029c6663a75187d9f341e53c5b7ca416 
  
src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java
 cbb9c468bf64691e573762f3734528c7b2e16490 

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