You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Suman Karumuri <ma...@apache.org> on 2014/01/24 08:13:09 UTC
Review Request 17303: Updated getJobs API to return task stats and latest
task config
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17303/
-----------------------------------------------------------
Review request for Aurora, Kevin Sweeney and Bill Farner.
Bugs: AURORA-64
https://issues.apache.org/jira/browse/AURORA-64
Repository: aurora
Description
-------
Added task stats to getJobs API so it can be used by the role 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 06a19d80483b6949c9851b5d38fe34ac712aa75e
src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java b0caca73b46fba928fb718ab45a608dad4685a2f
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java cf9099f307efa23ca34634e3512d9cdbebfa82f2
src/main/thrift/org/apache/aurora/gen/api.thrift 74010379baa2e47cefc228943f766c7b3a8b0d97
src/test/java/org/apache/aurora/scheduler/base/JobsTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/base/TaskUtil.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/http/SchedulerzRoleTest.java 912be189583419e7201e45650d18cd24a6a5a35b
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 6cefdfad469a9b69a5291ad46be1df14b443472e
src/test/resources/org/apache/aurora/gen/api.thrift.md5 42fdca2759f15d007bee058485c237268c57597a
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: Updated getJobs API to return task stats and
latest task config
Posted by Suman Karumuri <ma...@apache.org>.
> On Feb. 3, 2014, 10:40 p.m., Bill Farner wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 186
> > <https://reviews.apache.org/r/17303/diff/1/?file=447779#file447779line186>
> >
> > Please give a more informative doc. As it stands, it provides the same info as "struct JobStats".
Removed.
> On Feb. 3, 2014, 10:40 p.m., Bill Farner wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 191
> > <https://reviews.apache.org/r/17303/diff/1/?file=447779#file447779line191>
> >
> > As we discussed offline, please remove this.
Removed.
> On Feb. 3, 2014, 10:40 p.m., Bill Farner wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 214
> > <https://reviews.apache.org/r/17303/diff/1/?file=447779#file447779line214>
> >
> > Just to make sure it's not dropped — this should be removed.
Done.
> On Feb. 3, 2014, 10:40 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/base/JobsTest.java, line 1
> > <https://reviews.apache.org/r/17303/diff/1/?file=447780#file447780line1>
> >
> > missing license header
Added.
> On Feb. 3, 2014, 10:40 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Jobs.java, line 1
> > <https://reviews.apache.org/r/17303/diff/1/?file=447775#file447775line1>
> >
> > missing license header. please set this up in intellij so it's automatic.
Added.
> On Feb. 3, 2014, 10:40 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Jobs.java, line 21
> > <https://reviews.apache.org/r/17303/diff/1/?file=447775#file447775line21>
> >
> > Please doc.
Done.
> 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.
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.
> On Feb. 3, 2014, 10:40 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Tasks.java, line 48
> > <https://reviews.apache.org/r/17303/diff/1/?file=447776#file447776line48>
> >
> > ImmutableList.of(..)
Fixed.
> On Feb. 3, 2014, 10:40 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Tasks.java, line 188
> > <https://reviews.apache.org/r/17303/diff/1/?file=447776#file447776line188>
> >
> > Please fix premature wrap, and add @param, @return javadoc fields.
> >
> > Also, the term 'freshest' is ambiguous (and used interchangeably with 'latest' here and at a call site. How about getLatestTransitionedTask?
> >
> > Finally, please include a TODO in SchedulerThriftInterface to pull this method in there once the other caller is removed.
Done.
- Suman
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17303/#review33522
-----------------------------------------------------------
On Jan. 24, 2014, 7:13 a.m., Suman Karumuri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17303/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2014, 7:13 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 task stats to getJobs API so it can be used by the role 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 06a19d80483b6949c9851b5d38fe34ac712aa75e
> src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java b0caca73b46fba928fb718ab45a608dad4685a2f
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java cf9099f307efa23ca34634e3512d9cdbebfa82f2
> src/main/thrift/org/apache/aurora/gen/api.thrift 74010379baa2e47cefc228943f766c7b3a8b0d97
> src/test/java/org/apache/aurora/scheduler/base/JobsTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/base/TaskUtil.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/http/SchedulerzRoleTest.java 912be189583419e7201e45650d18cd24a6a5a35b
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 6cefdfad469a9b69a5291ad46be1df14b443472e
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 42fdca2759f15d007bee058485c237268c57597a
>
> 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
Posted by Bill Farner <wf...@apache.org>.
> 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: Updated getJobs API to return task stats and
latest task config
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17303/#review33522
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/base/Jobs.java
<https://reviews.apache.org/r/17303/#comment63012>
missing license header. please set this up in intellij so it's automatic.
src/main/java/org/apache/aurora/scheduler/base/Jobs.java
<https://reviews.apache.org/r/17303/#comment63017>
Please doc.
src/main/java/org/apache/aurora/scheduler/base/Tasks.java
<https://reviews.apache.org/r/17303/#comment63018>
ImmutableList.of(..)
src/main/java/org/apache/aurora/scheduler/base/Tasks.java
<https://reviews.apache.org/r/17303/#comment63019>
Please fix premature wrap, and add @param, @return javadoc fields.
Also, the term 'freshest' is ambiguous (and used interchangeably with 'latest' here and at a call site. How about getLatestTransitionedTask?
Finally, please include a TODO in SchedulerThriftInterface to pull this method in there once the other caller is removed.
src/main/thrift/org/apache/aurora/gen/api.thrift
<https://reviews.apache.org/r/17303/#comment63015>
Please give a more informative doc. As it stands, it provides the same info as "struct JobStats".
src/main/thrift/org/apache/aurora/gen/api.thrift
<https://reviews.apache.org/r/17303/#comment63014>
As we discussed offline, please remove this.
src/main/thrift/org/apache/aurora/gen/api.thrift
<https://reviews.apache.org/r/17303/#comment63016>
Just to make sure it's not dropped — this should be removed.
src/test/java/org/apache/aurora/scheduler/base/JobsTest.java
<https://reviews.apache.org/r/17303/#comment63013>
missing license header
src/test/java/org/apache/aurora/scheduler/base/TaskUtil.java
<https://reviews.apache.org/r/17303/#comment63010>
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.
- Bill Farner
On Jan. 24, 2014, 7:13 a.m., Suman Karumuri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17303/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2014, 7:13 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 task stats to getJobs API so it can be used by the role 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 06a19d80483b6949c9851b5d38fe34ac712aa75e
> src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java b0caca73b46fba928fb718ab45a608dad4685a2f
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java cf9099f307efa23ca34634e3512d9cdbebfa82f2
> src/main/thrift/org/apache/aurora/gen/api.thrift 74010379baa2e47cefc228943f766c7b3a8b0d97
> src/test/java/org/apache/aurora/scheduler/base/JobsTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/base/TaskUtil.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/http/SchedulerzRoleTest.java 912be189583419e7201e45650d18cd24a6a5a35b
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 6cefdfad469a9b69a5291ad46be1df14b443472e
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 42fdca2759f15d007bee058485c237268c57597a
>
> 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: Updated getJobs API to return task stats and
latest task config
Posted by Suman Karumuri <ma...@apache.org>.
> On Feb. 25, 2014, 11:11 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Jobs.java, line 48
> > <https://reviews.apache.org/r/17303/diff/1/?file=447775#file447775line48>
> >
> > This uses time so it should take a Clock instance for testing.
Since we got rid of mostRecentFailures logic, this code is dropped now.
> On Feb. 25, 2014, 11:11 p.m., Kevin Sweeney wrote:
> > src/test/java/org/apache/aurora/scheduler/base/TaskUtil.java, line 19
> > <https://reviews.apache.org/r/17303/diff/1/?file=447781#file447781line19>
> >
> > How would you feel about naming this TaskFixtures or TaskTestUtil?
Done.
> On Feb. 25, 2014, 11:11 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Tasks.java, line 192
> > <https://reviews.apache.org/r/17303/diff/1/?file=447776#file447776line192>
> >
> > explicit orderings will throw RuntimeExceptions if they encounter a value not in the explicit set. It doesn't seem to me that STATUSES is exhaustive and I don't see test coverage to guarantee that in the future.
Good catch. Updated the list to be exhaustive. Added a test case to test the same.
> On Feb. 25, 2014, 11:11 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Tasks.java, line 191
> > <https://reviews.apache.org/r/17303/diff/1/?file=447776#file447776line191>
> >
> > Why the indirection? Why not call this latestActiveTask?
Updated comment to make this clear. Can't use latestActiveTask since a job may have just finished but it's taskConfig is not a representative task config for the job.
- Suman
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17303/#review33465
-----------------------------------------------------------
On Jan. 24, 2014, 7:13 a.m., Suman Karumuri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17303/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2014, 7:13 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 task stats to getJobs API so it can be used by the role 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 06a19d80483b6949c9851b5d38fe34ac712aa75e
> src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java b0caca73b46fba928fb718ab45a608dad4685a2f
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java cf9099f307efa23ca34634e3512d9cdbebfa82f2
> src/main/thrift/org/apache/aurora/gen/api.thrift 74010379baa2e47cefc228943f766c7b3a8b0d97
> src/test/java/org/apache/aurora/scheduler/base/JobsTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/base/TaskUtil.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/http/SchedulerzRoleTest.java 912be189583419e7201e45650d18cd24a6a5a35b
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 6cefdfad469a9b69a5291ad46be1df14b443472e
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 42fdca2759f15d007bee058485c237268c57597a
>
> 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: Updated getJobs API to return task stats and
latest task config
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17303/#review33465
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/base/Jobs.java
<https://reviews.apache.org/r/17303/#comment62899>
This uses time so it should take a Clock instance for testing.
src/main/java/org/apache/aurora/scheduler/base/Tasks.java
<https://reviews.apache.org/r/17303/#comment62900>
Unclear from name what this variable is for. Is this supposed to be ScheduleStatus.values()?
src/main/java/org/apache/aurora/scheduler/base/Tasks.java
<https://reviews.apache.org/r/17303/#comment62902>
Why the indirection? Why not call this latestActiveTask?
src/main/java/org/apache/aurora/scheduler/base/Tasks.java
<https://reviews.apache.org/r/17303/#comment62904>
explicit orderings will throw RuntimeExceptions if they encounter a value not in the explicit set. It doesn't seem to me that STATUSES is exhaustive and I don't see test coverage to guarantee that in the future.
src/test/java/org/apache/aurora/scheduler/base/TaskUtil.java
<https://reviews.apache.org/r/17303/#comment62906>
How would you feel about naming this TaskFixtures or TaskTestUtil?
- Kevin Sweeney
On Jan. 23, 2014, 11:13 p.m., Suman Karumuri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17303/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2014, 11:13 p.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 task stats to getJobs API so it can be used by the role 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 06a19d80483b6949c9851b5d38fe34ac712aa75e
> src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java b0caca73b46fba928fb718ab45a608dad4685a2f
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java cf9099f307efa23ca34634e3512d9cdbebfa82f2
> src/main/thrift/org/apache/aurora/gen/api.thrift 74010379baa2e47cefc228943f766c7b3a8b0d97
> src/test/java/org/apache/aurora/scheduler/base/JobsTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/base/TaskUtil.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/http/SchedulerzRoleTest.java 912be189583419e7201e45650d18cd24a6a5a35b
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 6cefdfad469a9b69a5291ad46be1df14b443472e
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 42fdca2759f15d007bee058485c237268c57597a
>
> 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
Posted by Suman Karumuri <ma...@apache.org>.
> 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/
> -----------------------------------------------------------
>
> (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
>
>
Re: Review Request 17303: Added getJobSummary API
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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
> 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
Posted by Suman Karumuri <ma...@apache.org>.
-----------------------------------------------------------
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
Posted by Suman Karumuri <ma...@apache.org>.
> On March 11, 2014, 12:33 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Tasks.java, line 51
> > <https://reviews.apache.org/r/17303/diff/4/?file=514590#file514590line51>
> >
> > looks like this just barely fits on the previous line
Done.
> On March 11, 2014, 12:33 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Tasks.java, line 223
> > <https://reviews.apache.org/r/17303/diff/4/?file=514590#file514590line223>
> >
> > Ordering.max already throws NoSuchElementException, which is fine.
> >
> > However, you should generally prefer to use Preconditions.checkArgument if you were to perform the check here.
Preferring an explicit check to make intent clear. Changed to use Preconditions.checkArgument.
> On March 11, 2014, 12:33 a.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/base/TasksTest.java, line 37
> > <https://reviews.apache.org/r/17303/diff/4/?file=514596#file514596line37>
> >
> > OrderedTaskStatuses seems to refer to a thing, but i can't find the thing. Should this comment be rewritten?
Oh that was a typo. SchedulerStatus -> ScheduleStatus
> On March 11, 2014, 12:33 a.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/base/TasksTest.java, line 57
> > <https://reviews.apache.org/r/17303/diff/4/?file=514596#file514596line57>
> >
> > Always default to immutable.
> >
> > ImmutableList.<IScheduledTask>of()
> >
> >
Changed.
- Suman
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17303/#review36735
-----------------------------------------------------------
On March 9, 2014, 10:17 a.m., Suman Karumuri wrote:
>
> -----------------------------------------------------------
> 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.
>
>
> 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 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
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17303/#review36735
-----------------------------------------------------------
Ship it!
src/main/java/org/apache/aurora/scheduler/base/Tasks.java
<https://reviews.apache.org/r/17303/#comment67874>
looks like this just barely fits on the previous line
src/main/java/org/apache/aurora/scheduler/base/Tasks.java
<https://reviews.apache.org/r/17303/#comment67876>
Ordering.max already throws NoSuchElementException, which is fine.
However, you should generally prefer to use Preconditions.checkArgument if you were to perform the check here.
src/test/java/org/apache/aurora/scheduler/base/TasksTest.java
<https://reviews.apache.org/r/17303/#comment67880>
OrderedTaskStatuses seems to refer to a thing, but i can't find the thing. Should this comment be rewritten?
src/test/java/org/apache/aurora/scheduler/base/TasksTest.java
<https://reviews.apache.org/r/17303/#comment67882>
Always default to immutable.
ImmutableList.<IScheduledTask>of()
- Bill Farner
On March 9, 2014, 10:17 a.m., Suman Karumuri wrote:
>
> -----------------------------------------------------------
> 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.
>
>
> 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 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
Posted by Suman Karumuri <ma...@apache.org>.
-----------------------------------------------------------
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
Posted by Suman Karumuri <ma...@apache.org>.
-----------------------------------------------------------
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
Posted by Suman Karumuri <ma...@apache.org>.
> 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
>
>
Re: Review Request 17303: Added getJobSummary API
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 17303: Added getJobSummary API
Posted by Suman Karumuri <ma...@apache.org>.
-----------------------------------------------------------
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
Posted by Suman Karumuri <ma...@apache.org>.
-----------------------------------------------------------
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
Re: Review Request 17303: Updated getJobs API to return task stats and
latest task config
Posted by Suman Karumuri <ma...@apache.org>.
> On Jan. 24, 2014, 4:44 p.m., Bill Farner wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 187
> > <https://reviews.apache.org/r/17303/diff/1/?file=447779#file447779line187>
> >
> > I think pre-computing these stats restricts the freedom of an API consumer from displaying them differently. Also, ambiguous names like 'recently' should absolutely be avoided.
> >
> > Before we go down this road it would be good to collect real data showing that this aggregation is necessary on the server. I don't doubt it is, but the exercise will give us a baseline to work with. If it does prove necessary, it would be good to know what contributes to the problem (data transmission, memory, computation time, etc).
> >
> > My intuition is to err on the side of returning more information unless it causes problems; allowing the client to make up its own aggregations.
Sent out detailed stats in an email. If we send all the task configs(6000) instead of summarizing them, the response size for our largest role would be roughly 900kb to 1.2MB through a conservative estimate. Summarizing all these stats on the server, the response size for the current data is 50-100 bytes. On the server side that is not a lot of compute power, but with several concurrent requests, it may saturate the network. On the client side, that is a lot of data and given that JS is single threaded, it will certainly cause UI hiccups and browser crashes. In this particular case, I feel sending summary data is better, since the users have the ability to query the tasks per job using another API and since most of the data we send is discarded in the current use case.
I agree that recently is ambiguous. We can consider renaming it to tasksFailedInLast6Hours or dropping it entirely. I am in favor of the later since:
a) We define the status of a service rather arbitrarily. For example, if I have a failed task, that I fixed in a subsequent deployment, the jobs status is still warning for the next 6 hours which is not accurate.
b) I am unsure how useful Stable/UnStable and Warning badges are in practice since most services rely on a stats gathering framework to check the health of their service.
> On Jan. 24, 2014, 4:44 p.m., Bill Farner wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 214
> > <https://reviews.apache.org/r/17303/diff/1/?file=447779#file447779line214>
> >
> > This is overloading the purpose of JobConfiguration. It's currently "the configuration of a job", and i would prefer to not add use cases to that.
> >
> > It would be much tidier to compose this information _with_ JobConfiguration objects in another struct.
Agreed.
- Suman
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17303/#review32720
-----------------------------------------------------------
On Jan. 24, 2014, 7:13 a.m., Suman Karumuri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17303/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2014, 7:13 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 task stats to getJobs API so it can be used by the role 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 06a19d80483b6949c9851b5d38fe34ac712aa75e
> src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java b0caca73b46fba928fb718ab45a608dad4685a2f
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java cf9099f307efa23ca34634e3512d9cdbebfa82f2
> src/main/thrift/org/apache/aurora/gen/api.thrift 74010379baa2e47cefc228943f766c7b3a8b0d97
> src/test/java/org/apache/aurora/scheduler/base/JobsTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/base/TaskUtil.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/http/SchedulerzRoleTest.java 912be189583419e7201e45650d18cd24a6a5a35b
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 6cefdfad469a9b69a5291ad46be1df14b443472e
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 42fdca2759f15d007bee058485c237268c57597a
>
> 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: Updated getJobs API to return task stats and
latest task config
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17303/#review32720
-----------------------------------------------------------
src/main/thrift/org/apache/aurora/gen/api.thrift
<https://reviews.apache.org/r/17303/#comment61666>
I think pre-computing these stats restricts the freedom of an API consumer from displaying them differently. Also, ambiguous names like 'recently' should absolutely be avoided.
Before we go down this road it would be good to collect real data showing that this aggregation is necessary on the server. I don't doubt it is, but the exercise will give us a baseline to work with. If it does prove necessary, it would be good to know what contributes to the problem (data transmission, memory, computation time, etc).
My intuition is to err on the side of returning more information unless it causes problems; allowing the client to make up its own aggregations.
src/main/thrift/org/apache/aurora/gen/api.thrift
<https://reviews.apache.org/r/17303/#comment61664>
This is overloading the purpose of JobConfiguration. It's currently "the configuration of a job", and i would prefer to not add use cases to that.
It would be much tidier to compose this information _with_ JobConfiguration objects in another struct.
- Bill Farner
On Jan. 24, 2014, 7:13 a.m., Suman Karumuri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17303/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2014, 7:13 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 task stats to getJobs API so it can be used by the role 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 06a19d80483b6949c9851b5d38fe34ac712aa75e
> src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java b0caca73b46fba928fb718ab45a608dad4685a2f
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java cf9099f307efa23ca34634e3512d9cdbebfa82f2
> src/main/thrift/org/apache/aurora/gen/api.thrift 74010379baa2e47cefc228943f766c7b3a8b0d97
> src/test/java/org/apache/aurora/scheduler/base/JobsTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/base/TaskUtil.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/http/SchedulerzRoleTest.java 912be189583419e7201e45650d18cd24a6a5a35b
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 6cefdfad469a9b69a5291ad46be1df14b443472e
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 42fdca2759f15d007bee058485c237268c57597a
>
> Diff: https://reviews.apache.org/r/17303/diff/
>
>
> Testing
> -------
>
> gradle clean build
> gradle run to test local UI.
>
>
> Thanks,
>
> Suman Karumuri
>
>