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/
