aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Suman Karumuri" <ma...@apache.org>
Subject Re: Review Request 17303: Added getJobSummary API
Date Sat, 01 Mar 2014 01:26:55 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message