You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2014/08/09 02:14:38 UTC
Review Request 24521: Finalizing DB mapper implementation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24521/
-----------------------------------------------------------
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 4ea9ec2b96fc12429f33336b53e677e12662ec9a
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.
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
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 4ea9ec2b96fc12429f33336b53e677e12662ec9a
> 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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
>
>
Re: Review Request 24521: Finalizing DB mapper implementation.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 24521: Finalizing DB mapper implementation.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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 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
Re: Review Request 24521: Finalizing DB mapper implementation.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 12, 2014, 9:11 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java, line 62
> > <https://reviews.apache.org/r/24521/diff/6/?file=658774#file658774line62>
> >
> > just <p>, 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
> > <https://reviews.apache.org/r/24521/diff/6/?file=658776#file658776line82>
> >
> > 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
> > <https://reviews.apache.org/r/24521/diff/6/?file=658777#file658777line24>
> >
> > 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
> > <https://reviews.apache.org/r/24521/diff/6/?file=658782#file658782line324>
> >
> > 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
> > <https://reviews.apache.org/r/24521/diff/6/?file=658784#file658784line600>
> >
> > 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
> > <https://reviews.apache.org/r/24521/diff/6/?file=658785#file658785line63>
> >
> > 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
> > <https://reviews.apache.org/r/24521/diff/6/?file=658785#file658785line119>
> >
> > 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
> > <https://reviews.apache.org/r/24521/diff/6/?file=658785#file658785line148>
> >
> > 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/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
>
>
Re: Review Request 24521: Finalizing DB mapper implementation.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/24521/#comment88112>
just <p>, since p is a non-void HTML tag
src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java
<https://reviews.apache.org/r/24521/#comment88113>
revert
src/main/java/org/apache/aurora/scheduler/storage/db/InsertResult.java
<https://reviews.apache.org/r/24521/#comment88114>
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
<https://reviews.apache.org/r/24521/#comment88118>
This was clearly intentional, please document your reasoning.
src/main/thrift/org/apache/aurora/gen/api.thrift
<https://reviews.apache.org/r/24521/#comment88119>
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
<https://reviews.apache.org/r/24521/#comment88124>
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
<https://reviews.apache.org/r/24521/#comment88123>
Doesn't this duplicate the test case above?
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
<https://reviews.apache.org/r/24521/#comment88121>
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 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
>
>
Re: Review Request 24521: Finalizing DB mapper implementation.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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 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
Re: Review Request 24521: Finalizing DB mapper implementation.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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 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
Re: Review Request 24521: Finalizing DB mapper implementation.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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 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
Re: Review Request 24521: Finalizing DB mapper implementation.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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 4ea9ec2b96fc12429f33336b53e677e12662ec9a
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.
Posted by David McLaughlin <da...@dmclaughlin.com>.
> On Aug. 9, 2014, 12:48 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java, line 40
> > <https://reviews.apache.org/r/24521/diff/2/?file=656929#file656929line40>
> >
> > 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 4ea9ec2b96fc12429f33336b53e677e12662ec9a
> 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 9, 2014, 12:48 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java, line 40
> > <https://reviews.apache.org/r/24521/diff/2/?file=656929#file656929line40>
> >
> > 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 4ea9ec2b96fc12429f33336b53e677e12662ec9a
> 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 9, 2014, 12:48 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java, line 40
> > <https://reviews.apache.org/r/24521/diff/2/?file=656929#file656929line40>
> >
> > 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 4ea9ec2b96fc12429f33336b53e677e12662ec9a
> 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.
Posted by David McLaughlin <da...@dmclaughlin.com>.
> On Aug. 9, 2014, 12:48 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java, line 40
> > <https://reviews.apache.org/r/24521/diff/2/?file=656929#file656929line40>
> >
> > 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 4ea9ec2b96fc12429f33336b53e677e12662ec9a
> 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.
Posted by Bill Farner <wf...@apache.org>.
> On Aug. 9, 2014, 12:48 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java, line 40
> > <https://reviews.apache.org/r/24521/diff/2/?file=656929#file656929line40>
> >
> > 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 4ea9ec2b96fc12429f33336b53e677e12662ec9a
> 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.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/24521/#comment87647>
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 4ea9ec2b96fc12429f33336b53e677e12662ec9a
> 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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 4ea9ec2b96fc12429f33336b53e677e12662ec9a
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