You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2014/09/09 21:46:30 UTC
Review Request 25481: Adding JobUpdateRequest validation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25481/
-----------------------------------------------------------
Review request for Aurora, Bill Farner and Zameer Manji.
Bugs: AURORA-649
https://issues.apache.org/jira/browse/AURORA-649
Repository: aurora
Description
-------
Adding JobUpdateRequest validation.
Diffs
-----
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e
Diff: https://reviews.apache.org/r/25481/diff/
Testing
-------
gradle -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 25481: Adding JobUpdateRequest validation.
Posted by Zameer Manji <zm...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25481/#review54613
-----------------------------------------------------------
Ship it!
Ship It!
- Zameer Manji
On Sept. 25, 2014, 3:02 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25481/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2014, 3:02 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-649
> https://issues.apache.org/jira/browse/AURORA-649
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding JobUpdateRequest validation.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 9152c211c49b433c835e2345320e97010cb588e2
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java f4aefb21a41d41f11cb4a8caf402bbe18eb2d1d5
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java a894a3aca18d3101543c3520ab4d547d63cd6d61
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 5b00d3cf72adc154f130bb067723c3bd6960a314
>
> Diff: https://reviews.apache.org/r/25481/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 25481: Adding JobUpdateRequest validation.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Sept. 26, 2014, 6:18 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java, line 80
> > <https://reviews.apache.org/r/25481/diff/2/?file=705767#file705767line80>
> >
> > This code should still be defensive - it cares about these values. Please revert.
>
> Maxim Khutornenko wrote:
> Feels kind of redundant but I am OK reverting all but the last desiredState.instances validation (AURORA-756).
On the second thought, just removing that assertion is not going to be enough. Will address AURORA-756 separately.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25481/#review54705
-----------------------------------------------------------
On Sept. 25, 2014, 10:02 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25481/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2014, 10:02 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-649
> https://issues.apache.org/jira/browse/AURORA-649
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding JobUpdateRequest validation.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 9152c211c49b433c835e2345320e97010cb588e2
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java f4aefb21a41d41f11cb4a8caf402bbe18eb2d1d5
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java a894a3aca18d3101543c3520ab4d547d63cd6d61
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 5b00d3cf72adc154f130bb067723c3bd6960a314
>
> Diff: https://reviews.apache.org/r/25481/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 25481: Adding JobUpdateRequest validation.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Sept. 26, 2014, 6:18 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java, line 80
> > <https://reviews.apache.org/r/25481/diff/2/?file=705767#file705767line80>
> >
> > This code should still be defensive - it cares about these values. Please revert.
Feels kind of redundant but I am OK reverting all but the last desiredState.instances validation (AURORA-756).
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25481/#review54705
-----------------------------------------------------------
On Sept. 25, 2014, 10:02 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25481/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2014, 10:02 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-649
> https://issues.apache.org/jira/browse/AURORA-649
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding JobUpdateRequest validation.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 9152c211c49b433c835e2345320e97010cb588e2
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java f4aefb21a41d41f11cb4a8caf402bbe18eb2d1d5
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java a894a3aca18d3101543c3520ab4d547d63cd6d61
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 5b00d3cf72adc154f130bb067723c3bd6960a314
>
> Diff: https://reviews.apache.org/r/25481/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 25481: Adding JobUpdateRequest validation.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25481/#review54705
-----------------------------------------------------------
Ship it!
Error handling is objectively better, but i'm a -1 on removal of input validation at lower layers.
src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java
<https://reviews.apache.org/r/25481/#comment94971>
This code should still be defensive - it cares about these values. Please revert.
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java
<https://reviews.apache.org/r/25481/#comment94972>
Ditto.
- Bill Farner
On Sept. 25, 2014, 10:02 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25481/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2014, 10:02 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-649
> https://issues.apache.org/jira/browse/AURORA-649
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding JobUpdateRequest validation.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 9152c211c49b433c835e2345320e97010cb588e2
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java f4aefb21a41d41f11cb4a8caf402bbe18eb2d1d5
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java a894a3aca18d3101543c3520ab4d547d63cd6d61
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 5b00d3cf72adc154f130bb067723c3bd6960a314
>
> Diff: https://reviews.apache.org/r/25481/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 25481: Adding JobUpdateRequest validation.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25481/
-----------------------------------------------------------
(Updated Sept. 26, 2014, 7 p.m.)
Review request for Aurora, Bill Farner and Zameer Manji.
Changes
-------
Reverting UpdateFactory assertions removal.
Bugs: AURORA-649
https://issues.apache.org/jira/browse/AURORA-649
Repository: aurora
Description
-------
Adding JobUpdateRequest validation.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 9152c211c49b433c835e2345320e97010cb588e2
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java a894a3aca18d3101543c3520ab4d547d63cd6d61
Diff: https://reviews.apache.org/r/25481/diff/
Testing
-------
gradle -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 25481: Adding JobUpdateRequest validation.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25481/
-----------------------------------------------------------
(Updated Sept. 25, 2014, 10:02 p.m.)
Review request for Aurora, Bill Farner and Zameer Manji.
Changes
-------
Rebased and refactored to standardize response handling.
Bugs: AURORA-649
https://issues.apache.org/jira/browse/AURORA-649
Repository: aurora
Description
-------
Adding JobUpdateRequest validation.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 9152c211c49b433c835e2345320e97010cb588e2
src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java f4aefb21a41d41f11cb4a8caf402bbe18eb2d1d5
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java a894a3aca18d3101543c3520ab4d547d63cd6d61
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 5b00d3cf72adc154f130bb067723c3bd6960a314
Diff: https://reviews.apache.org/r/25481/diff/
Testing
-------
gradle -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 25481: Adding JobUpdateRequest validation.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Sept. 11, 2014, 6:13 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1371
> > <https://reviews.apache.org/r/25481/diff/1/?file=684118#file684118line1371>
> >
> > I'm a little unfamilar with JobUpdateRequest and this RPC but it seems we should update "StartJobUpdateResult" to have a message field that we can surface to the user?
Messages are already sent via thrift Response object.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25481/#review53073
-----------------------------------------------------------
On Sept. 9, 2014, 7:46 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25481/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2014, 7:46 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-649
> https://issues.apache.org/jira/browse/AURORA-649
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding JobUpdateRequest validation.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e
>
> Diff: https://reviews.apache.org/r/25481/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 25481: Adding JobUpdateRequest validation.
Posted by Zameer Manji <zm...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25481/#review53073
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
<https://reviews.apache.org/r/25481/#comment92464>
I'm a little unfamilar with JobUpdateRequest and this RPC but it seems we should update "StartJobUpdateResult" to have a message field that we can surface to the user?
- Zameer Manji
On Sept. 9, 2014, 12:46 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25481/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2014, 12:46 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-649
> https://issues.apache.org/jira/browse/AURORA-649
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding JobUpdateRequest validation.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e
>
> Diff: https://reviews.apache.org/r/25481/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 25481: Adding JobUpdateRequest validation.
Posted by Bill Farner <wf...@apache.org>.
> On Sept. 11, 2014, 5:54 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1371
> > <https://reviews.apache.org/r/25481/diff/1/?file=684118#file684118line1371>
> >
> > I believe this will not surface nicely to the user, but will instead present as an internal error (based on LoggingInterceptor, which handles uncaught exceptions).
>
> Maxim Khutornenko wrote:
> ERROR type is correctly consumed by the client. Here is an example from vagrant:
>
> ```INFO] Response from scheduler: ERROR (message: TaskQuery is missing.)```
Right, but shouldn't we be returning `INVALID_REQUEST`?
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25481/#review53061
-----------------------------------------------------------
On Sept. 9, 2014, 7:46 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25481/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2014, 7:46 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-649
> https://issues.apache.org/jira/browse/AURORA-649
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding JobUpdateRequest validation.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e
>
> Diff: https://reviews.apache.org/r/25481/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 25481: Adding JobUpdateRequest validation.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Sept. 11, 2014, 5:54 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1371
> > <https://reviews.apache.org/r/25481/diff/1/?file=684118#file684118line1371>
> >
> > I believe this will not surface nicely to the user, but will instead present as an internal error (based on LoggingInterceptor, which handles uncaught exceptions).
ERROR type is correctly consumed by the client. Here is an example from vagrant:
```INFO] Response from scheduler: ERROR (message: TaskQuery is missing.)```
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25481/#review53061
-----------------------------------------------------------
On Sept. 9, 2014, 7:46 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25481/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2014, 7:46 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-649
> https://issues.apache.org/jira/browse/AURORA-649
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding JobUpdateRequest validation.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e
>
> Diff: https://reviews.apache.org/r/25481/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 25481: Adding JobUpdateRequest validation.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Sept. 11, 2014, 5:54 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1371
> > <https://reviews.apache.org/r/25481/diff/1/?file=684118#file684118line1371>
> >
> > I believe this will not surface nicely to the user, but will instead present as an internal error (based on LoggingInterceptor, which handles uncaught exceptions).
>
> Maxim Khutornenko wrote:
> ERROR type is correctly consumed by the client. Here is an example from vagrant:
>
> ```INFO] Response from scheduler: ERROR (message: TaskQuery is missing.)```
>
> Bill Farner wrote:
> Right, but shouldn't we be returning `INVALID_REQUEST`?
Good point. Raised https://issues.apache.org/jira/browse/AURORA-701 to validate on the client instead. These should rather stay ERRORs to catch API-side violations.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25481/#review53061
-----------------------------------------------------------
On Sept. 9, 2014, 7:46 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25481/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2014, 7:46 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-649
> https://issues.apache.org/jira/browse/AURORA-649
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding JobUpdateRequest validation.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e
>
> Diff: https://reviews.apache.org/r/25481/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 25481: Adding JobUpdateRequest validation.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25481/#review53061
-----------------------------------------------------------
Aha, i thought you wanted this done down in JobUpdateController, so i put the validation in there. I'm happy to remove it from my diff.
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
<https://reviews.apache.org/r/25481/#comment92450>
I believe this will not surface nicely to the user, but will instead present as an internal error (based on LoggingInterceptor, which handles uncaught exceptions).
- Bill Farner
On Sept. 9, 2014, 7:46 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25481/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2014, 7:46 p.m.)
>
>
> Review request for Aurora, Bill Farner and Zameer Manji.
>
>
> Bugs: AURORA-649
> https://issues.apache.org/jira/browse/AURORA-649
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding JobUpdateRequest validation.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e
>
> Diff: https://reviews.apache.org/r/25481/diff/
>
>
> Testing
> -------
>
> gradle -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>