Re: Review Request 17303: Added getJobSummary API

2014-03-10 Thread Suman Karumuri 

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

(Updated March 11, 2014, 6:22 a.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

Updated per code review comments.


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

  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 
60b2259f21b598fa38bec5a590516cba2c07e1ac 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
4911c77a16c486fabfead7ad2f84ee95423ecd93 
  src/main/thrift/org/apache/aurora/gen/api.thrift 
d72b28c3378a651a8cff49216c1435ce7aee5977 
  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
 9712f30a71be206fbf417198d0af673b45e0281e 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
a5fcbd465b5e07e23b24524e060cea304f102492 
  src/test/resources/org/apache/aurora/gen/api.thrift.md5 
2308ba8da96197d41040ba772ea871003615698a 

Diff: https://reviews.apache.org/r/17303/diff/


Testing
---

gradle clean build
gradle run to test local UI.


Thanks,

Suman Karumuri

Re: Review Request 17303: Added getJobSummary API

2014-03-10 Thread Suman Karumuri 


> On March 11, 2014, 12:33 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Tasks.java, line 51
> > 
> >
> > looks like this just barely fits on the previous line

Done.


> On March 11, 2014, 12:33 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Tasks.java, line 223
> > 
> >
> > Ordering.max already throws NoSuchElementException, which is fine.
> > 
> > However, you should generally prefer to use Preconditions.checkArgument 
> > if you were to perform the check here.

Preferring an explicit check to make intent clear. Changed to use 
Preconditions.checkArgument.


> On March 11, 2014, 12:33 a.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/base/TasksTest.java, line 37
> > 
> >
> > OrderedTaskStatuses seems to refer to a thing, but i can't find the 
> > thing.  Should this comment be rewritten?

Oh that was a typo. SchedulerStatus -> ScheduleStatus


> On March 11, 2014, 12:33 a.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/base/TasksTest.java, line 57
> > 
> >
> > Always default to immutable.
> > 
> >   ImmutableList.of()
> > 
> >

Changed.


- Suman


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


On March 9, 2014, 10:17 a.m., Suman Karumuri wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17303/
> ---
> 
> (Updated March 9, 2014, 10:17 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 
> 60b2259f21b598fa38bec5a590516cba2c07e1ac 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  4911c77a16c486fabfead7ad2f84ee95423ecd93 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> d72b28c3378a651a8cff49216c1435ce7aee5977 
>   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
>  9712f30a71be206fbf417198d0af673b45e0281e 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> a5fcbd465b5e07e23b24524e060cea304f102492 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 2308ba8da96197d41040ba772ea871003615698a 
> 
> Diff: https://reviews.apache.org/r/17303/diff/
> 
> 
> Testing
> ---
> 
> gradle clean build
> gradle run to test local UI.
> 
> 
> Thanks,
> 
> Suman Karumuri
> 
>

Re: Review Request 17303: Added getJobSummary API

2014-03-10 Thread Bill Farner 

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

Ship it!



src/main/java/org/apache/aurora/scheduler/base/Tasks.java


looks like this just barely fits on the previous line



src/main/java/org/apache/aurora/scheduler/base/Tasks.java


Ordering.max already throws NoSuchElementException, which is fine.

However, you should generally prefer to use Preconditions.checkArgument if 
you were to perform the check here.



src/test/java/org/apache/aurora/scheduler/base/TasksTest.java


OrderedTaskStatuses seems to refer to a thing, but i can't find the thing.  
Should this comment be rewritten?



src/test/java/org/apache/aurora/scheduler/base/TasksTest.java


Always default to immutable.

  ImmutableList.of()




- Bill Farner


On March 9, 2014, 10:17 a.m., Suman Karumuri wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17303/
> ---
> 
> (Updated March 9, 2014, 10:17 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 
> 60b2259f21b598fa38bec5a590516cba2c07e1ac 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  4911c77a16c486fabfead7ad2f84ee95423ecd93 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> d72b28c3378a651a8cff49216c1435ce7aee5977 
>   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
>  9712f30a71be206fbf417198d0af673b45e0281e 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> a5fcbd465b5e07e23b24524e060cea304f102492 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 2308ba8da96197d41040ba772ea871003615698a 
> 
> Diff: https://reviews.apache.org/r/17303/diff/
> 
> 
> Testing
> ---
> 
> gradle clean build
> gradle run to test local UI.
> 
> 
> Thanks,
> 
> Suman Karumuri
> 
>

Re: Review Request 17303: Added getJobSummary API

2014-03-09 Thread Suman Karumuri 

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

(Updated March 9, 2014, 10:17 a.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

Updated code per review comments.


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

  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 
60b2259f21b598fa38bec5a590516cba2c07e1ac 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
4911c77a16c486fabfead7ad2f84ee95423ecd93 
  src/main/thrift/org/apache/aurora/gen/api.thrift 
d72b28c3378a651a8cff49216c1435ce7aee5977 
  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
 9712f30a71be206fbf417198d0af673b45e0281e 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
a5fcbd465b5e07e23b24524e060cea304f102492 
  src/test/resources/org/apache/aurora/gen/api.thrift.md5 
2308ba8da96197d41040ba772ea871003615698a 

Diff: https://reviews.apache.org/r/17303/diff/


Testing
---

gradle clean build
gradle run to test local UI.


Thanks,

Suman Karumuri

Re: Review Request 17303: Added getJobSummary API

2014-03-09 Thread Suman Karumuri 


> On March 3, 2014, 8:51 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Jobs.java, line 38
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > "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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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:
> 
> --

Re: Review Request 17303: Added getJobSummary API

2014-03-03 Thread Bill Farner 

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


Looking pretty good now, found a handful of places where comments from previous 
rounds weren't fixed throughout.


src/main/java/org/apache/aurora/scheduler/base/Jobs.java


s/Collection/Iterable/



src/main/java/org/apache/aurora/scheduler/base/Tasks.java


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.



src/main/java/org/apache/aurora/scheduler/base/Tasks.java


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



src/main/java/org/apache/aurora/scheduler/base/Tasks.java


How about getLatestActiveTask()?  getLatestTransitionedTask() seems like it 
should be skipping the LATEST_ACTIVITY order.



src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java


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.



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


still planning to move this closer to JobSummary?



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


"optionally only those owned by a specific role"



src/test/java/org/apache/aurora/scheduler/base/JobsTest.java


2014



src/test/java/org/apache/aurora/scheduler/base/TaskTestUtil.java


2014



src/test/java/org/apache/aurora/scheduler/base/TasksTest.java


2014



src/test/java/org/apache/aurora/scheduler/base/TasksTest.java


s/final //

throughout



src/test/java/org/apache/aurora/scheduler/base/TasksTest.java


What's the expected behavior when an empty iterable is provided?



src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java


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.


- Bill Farner


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

Re: Review Request 17303: Added getJobSummary API

2014-02-28 Thread Suman Karumuri 


> On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Jobs.java, line 2
> > 
> >
> > 2014

Done. I wish the license plugin checked for things like these.


> On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Jobs.java, line 24
> > 
> >
> > "A class that"

Fixed. getting there...


> On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Jobs.java, line 34
> > 
> >
> > empty line between javadoc body and tags

Fixed.


> On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Tasks.java, line 49
> > 
> >
> > Can you elaborate on why this is necessary?  As it stands, i can't tell 
> > why 'rough ordering' is useful.

I used the term roughly because, one can argue about the ordering of the 
classes. For example, one can argue that THROTTLED is an inactive state. To 
remove the confusion dropped the term roughly.


> On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Tasks.java, line 51
> > 
> >
> > s/public //

Fixed.


> On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Tasks.java, line 53
> > 
> >
> > Please break these out as arg per line, your comment will still be just 
> > as readable.

Done.


> On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Tasks.java, line 161
> > 
> >
> > Put this in the javadoc comment

Done.


> On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Tasks.java, line 222
> > 
> >
> > empty line above

Fixed. Was following the other comments in the file.


> On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Tasks.java, line 227
> > 
> >
> > Please pull this to the previous line

Done.


> On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Tasks.java, line 228
> > 
> >
> > please put the chained calls on individual lines for readability

Done.


> On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 186
> > 
> >
> > Any particular reason this is declared so far away from JobSummary?

JobStats was added here since it was part of JobConfiguration initially. Didn't 
move it after the API was added. Moved it now.


> On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 187
> > 
> >
> > Number of spaces between fields and comments seems arbitrary.  Other 
> > places, the convention os +2 past the right-most column in the block.  Can 
> > you follow that for consistency?

Fixed.


> On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 456
> > 
> >
> > s/the //

Done.


> On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/base/TaskTestUtil.java, line 32
> > 
> >
> > s/A utility class containing c/C/

Done.


> On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/base/TasksTest.java, line 42
> > 
> >
> > I'm surprised checkstyle didn't complain about this, please follow the 
> > JLS naming conventions [1] (don't start with uppercase).
> > 
> > [1] http://docs.oracle.com/javase/specs/jls/se7/html/jls-6.html

Fixed.


> On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/base/TasksTest.java, line 49
> > 
> >
> > This gets much more concise with a helper function:
> > 
> > private static void asertLatestTask(ISchedul

Re: Review Request 17303: Added getJobSummary API

2014-02-28 Thread Suman Karumuri 

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


Changes
---

Fixed code review nits.


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

  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

Re: Review Request 17303: Added getJobSummary API

2014-02-28 Thread Bill Farner 

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



src/main/java/org/apache/aurora/scheduler/base/Jobs.java


2014



src/main/java/org/apache/aurora/scheduler/base/Jobs.java


"A class that"



src/main/java/org/apache/aurora/scheduler/base/Jobs.java


empty line between javadoc body and tags



src/main/java/org/apache/aurora/scheduler/base/Tasks.java


Can you elaborate on why this is necessary?  As it stands, i can't tell why 
'rough ordering' is useful.



src/main/java/org/apache/aurora/scheduler/base/Tasks.java


s/public //



src/main/java/org/apache/aurora/scheduler/base/Tasks.java


Please break these out as arg per line, your comment will still be just as 
readable.



src/main/java/org/apache/aurora/scheduler/base/Tasks.java


Put this in the javadoc comment



src/main/java/org/apache/aurora/scheduler/base/Tasks.java


empty line above



src/main/java/org/apache/aurora/scheduler/base/Tasks.java


Please pull this to the previous line



src/main/java/org/apache/aurora/scheduler/base/Tasks.java


please put the chained calls on individual lines for readability



src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java


Why was this abandoned to form the list Tasks.ORDERED_TASK_STATUSES?



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


Any particular reason this is declared so far away from JobSummary?



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


Number of spaces between fields and comments seems arbitrary.  Other 
places, the convention os +2 past the right-most column in the block.  Can you 
follow that for consistency?



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


s/the //



src/test/java/org/apache/aurora/scheduler/base/TaskTestUtil.java


s/A utility class containing c/C/



src/test/java/org/apache/aurora/scheduler/base/TasksTest.java


I'm surprised checkstyle didn't complain about this, please follow the JLS 
naming conventions [1] (don't start with uppercase).

[1] http://docs.oracle.com/javase/specs/jls/se7/html/jls-6.html



src/test/java/org/apache/aurora/scheduler/base/TasksTest.java


This gets much more concise with a helper function:

private static void asertLatestTask(IScheduledTask expectedLatest, 
IScheduledTask... tasks) {
  ...
}



src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java


remove extra newline



src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java


These variable names suggest you're testing different things.  Perhaps this 
should be split into different cases, with less wordy variable names?


- Bill Farner


On Feb. 26, 2014, 4:18 a.m., Suman Karumuri wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17303/
> ---
> 
> (Updated Feb. 26, 2014, 4:18 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

Re: Review Request 17303: Added getJobSummary API

2014-02-28 Thread Bill Farner 


> On Feb. 3, 2014, 10:40 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/base/TaskUtil.java, line 19
> > 
> >
> > Do you think this class scales to multiple consumers?  i.e. there's a 
> > bunch of hard-coded fields that work okay with the existing 2 callers, but 
> > we would need to plumb a lot more through for more broad usage.
> 
> Suman Karumuri wrote:
> I think so, since a lot of tests have a makeTask method in them and we 
> can get rid of some redundant code that way.

Emphasis of this question was about whether the class scales.  Right now it 
works great because the two classes don't care about the fields you have 
hardcoded.  However, this can quickly devolve into a bunch of factory methods, 
or methods with a ton of parameters.  For now, i actually suggest accepting the 
duplication until we have an approach that prevents that situation.


- Bill


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


On Feb. 26, 2014, 4:18 a.m., Suman Karumuri wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17303/
> ---
> 
> (Updated Feb. 26, 2014, 4:18 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
>  dccae15a2c529025c9bb6a6f7a0220779ca4f9a1 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> cd60f47bf34b4a634004e2ad9eadad37aa1556bb 
>   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
>  4a2d39d8b25c4a6b161c47d6ba7068d74f8a60e0 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> 1edc0d7b224cc477ea6e8873e76ee8c70c6b4d50 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> fafb5100443482e662db453429c5259f2ab80ae5 
> 
> Diff: https://reviews.apache.org/r/17303/diff/
> 
> 
> Testing
> ---
> 
> gradle clean build
> gradle run to test local UI.
> 
> 
> Thanks,
> 
> Suman Karumuri
> 
>

Re: Review Request 17303: Added getJobSummary API

2014-02-25 Thread Suman Karumuri 

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

(Updated Feb. 26, 2014, 4:18 a.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

Added getJobSummary API per code review.
Addressed review feedback.


Summary (updated)
-

Added getJobSummary API


Bugs: AURORA-64
https://issues.apache.org/jira/browse/AURORA-64


Repository: aurora


Description (updated)
---

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

  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 
dccae15a2c529025c9bb6a6f7a0220779ca4f9a1 
  src/main/thrift/org/apache/aurora/gen/api.thrift 
cd60f47bf34b4a634004e2ad9eadad37aa1556bb 
  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
 4a2d39d8b25c4a6b161c47d6ba7068d74f8a60e0 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
1edc0d7b224cc477ea6e8873e76ee8c70c6b4d50 
  src/test/resources/org/apache/aurora/gen/api.thrift.md5 
fafb5100443482e662db453429c5259f2ab80ae5 

Diff: https://reviews.apache.org/r/17303/diff/


Testing
---

gradle clean build
gradle run to test local UI.


Thanks,

Suman Karumuri

Re: Review Request 17303: Added getJobSummary API

2014-02-25 Thread Suman Karumuri 

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

(Updated Feb. 26, 2014, 4:18 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 
dccae15a2c529025c9bb6a6f7a0220779ca4f9a1 
  src/main/thrift/org/apache/aurora/gen/api.thrift 
cd60f47bf34b4a634004e2ad9eadad37aa1556bb 
  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
 4a2d39d8b25c4a6b161c47d6ba7068d74f8a60e0 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
1edc0d7b224cc477ea6e8873e76ee8c70c6b4d50 
  src/test/resources/org/apache/aurora/gen/api.thrift.md5 
fafb5100443482e662db453429c5259f2ab80ae5 

Diff: https://reviews.apache.org/r/17303/diff/


Testing
---

gradle clean build
gradle run to test local UI.


Thanks,

Suman Karumuri