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/28 00:45:57 UTC
Review Request 17430: Bug fix: Return job count instead of task count on
GetRoleSummary() call.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17430/
-----------------------------------------------------------
Review request for Aurora, Kevin Sweeney and Bill Farner.
Repository: aurora
Description
-------
Updated test cases to catch this bug.
Diffs
-----
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java cf9099f307efa23ca34634e3512d9cdbebfa82f2
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 6cefdfad469a9b69a5291ad46be1df14b443472e
Diff: https://reviews.apache.org/r/17430/diff/
Testing
-------
gradle clean build
gradle run - tested in local UI.
Thanks,
Suman Karumuri
Re: Review Request 17430: Bug fix: Return job count instead of task count on
GetRoleSummary() call.
Posted by Suman Karumuri <ma...@apache.org>.
> On Jan. 28, 2014, 3:32 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 384
> > <https://reviews.apache.org/r/17430/diff/1/?file=451962#file451962line384>
> >
> > Thinking out loud, a helper function reduces some duplication, and wrapping in a HashMultimap puts the values in a Set for you (so you can skip the ImmutableSet.copyOf later):
> >
> > private static <T> Multimap<String, IJobKey> mapByRole(
> > Iterable<T> entities,
> > Function<T, IJobKey> keyExtractor) {
> >
> > return HashMultimap.create(
> > Multimaps.index(Iterables.transform(tasks, keyExtractor)));
> > }
> >
> > Then, your calling code is:
> >
> > Multimap<String, IJobKey> jobsByRole = mapByRole(
> > Storage.weaklyConsistentFetchTasks(storage, Query.unscoped()),
> > Tasks.SCHEDULED_TO_JOB_KEY);
> > Multimap<String, IJobKey> cronJobsByRole =
> > mapByRole(cronJobManager.getJobs(), JobKeys.FROM_CONFIG);
> >
> > (note: you'll want to tweak line wrapping, trying to make it pretty in the comment box here)
Done.
Changed
Multimaps.index(Iterables.transform(tasks, keyExtractor)));
to
Multimaps.index(Iterables.transform(tasks, keyExtractor), JobKeys.TO_ROLE));
to match the signature.
> On Jan. 28, 2014, 3:32 a.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java, line 1112
> > <https://reviews.apache.org/r/17430/diff/1/?file=451963#file451963line1112>
> >
> > Does this test reproduce the bug? I would have expected to see something like two tasks in the same job resulting in a job count of 1.
Good catch. I added the task, but it got lost among other fixes. Fixed it now.
- Suman
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17430/#review32945
-----------------------------------------------------------
On Jan. 28, 2014, 8:51 p.m., Suman Karumuri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17430/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2014, 8:51 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Bugs: AURORA-116
> https://issues.apache.org/jira/browse/AURORA-116
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Updated test cases to catch this bug.
>
>
> Diffs
> -----
>
> 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/thrift/SchedulerThriftInterfaceTest.java 6cefdfad469a9b69a5291ad46be1df14b443472e
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 42fdca2759f15d007bee058485c237268c57597a
>
> Diff: https://reviews.apache.org/r/17430/diff/
>
>
> Testing
> -------
>
> gradle clean build
> gradle run - tested in local UI.
>
>
> Thanks,
>
> Suman Karumuri
>
>
Re: Review Request 17430: Bug fix: Return job count instead of task count on
GetRoleSummary() call.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17430/#review32945
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
<https://reviews.apache.org/r/17430/#comment61989>
Thinking out loud, a helper function reduces some duplication, and wrapping in a HashMultimap puts the values in a Set for you (so you can skip the ImmutableSet.copyOf later):
private static <T> Multimap<String, IJobKey> mapByRole(
Iterable<T> entities,
Function<T, IJobKey> keyExtractor) {
return HashMultimap.create(
Multimaps.index(Iterables.transform(tasks, keyExtractor)));
}
Then, your calling code is:
Multimap<String, IJobKey> jobsByRole = mapByRole(
Storage.weaklyConsistentFetchTasks(storage, Query.unscoped()),
Tasks.SCHEDULED_TO_JOB_KEY);
Multimap<String, IJobKey> cronJobsByRole =
mapByRole(cronJobManager.getJobs(), JobKeys.FROM_CONFIG);
(note: you'll want to tweak line wrapping, trying to make it pretty in the comment box here)
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
<https://reviews.apache.org/r/17430/#comment61988>
Does this test reproduce the bug? I would have expected to see something like two tasks in the same job resulting in a job count of 1.
- Bill Farner
On Jan. 27, 2014, 11:45 p.m., Suman Karumuri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17430/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2014, 11:45 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Updated test cases to catch this bug.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java cf9099f307efa23ca34634e3512d9cdbebfa82f2
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 6cefdfad469a9b69a5291ad46be1df14b443472e
>
> Diff: https://reviews.apache.org/r/17430/diff/
>
>
> Testing
> -------
>
> gradle clean build
> gradle run - tested in local UI.
>
>
> Thanks,
>
> Suman Karumuri
>
>
Re: Review Request 17430: Bug fix: Return job count instead of task count on
GetRoleSummary() call.
Posted by Suman Karumuri <ma...@apache.org>.
> On Jan. 28, 2014, 3:37 a.m., Bill Farner wrote:
> > Can you also attach the JIRA issue please?
Done.
- Suman
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17430/#review32947
-----------------------------------------------------------
On Jan. 28, 2014, 8:51 p.m., Suman Karumuri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17430/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2014, 8:51 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Bugs: AURORA-116
> https://issues.apache.org/jira/browse/AURORA-116
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Updated test cases to catch this bug.
>
>
> Diffs
> -----
>
> 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/thrift/SchedulerThriftInterfaceTest.java 6cefdfad469a9b69a5291ad46be1df14b443472e
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 42fdca2759f15d007bee058485c237268c57597a
>
> Diff: https://reviews.apache.org/r/17430/diff/
>
>
> Testing
> -------
>
> gradle clean build
> gradle run - tested in local UI.
>
>
> Thanks,
>
> Suman Karumuri
>
>
Re: Review Request 17430: Bug fix: Return job count instead of task count on
GetRoleSummary() call.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17430/#review32947
-----------------------------------------------------------
Can you also attach the JIRA issue please?
- Bill Farner
On Jan. 27, 2014, 11:45 p.m., Suman Karumuri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17430/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2014, 11:45 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Updated test cases to catch this bug.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java cf9099f307efa23ca34634e3512d9cdbebfa82f2
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 6cefdfad469a9b69a5291ad46be1df14b443472e
>
> Diff: https://reviews.apache.org/r/17430/diff/
>
>
> Testing
> -------
>
> gradle clean build
> gradle run - tested in local UI.
>
>
> Thanks,
>
> Suman Karumuri
>
>
Re: Review Request 17430: Bug fix: Return job count instead of task count on
GetRoleSummary() call.
Posted by Suman Karumuri <ma...@apache.org>.
> On Jan. 28, 2014, 9:02 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java, line 1087
> > <https://reviews.apache.org/r/17430/diff/2/?file=453063#file453063line1087>
> >
> > This task needs to be different from task1 in some way, otherwise this test could deceivingly pass if the two tasks are ever placed into a Set.
Fixed.
> On Jan. 28, 2014, 9:02 p.m., Bill Farner wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 395
> > <https://reviews.apache.org/r/17430/diff/2/?file=453062#file453062line395>
> >
> > In the future (post-release), i'll ship-block changes like this. Since we're not there yet, and this is a nascent API it LGTM.
Ok.
- Suman
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17430/#review33041
-----------------------------------------------------------
On Jan. 28, 2014, 10:17 p.m., Suman Karumuri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17430/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2014, 10:17 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Bugs: AURORA-116
> https://issues.apache.org/jira/browse/AURORA-116
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Updated test cases to catch this bug.
>
>
> Diffs
> -----
>
> 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/thrift/SchedulerThriftInterfaceTest.java 6cefdfad469a9b69a5291ad46be1df14b443472e
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 42fdca2759f15d007bee058485c237268c57597a
>
> Diff: https://reviews.apache.org/r/17430/diff/
>
>
> Testing
> -------
>
> gradle clean build
> gradle run - tested in local UI.
>
>
> Thanks,
>
> Suman Karumuri
>
>
Re: Review Request 17430: Bug fix: Return job count instead of task count on
GetRoleSummary() call.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17430/#review33041
-----------------------------------------------------------
src/main/thrift/org/apache/aurora/gen/api.thrift
<https://reviews.apache.org/r/17430/#comment62250>
In the future (post-release), i'll ship-block changes like this. Since we're not there yet, and this is a nascent API it LGTM.
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
<https://reviews.apache.org/r/17430/#comment62252>
This task needs to be different from task1 in some way, otherwise this test could deceivingly pass if the two tasks are ever placed into a Set.
- Bill Farner
On Jan. 28, 2014, 8:51 p.m., Suman Karumuri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17430/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2014, 8:51 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Bugs: AURORA-116
> https://issues.apache.org/jira/browse/AURORA-116
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Updated test cases to catch this bug.
>
>
> Diffs
> -----
>
> 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/thrift/SchedulerThriftInterfaceTest.java 6cefdfad469a9b69a5291ad46be1df14b443472e
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 42fdca2759f15d007bee058485c237268c57597a
>
> Diff: https://reviews.apache.org/r/17430/diff/
>
>
> Testing
> -------
>
> gradle clean build
> gradle run - tested in local UI.
>
>
> Thanks,
>
> Suman Karumuri
>
>
Re: Review Request 17430: Bug fix: Return job count instead of task count on
GetRoleSummary() call.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17430/#review33074
-----------------------------------------------------------
Ship it!
Ship It!
- Bill Farner
On Jan. 28, 2014, 10:17 p.m., Suman Karumuri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17430/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2014, 10:17 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Bugs: AURORA-116
> https://issues.apache.org/jira/browse/AURORA-116
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Updated test cases to catch this bug.
>
>
> Diffs
> -----
>
> 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/thrift/SchedulerThriftInterfaceTest.java 6cefdfad469a9b69a5291ad46be1df14b443472e
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 42fdca2759f15d007bee058485c237268c57597a
>
> Diff: https://reviews.apache.org/r/17430/diff/
>
>
> Testing
> -------
>
> gradle clean build
> gradle run - tested in local UI.
>
>
> Thanks,
>
> Suman Karumuri
>
>
Re: Review Request 17430: Bug fix: Return job count instead of task count on
GetRoleSummary() call.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17430/#review33076
-----------------------------------------------------------
Ship it!
Ship It!
- Kevin Sweeney
On Jan. 28, 2014, 2:17 p.m., Suman Karumuri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17430/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2014, 2:17 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Bugs: AURORA-116
> https://issues.apache.org/jira/browse/AURORA-116
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Updated test cases to catch this bug.
>
>
> Diffs
> -----
>
> 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/thrift/SchedulerThriftInterfaceTest.java 6cefdfad469a9b69a5291ad46be1df14b443472e
> src/test/resources/org/apache/aurora/gen/api.thrift.md5 42fdca2759f15d007bee058485c237268c57597a
>
> Diff: https://reviews.apache.org/r/17430/diff/
>
>
> Testing
> -------
>
> gradle clean build
> gradle run - tested in local UI.
>
>
> Thanks,
>
> Suman Karumuri
>
>
Re: Review Request 17430: Bug fix: Return job count instead of task count on
GetRoleSummary() call.
Posted by Suman Karumuri <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17430/
-----------------------------------------------------------
(Updated Jan. 28, 2014, 10:17 p.m.)
Review request for Aurora, Kevin Sweeney and Bill Farner.
Changes
-------
Fixed code review comments.
Bugs: AURORA-116
https://issues.apache.org/jira/browse/AURORA-116
Repository: aurora
Description
-------
Updated test cases to catch this bug.
Diffs (updated)
-----
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/thrift/SchedulerThriftInterfaceTest.java 6cefdfad469a9b69a5291ad46be1df14b443472e
src/test/resources/org/apache/aurora/gen/api.thrift.md5 42fdca2759f15d007bee058485c237268c57597a
Diff: https://reviews.apache.org/r/17430/diff/
Testing
-------
gradle clean build
gradle run - tested in local UI.
Thanks,
Suman Karumuri
Re: Review Request 17430: Bug fix: Return job count instead of task count on
GetRoleSummary() call.
Posted by Suman Karumuri <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17430/
-----------------------------------------------------------
(Updated Jan. 28, 2014, 8:51 p.m.)
Review request for Aurora, Kevin Sweeney and Bill Farner.
Changes
-------
Addressed code review feedback.
Fixed the thrift response to use a set instead of a list, since I found an ordering bug while running the test.
Bugs: AURORA-116
https://issues.apache.org/jira/browse/AURORA-116
Repository: aurora
Description
-------
Updated test cases to catch this bug.
Diffs (updated)
-----
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/thrift/SchedulerThriftInterfaceTest.java 6cefdfad469a9b69a5291ad46be1df14b443472e
src/test/resources/org/apache/aurora/gen/api.thrift.md5 42fdca2759f15d007bee058485c237268c57597a
Diff: https://reviews.apache.org/r/17430/diff/
Testing
-------
gradle clean build
gradle run - tested in local UI.
Thanks,
Suman Karumuri