----------------------------------------------------------- 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 <https://reviews.apache.org/r/17303/#comment66584> 2014 src/main/java/org/apache/aurora/scheduler/base/Jobs.java <https://reviews.apache.org/r/17303/#comment66585> "A class that" src/main/java/org/apache/aurora/scheduler/base/Jobs.java <https://reviews.apache.org/r/17303/#comment66586> empty line between javadoc body and tags src/main/java/org/apache/aurora/scheduler/base/Tasks.java <https://reviews.apache.org/r/17303/#comment66587> 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 <https://reviews.apache.org/r/17303/#comment66592> s/public // src/main/java/org/apache/aurora/scheduler/base/Tasks.java <https://reviews.apache.org/r/17303/#comment66588> 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 <https://reviews.apache.org/r/17303/#comment66591> Put this in the javadoc comment src/main/java/org/apache/aurora/scheduler/base/Tasks.java <https://reviews.apache.org/r/17303/#comment66590> empty line above src/main/java/org/apache/aurora/scheduler/base/Tasks.java <https://reviews.apache.org/r/17303/#comment66589> Please pull this to the previous line src/main/java/org/apache/aurora/scheduler/base/Tasks.java <https://reviews.apache.org/r/17303/#comment66594> please put the chained calls on individual lines for readability src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java <https://reviews.apache.org/r/17303/#comment66595> Why was this abandoned to form the list Tasks.ORDERED_TASK_STATUSES? src/main/thrift/org/apache/aurora/gen/api.thrift <https://reviews.apache.org/r/17303/#comment66601> Any particular reason this is declared so far away from JobSummary? src/main/thrift/org/apache/aurora/gen/api.thrift <https://reviews.apache.org/r/17303/#comment66602> 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 <https://reviews.apache.org/r/17303/#comment66603> s/the // src/test/java/org/apache/aurora/scheduler/base/TaskTestUtil.java <https://reviews.apache.org/r/17303/#comment66604> s/A utility class containing c/C/ src/test/java/org/apache/aurora/scheduler/base/TasksTest.java <https://reviews.apache.org/r/17303/#comment66598> 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 <https://reviews.apache.org/r/17303/#comment66597> 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 <https://reviews.apache.org/r/17303/#comment66599> remove extra newline src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java <https://reviews.apache.org/r/17303/#comment66600> 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/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 > >
