Re: Review Request 25481: Adding JobUpdateRequest validation.
--- 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.
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.
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.
--- 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.
--- 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.
--- 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.
--- 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
Re: Review Request 25481: Adding JobUpdateRequest validation.
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.
--- 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.
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.
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.
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
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
