> On March 3, 2014, 8:51 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Jobs.java, line 38
> > <https://reviews.apache.org/r/17303/diff/3/?file=507562#file507562line38>
> >
> >     s/Collection/Iterable/

Done.


> On March 3, 2014, 8:51 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Tasks.java, line 49
> > <https://reviews.apache.org/r/17303/diff/3/?file=507563#file507563line49>
> >
> >     I actually find this more confusing than "roughly".  You're better off 
> > saying "inactive tasks are ordered before active tasks", otherwise i start 
> > wondering why FAILED is less active than FINISHED.

Changed the List to use ACTIVE_STATES and TERMINAL_STATES per next comment. 
Dropping this comment since it's redundant.


> On March 3, 2014, 8:51 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Tasks.java, line 52
> > <https://reviews.apache.org/r/17303/diff/3/?file=507563#file507563line52>
> >
> >     I still think it's best to directly reference ACTIVE_STATES and 
> > TERMINAL_STATES here.  You noted that those are not exhaustive of all 
> > states, but the unrepresented states are INIT and UNKNOWN.  If consuming 
> > code reads tasks in those states, it's a  bug (they're currently only there 
> > assist the state machine).

Yeah this is better. 


> On March 3, 2014, 8:51 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Tasks.java, line 237
> > <https://reviews.apache.org/r/17303/diff/3/?file=507563#file507563line237>
> >
> >     How about getLatestActiveTask()?  getLatestTransitionedTask() seems 
> > like it should be skipping the LATEST_ACTIVITY order.

Changed it.


> On March 3, 2014, 8:51 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
> >  line 469
> > <https://reviews.apache.org/r/17303/diff/3/?file=507565#file507565line469>
> >
> >     Barring naming conflict, can you remove the "ByRole" qualifier from 
> > these method names?  It reads unnaturally to "get by role, maybe without a 
> > role", while "get, with optional role" is more obvious from the call site.

Done.


> On March 3, 2014, 8:51 p.m., Bill Farner wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 184
> > <https://reviews.apache.org/r/17303/diff/3/?file=507566#file507566line184>
> >
> >     still planning to move this closer to JobSummary?

Moved.


> On March 3, 2014, 8:51 p.m., Bill Farner wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 454
> > <https://reviews.apache.org/r/17303/diff/3/?file=507566#file507566line454>
> >
> >     "optionally only those owned by a specific role"

Done.


> On March 3, 2014, 8:51 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/base/JobsTest.java, line 2
> > <https://reviews.apache.org/r/17303/diff/3/?file=507567#file507567line2>
> >
> >     2014

oops missed those. Done.


> On March 3, 2014, 8:51 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/base/TaskTestUtil.java, line 2
> > <https://reviews.apache.org/r/17303/diff/3/?file=507568#file507568line2>
> >
> >     2014

Done.


> On March 3, 2014, 8:51 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/base/TasksTest.java, line 2
> > <https://reviews.apache.org/r/17303/diff/3/?file=507569#file507569line2>
> >
> >     2014

Done.


> On March 3, 2014, 8:51 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/base/TasksTest.java, line 42
> > <https://reviews.apache.org/r/17303/diff/3/?file=507569#file507569line42>
> >
> >     s/final //
> >     
> >     throughout

Changed.


> On March 3, 2014, 8:51 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/base/TasksTest.java, line 48
> > <https://reviews.apache.org/r/17303/diff/3/?file=507569#file507569line48>
> >
> >     What's the expected behavior when an empty iterable is provided?

We throw an NoSuchElement Exception. Formalized the case by throwing an 
IllegalArgumentException now and updated the test case accordingly.


> On March 3, 2014, 8:51 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java,
> >  line 1004
> > <https://reviews.apache.org/r/17303/diff/3/?file=507571#file507571line1004>
> >
> >     It would be really nice to see all of these assertEquals looking more 
> > like:
> >     
> >       assertEquals(expected, actual);
> >     
> >     Where 'actual' is the top-level response rather than the result of 
> > peeking into fields.  This gives confidence that the response code and 
> > message are also set appropriately.

Agreed. Done. 


- Suman


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


On March 1, 2014, 1:26 a.m., Suman Karumuri wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17303/
> -----------------------------------------------------------
> 
> (Updated March 1, 2014, 1:26 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-64
>     https://issues.apache.org/jira/browse/AURORA-64
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Added getJobSummary API so it can be used by the role and role/environment 
> page in the UI.
> Refactored code from SchedulerzRole and SchedulerzRoleTest into relevant 
> classes so it can be used by the UI and the thrift API.
> Added tests for new code.
> Moved populateJobConfig call into ReadOnlyScheduler.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/base/Jobs.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/base/Tasks.java 
> d9cb886ef333e108d5d5f86043ac80e450689894 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java 
> 25ba7da5f8bbe5416f41bb0b14850beb84392cc7 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  7b9f185cea77825e46ecfc588c72e146cd864a32 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> 3ee24c75f961af61048a78ec6c3f244361bed5bd 
>   src/test/java/org/apache/aurora/scheduler/base/JobsTest.java PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/base/TasksTest.java PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/SchedulerzRoleTest.java 
> 912be189583419e7201e45650d18cd24a6a5a35b 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  dc557718269064a202c3e4eb1272ff2b9f209ad9 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> a5fcbd465b5e07e23b24524e060cea304f102492 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 4e6c51d9298bf6fc1935ec9080f38726f79e7959 
> 
> Diff: https://reviews.apache.org/r/17303/diff/
> 
> 
> Testing
> -------
> 
> gradle clean build
> gradle run to test local UI.
> 
> 
> Thanks,
> 
> Suman Karumuri
> 
>

Reply via email to