> On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/base/Jobs.java, line 2 > > <https://reviews.apache.org/r/17303/diff/2/?file=504348#file504348line2> > > > > 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 > > <https://reviews.apache.org/r/17303/diff/2/?file=504348#file504348line24> > > > > "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 > > <https://reviews.apache.org/r/17303/diff/2/?file=504348#file504348line34> > > > > 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 > > <https://reviews.apache.org/r/17303/diff/2/?file=504349#file504349line49> > > > > 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 > > <https://reviews.apache.org/r/17303/diff/2/?file=504349#file504349line51> > > > > 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 > > <https://reviews.apache.org/r/17303/diff/2/?file=504349#file504349line53> > > > > 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 > > <https://reviews.apache.org/r/17303/diff/2/?file=504349#file504349line161> > > > > 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 > > <https://reviews.apache.org/r/17303/diff/2/?file=504349#file504349line222> > > > > 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 > > <https://reviews.apache.org/r/17303/diff/2/?file=504349#file504349line227> > > > > 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 > > <https://reviews.apache.org/r/17303/diff/2/?file=504349#file504349line228> > > > > 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 > > <https://reviews.apache.org/r/17303/diff/2/?file=504352#file504352line186> > > > > 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 > > <https://reviews.apache.org/r/17303/diff/2/?file=504352#file504352line187> > > > > 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 > > <https://reviews.apache.org/r/17303/diff/2/?file=504352#file504352line456> > > > > 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 > > <https://reviews.apache.org/r/17303/diff/2/?file=504354#file504354line32> > > > > 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 > > <https://reviews.apache.org/r/17303/diff/2/?file=504355#file504355line42> > > > > 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 > > <https://reviews.apache.org/r/17303/diff/2/?file=504355#file504355line49> > > > > This gets much more concise with a helper function: > > > > private static void asertLatestTask(IScheduledTask expectedLatest, > > IScheduledTask... tasks) { > > ... > > } Good one. Changed. > On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote: > > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java, > > line 1006 > > <https://reviews.apache.org/r/17303/diff/2/?file=504357#file504357line1006> > > > > remove extra newline Done. > On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote: > > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java, > > line 1016 > > <https://reviews.apache.org/r/17303/diff/2/?file=504357#file504357line1016> > > > > These variable names suggest you're testing different things. Perhaps > > this should be split into different cases, with less wordy variable names? We are testing slightly different things in this case. Thought of breaking it into a separate test case, but didn't like to break the convention of one test case per thrift call that is set in the file just for one test case. So, leaving it as is. > On Feb. 28, 2014, 8:21 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java, line 81 > > <https://reviews.apache.org/r/17303/diff/2/?file=504350#file504350line81> > > > > Why was this abandoned to form the list Tasks.ORDERED_TASK_STATUSES? This field was used to get the freshest tasks. Since that logic refactored into Tasks, this field moved as well. Also, this list wasn't exhaustive, so it was updated in Tasks.ORDERED_TASK_STATUS. - Suman ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17303/#review35831 ----------------------------------------------------------- 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 > >
