You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by David McLaughlin <da...@dmclaughlin.com> on 2014/06/11 02:32:45 UTC
Review Request 22448: Add pagination to getTasksStatus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22448/
-----------------------------------------------------------
Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
Bugs: AURORA-521
https://issues.apache.org/jira/browse/AURORA-521
Repository: aurora
Description
-------
Add pagination to getTasksStatus
Diffs
-----
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java d9c3a1eecec4995b0539429b1079ff63118fb9e5
src/main/thrift/org/apache/aurora/gen/api.thrift e72bcfb57d288d585307549be5d74067ab08c42b
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 299679618f135f29bbf4e6372585ae46a1224a27
Diff: https://reviews.apache.org/r/22448/diff/
Testing
-------
./gradlew build
Thanks,
David McLaughlin
Re: Review Request 22448: Add pagination to getTasksStatus
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22448/#review45394
-----------------------------------------------------------
Ship it!
Just style nits
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
<https://reviews.apache.org/r/22448/#comment80223>
style nit: space between if and ( here
please consider running ./gradlew -Pq build to catch these automatically
- Kevin Sweeney
On June 10, 2014, 5:32 p.m., David McLaughlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22448/
> -----------------------------------------------------------
>
> (Updated June 10, 2014, 5:32 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: AURORA-521
> https://issues.apache.org/jira/browse/AURORA-521
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Add pagination to getTasksStatus
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java d9c3a1eecec4995b0539429b1079ff63118fb9e5
> src/main/thrift/org/apache/aurora/gen/api.thrift e72bcfb57d288d585307549be5d74067ab08c42b
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 299679618f135f29bbf4e6372585ae46a1224a27
>
> Diff: https://reviews.apache.org/r/22448/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> David McLaughlin
>
>
Re: Review Request 22448: Add pagination to getTasksStatus
Posted by David McLaughlin <da...@dmclaughlin.com>.
> On June 11, 2014, 6:40 p.m., Maxim Khutornenko wrote:
> > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java, line 1251
> > <https://reviews.apache.org/r/22448/diff/2/?file=607510#file607510line1251>
> >
> > You might want to encapsulate this into a helper method..
Done.
> On June 11, 2014, 6:40 p.m., Maxim Khutornenko wrote:
> > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java, line 1244
> > <https://reviews.apache.org/r/22448/diff/2/?file=607510#file607510line1244>
> >
> > How about a test that validates all tasks were returned in multiple pages with no dupes? I.e.:
> > 1,2,3,4
> > 5,6,7,8
> > 9,10
>
> David McLaughlin wrote:
> Hmm, isn't that the responsibility of the client to pass the correct offset/limit each time?
>
> Maxim Khutornenko wrote:
> Correct. This test could be a validation of that client story.
Done.
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22448/#review45410
-----------------------------------------------------------
On June 11, 2014, 7:55 p.m., David McLaughlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22448/
> -----------------------------------------------------------
>
> (Updated June 11, 2014, 7:55 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: AURORA-521
> https://issues.apache.org/jira/browse/AURORA-521
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Add pagination to getTasksStatus
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java d9c3a1eecec4995b0539429b1079ff63118fb9e5
> src/main/thrift/org/apache/aurora/gen/api.thrift e72bcfb57d288d585307549be5d74067ab08c42b
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 299679618f135f29bbf4e6372585ae46a1224a27
>
> Diff: https://reviews.apache.org/r/22448/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> David McLaughlin
>
>
Re: Review Request 22448: Add pagination to getTasksStatus
Posted by Maxim Khutornenko <ma...@apache.org>.
> On June 11, 2014, 6:40 p.m., Maxim Khutornenko wrote:
> > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java, line 1244
> > <https://reviews.apache.org/r/22448/diff/2/?file=607510#file607510line1244>
> >
> > How about a test that validates all tasks were returned in multiple pages with no dupes? I.e.:
> > 1,2,3,4
> > 5,6,7,8
> > 9,10
>
> David McLaughlin wrote:
> Hmm, isn't that the responsibility of the client to pass the correct offset/limit each time?
Correct. This test could be a validation of that client story.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22448/#review45410
-----------------------------------------------------------
On June 11, 2014, 6:32 p.m., David McLaughlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22448/
> -----------------------------------------------------------
>
> (Updated June 11, 2014, 6:32 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: AURORA-521
> https://issues.apache.org/jira/browse/AURORA-521
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Add pagination to getTasksStatus
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java d9c3a1eecec4995b0539429b1079ff63118fb9e5
> src/main/thrift/org/apache/aurora/gen/api.thrift e72bcfb57d288d585307549be5d74067ab08c42b
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 299679618f135f29bbf4e6372585ae46a1224a27
>
> Diff: https://reviews.apache.org/r/22448/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> David McLaughlin
>
>
Re: Review Request 22448: Add pagination to getTasksStatus
Posted by David McLaughlin <da...@dmclaughlin.com>.
> On June 11, 2014, 6:40 p.m., Maxim Khutornenko wrote:
> > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java, line 1244
> > <https://reviews.apache.org/r/22448/diff/2/?file=607510#file607510line1244>
> >
> > How about a test that validates all tasks were returned in multiple pages with no dupes? I.e.:
> > 1,2,3,4
> > 5,6,7,8
> > 9,10
Hmm, isn't that the responsibility of the client to pass the correct offset/limit each time?
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22448/#review45410
-----------------------------------------------------------
On June 11, 2014, 6:32 p.m., David McLaughlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22448/
> -----------------------------------------------------------
>
> (Updated June 11, 2014, 6:32 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: AURORA-521
> https://issues.apache.org/jira/browse/AURORA-521
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Add pagination to getTasksStatus
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java d9c3a1eecec4995b0539429b1079ff63118fb9e5
> src/main/thrift/org/apache/aurora/gen/api.thrift e72bcfb57d288d585307549be5d74067ab08c42b
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 299679618f135f29bbf4e6372585ae46a1224a27
>
> Diff: https://reviews.apache.org/r/22448/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> David McLaughlin
>
>
Re: Review Request 22448: Add pagination to getTasksStatus
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22448/#review45410
-----------------------------------------------------------
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
<https://reviews.apache.org/r/22448/#comment80261>
How about a test that validates all tasks were returned in multiple pages with no dupes? I.e.:
1,2,3,4
5,6,7,8
9,10
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
<https://reviews.apache.org/r/22448/#comment80262>
You might want to encapsulate this into a helper method..
- Maxim Khutornenko
On June 11, 2014, 6:32 p.m., David McLaughlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22448/
> -----------------------------------------------------------
>
> (Updated June 11, 2014, 6:32 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: AURORA-521
> https://issues.apache.org/jira/browse/AURORA-521
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Add pagination to getTasksStatus
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java d9c3a1eecec4995b0539429b1079ff63118fb9e5
> src/main/thrift/org/apache/aurora/gen/api.thrift e72bcfb57d288d585307549be5d74067ab08c42b
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 299679618f135f29bbf4e6372585ae46a1224a27
>
> Diff: https://reviews.apache.org/r/22448/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> David McLaughlin
>
>
Re: Review Request 22448: Add pagination to getTasksStatus
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22448/#review45419
-----------------------------------------------------------
Ship it!
Ship It!
- Maxim Khutornenko
On June 11, 2014, 7:55 p.m., David McLaughlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22448/
> -----------------------------------------------------------
>
> (Updated June 11, 2014, 7:55 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: AURORA-521
> https://issues.apache.org/jira/browse/AURORA-521
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Add pagination to getTasksStatus
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java d9c3a1eecec4995b0539429b1079ff63118fb9e5
> src/main/thrift/org/apache/aurora/gen/api.thrift e72bcfb57d288d585307549be5d74067ab08c42b
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 299679618f135f29bbf4e6372585ae46a1224a27
>
> Diff: https://reviews.apache.org/r/22448/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> David McLaughlin
>
>
Re: Review Request 22448: Add pagination to getTasksStatus
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22448/
-----------------------------------------------------------
(Updated June 11, 2014, 7:55 p.m.)
Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
Changes
-------
Review feedback.
Bugs: AURORA-521
https://issues.apache.org/jira/browse/AURORA-521
Repository: aurora
Description
-------
Add pagination to getTasksStatus
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java d9c3a1eecec4995b0539429b1079ff63118fb9e5
src/main/thrift/org/apache/aurora/gen/api.thrift e72bcfb57d288d585307549be5d74067ab08c42b
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 299679618f135f29bbf4e6372585ae46a1224a27
Diff: https://reviews.apache.org/r/22448/diff/
Testing
-------
./gradlew build
Thanks,
David McLaughlin
Re: Review Request 22448: Add pagination to getTasksStatus
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22448/
-----------------------------------------------------------
(Updated June 11, 2014, 6:32 p.m.)
Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
Changes
-------
checkstyle.
Bugs: AURORA-521
https://issues.apache.org/jira/browse/AURORA-521
Repository: aurora
Description
-------
Add pagination to getTasksStatus
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java d9c3a1eecec4995b0539429b1079ff63118fb9e5
src/main/thrift/org/apache/aurora/gen/api.thrift e72bcfb57d288d585307549be5d74067ab08c42b
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 299679618f135f29bbf4e6372585ae46a1224a27
Diff: https://reviews.apache.org/r/22448/diff/
Testing
-------
./gradlew build
Thanks,
David McLaughlin