Re: Review Request 17303: Added getJobSummary API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17303/ --- (Updated March 11, 2014, 6:22 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Changes --- Updated per code review comments. 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 (updated) - 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 60b2259f21b598fa38bec5a590516cba2c07e1ac src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 4911c77a16c486fabfead7ad2f84ee95423ecd93 src/main/thrift/org/apache/aurora/gen/api.thrift d72b28c3378a651a8cff49216c1435ce7aee5977 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 9712f30a71be206fbf417198d0af673b45e0281e src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java a5fcbd465b5e07e23b24524e060cea304f102492 src/test/resources/org/apache/aurora/gen/api.thrift.md5 2308ba8da96197d41040ba772ea871003615698a Diff: https://reviews.apache.org/r/17303/diff/ Testing --- gradle clean build gradle run to test local UI. Thanks, Suman Karumuri
Re: Review Request 17303: Added getJobSummary API
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/
Re: Review Request 17303: Added getJobSummary API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17303/ --- (Updated March 9, 2014, 10:17 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Changes --- Updated code per review comments. 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 (updated) - 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 60b2259f21b598fa38bec5a590516cba2c07e1ac src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 4911c77a16c486fabfead7ad2f84ee95423ecd93 src/main/thrift/org/apache/aurora/gen/api.thrift d72b28c3378a651a8cff49216c1435ce7aee5977 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 9712f30a71be206fbf417198d0af673b45e0281e src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java a5fcbd465b5e07e23b24524e060cea304f102492 src/test/resources/org/apache/aurora/gen/api.thrift.md5 2308ba8da96197d41040ba772ea871003615698a Diff: https://reviews.apache.org/r/17303/diff/ Testing --- gradle clean build gradle run to test local UI. Thanks, Suman Karumuri
Re: Review Request 17303: Added getJobSummary API
--- 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
Re: Review Request 17303: Added getJobSummary API
On Feb. 3, 2014, 10:40 p.m., Bill Farner wrote: src/test/java/org/apache/aurora/scheduler/base/TaskUtil.java, line 19 https://reviews.apache.org/r/17303/diff/1/?file=447781#file447781line19 Do you think this class scales to multiple consumers? i.e. there's a bunch of hard-coded fields that work okay with the existing 2 callers, but we would need to plumb a lot more through for more broad usage. Suman Karumuri wrote: I think so, since a lot of tests have a makeTask method in them and we can get rid of some redundant code that way. Emphasis of this question was about whether the class scales. Right now it works great because the two classes don't care about the fields you have hardcoded. However, this can quickly devolve into a bunch of factory methods, or methods with a ton of parameters. For now, i actually suggest accepting the duplication until we have an approach that prevents that situation. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17303/#review33522 --- 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
Re: Review Request 17303: Added getJobSummary API
---
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
Re: Review Request 17303: Added getJobSummary API
--- 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. Changes --- Fixed code review nits. 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 (updated) - 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
Re: Review Request 17303: Added getJobSummary API
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:
Re: Review Request 17303: Added getJobSummary API
--- 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
Re: Review Request 17303: Added getJobSummary API
--- 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. Changes --- Added getJobSummary API per code review. Addressed review feedback. Summary (updated) - Added getJobSummary API Bugs: AURORA-64 https://issues.apache.org/jira/browse/AURORA-64 Repository: aurora Description (updated) --- 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 (updated) - 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
