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