----------------------------------------------------------- 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 <https://reviews.apache.org/r/17303/#comment66813> s/Collection/Iterable/ src/main/java/org/apache/aurora/scheduler/base/Tasks.java <https://reviews.apache.org/r/17303/#comment66817> 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 <https://reviews.apache.org/r/17303/#comment66814> 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 <https://reviews.apache.org/r/17303/#comment66822> How about getLatestActiveTask()? getLatestTransitionedTask() seems like it should be skipping the LATEST_ACTIVITY order. src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java <https://reviews.apache.org/r/17303/#comment66824> 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 <https://reviews.apache.org/r/17303/#comment66825> still planning to move this closer to JobSummary? src/main/thrift/org/apache/aurora/gen/api.thrift <https://reviews.apache.org/r/17303/#comment66826> "optionally only those owned by a specific role" src/test/java/org/apache/aurora/scheduler/base/JobsTest.java <https://reviews.apache.org/r/17303/#comment66827> 2014 src/test/java/org/apache/aurora/scheduler/base/TaskTestUtil.java <https://reviews.apache.org/r/17303/#comment66828> 2014 src/test/java/org/apache/aurora/scheduler/base/TasksTest.java <https://reviews.apache.org/r/17303/#comment66830> 2014 src/test/java/org/apache/aurora/scheduler/base/TasksTest.java <https://reviews.apache.org/r/17303/#comment66831> s/final // throughout src/test/java/org/apache/aurora/scheduler/base/TasksTest.java <https://reviews.apache.org/r/17303/#comment66833> What's the expected behavior when an empty iterable is provided? src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java <https://reviews.apache.org/r/17303/#comment66834> 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/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 > >
