aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Maxim Khutornenko" <ma...@apache.org>
Subject Re: Review Request 24521: Finalizing DB mapper implementation.
Date Tue, 12 Aug 2014 20:26:42 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message