Re: Review Request 23188: Adding getPendingReason RPC to expose scheduling vetos in the UI/client.
On July 2, 2014, 11:11 p.m., Bill Farner wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 414 https://reviews.apache.org/r/23188/diff/2/?file=623177#file623177line414 What's the motivation for including the job key? I would assume that the caller either already knows this, and if not, they can turn back around and query for the task IDs. Maxim Khutornenko wrote: TaskQuery allows pulling tasks from multiple jobs. Without specifying some kind of reference towards the parent job, pending reasons would be useless when pulled by anything other than the JobKey. Either we have to further restrict TaskQuery to require role/env/name fields or provide a reference back in the result. Feels like making this RPC effectively job-scoped is an unnecessary restriction at this point. Bill Farner wrote: Without specifying some kind of reference towards the parent job, pending reasons would be useless when pulled by anything other than the JobKey I disagree that they would be useless. Like i mentioned above, the caller will either have context, or they can turn around and query for the tasks to gain context. Including the job key seems arbitrary (i.e. why that and not the instance id?). Dropped. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23188/#review47247 --- On July 2, 2014, 11:07 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23188/ --- (Updated July 2, 2014, 11:07 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-377 https://issues.apache.org/jira/browse/AURORA-377 Repository: aurora Description --- Adding getPendingReason RPC to expose scheduling vetos in the UI/client. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 2549dd33d08dfc6058d985127a3f0c1f3984eaa7 src/main/thrift/org/apache/aurora/gen/api.thrift 8ee43fa1f0e2e699b0f1a321e673e49221b528ad src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 2cffa74ba36e2afda3340658d6b1afd6cb50cf2c src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java ed24ca058c013dfdec8366522893bd4f83e30a9c Diff: https://reviews.apache.org/r/23188/diff/ Testing --- gradle -Pq clean build Thanks, Maxim Khutornenko
Re: Review Request 23188: Adding getPendingReason RPC to expose scheduling vetos in the UI/client.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23188/#review48303 --- Ship it! Ship It! - Kevin Sweeney On July 21, 2014, 2:58 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23188/ --- (Updated July 21, 2014, 2:58 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-377 https://issues.apache.org/jira/browse/AURORA-377 Repository: aurora Description --- Adding getPendingReason RPC to expose scheduling vetos in the UI/client. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 2549dd33d08dfc6058d985127a3f0c1f3984eaa7 src/main/thrift/org/apache/aurora/gen/api.thrift 8ee43fa1f0e2e699b0f1a321e673e49221b528ad src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 2cffa74ba36e2afda3340658d6b1afd6cb50cf2c src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java ed24ca058c013dfdec8366522893bd4f83e30a9c Diff: https://reviews.apache.org/r/23188/diff/ Testing --- gradle -Pq clean build Thanks, Maxim Khutornenko
Re: Review Request 23188: Adding getPendingReason RPC to expose scheduling vetos in the UI/client.
On July 2, 2014, 11:11 p.m., Bill Farner wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 414 https://reviews.apache.org/r/23188/diff/2/?file=623177#file623177line414 What's the motivation for including the job key? I would assume that the caller either already knows this, and if not, they can turn back around and query for the task IDs. Maxim Khutornenko wrote: TaskQuery allows pulling tasks from multiple jobs. Without specifying some kind of reference towards the parent job, pending reasons would be useless when pulled by anything other than the JobKey. Either we have to further restrict TaskQuery to require role/env/name fields or provide a reference back in the result. Feels like making this RPC effectively job-scoped is an unnecessary restriction at this point. Without specifying some kind of reference towards the parent job, pending reasons would be useless when pulled by anything other than the JobKey I disagree that they would be useless. Like i mentioned above, the caller will either have context, or they can turn around and query for the tasks to gain context. Including the job key seems arbitrary (i.e. why that and not the instance id?). - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23188/#review47247 --- On July 2, 2014, 11:07 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23188/ --- (Updated July 2, 2014, 11:07 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-377 https://issues.apache.org/jira/browse/AURORA-377 Repository: aurora Description --- Adding getPendingReason RPC to expose scheduling vetos in the UI/client. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 2549dd33d08dfc6058d985127a3f0c1f3984eaa7 src/main/thrift/org/apache/aurora/gen/api.thrift 8ee43fa1f0e2e699b0f1a321e673e49221b528ad src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 2cffa74ba36e2afda3340658d6b1afd6cb50cf2c src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java ed24ca058c013dfdec8366522893bd4f83e30a9c Diff: https://reviews.apache.org/r/23188/diff/ Testing --- gradle -Pq clean build Thanks, Maxim Khutornenko
Re: Review Request 23188: Adding getPendingReason RPC to expose scheduling vetos in the UI/client.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23188/#review47111 --- src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java https://reviews.apache.org/r/23188/#comment82714 I much prefer the previous signature. Why not push the Set-String translation to the call site? src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java https://reviews.apache.org/r/23188/#comment82715 Doesn't it make more sense to inject this filter into the query? src/main/thrift/org/apache/aurora/gen/api.thrift https://reviews.apache.org/r/23188/#comment82716 This restricts the API in a way that is asymmetric with the request. i.e. if i'm given full control with a TaskQuery, why is the response limited to a single job? src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java https://reviews.apache.org/r/23188/#comment82718 The Set-String change rears its head here - since you now need to match the ordering to satisfy equals(). - Bill Farner On July 1, 2014, 12:28 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23188/ --- (Updated July 1, 2014, 12:28 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-377 https://issues.apache.org/jira/browse/AURORA-377 Repository: aurora Description --- Adding getPendingReason RPC to expose scheduling vetos in the UI/client. Diffs - src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java c86f598e2ac226e0a356515b389e76f7c5efb67e src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 5d21e5ebe80cda75957475afa9c2b94e202b73b2 src/main/thrift/org/apache/aurora/gen/api.thrift 8ee43fa1f0e2e699b0f1a321e673e49221b528ad src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java b60b004adbd6753ec6fef125fd70286be5071c56 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 2b9edd41631b48dd729ec1dafcf0cd20808e9c7c src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java a746c48dd21a401b84ddcc610d7c99b4f35f8135 Diff: https://reviews.apache.org/r/23188/diff/ Testing --- gradle -Pq clean build Thanks, Maxim Khutornenko
Re: Review Request 23188: Adding getPendingReason RPC to expose scheduling vetos in the UI/client.
On July 1, 2014, 6:14 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java, line 89 https://reviews.apache.org/r/23188/diff/1/?file=620711#file620711line89 I much prefer the previous signature. Why not push the Set-String translation to the call site? That would require making Veto public, which I did not quite like. Besides, there is only one user of that API and no other use cases that would require a SetVeto. I don't quite like String either but that seemed like a reasonable tradeoff given the current semantic. I am ok reverting it if you think Veto can be made public. On July 1, 2014, 6:14 p.m., Bill Farner wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 414 https://reviews.apache.org/r/23188/diff/1/?file=620713#file620713line414 This restricts the API in a way that is asymmetric with the request. i.e. if i'm given full control with a TaskQuery, why is the response limited to a single job? The GetPendingReasonResult has a set of these. It's just an instance of a PendingReason that is restricted to a single job. On July 1, 2014, 6:14 p.m., Bill Farner wrote: src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java, line 133 https://reviews.apache.org/r/23188/diff/1/?file=620714#file620714line133 The Set-String change rears its head here - since you now need to match the ordering to satisfy equals(). - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23188/#review47111 --- On July 1, 2014, 12:28 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23188/ --- (Updated July 1, 2014, 12:28 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-377 https://issues.apache.org/jira/browse/AURORA-377 Repository: aurora Description --- Adding getPendingReason RPC to expose scheduling vetos in the UI/client. Diffs - src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java c86f598e2ac226e0a356515b389e76f7c5efb67e src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 5d21e5ebe80cda75957475afa9c2b94e202b73b2 src/main/thrift/org/apache/aurora/gen/api.thrift 8ee43fa1f0e2e699b0f1a321e673e49221b528ad src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java b60b004adbd6753ec6fef125fd70286be5071c56 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 2b9edd41631b48dd729ec1dafcf0cd20808e9c7c src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java a746c48dd21a401b84ddcc610d7c99b4f35f8135 Diff: https://reviews.apache.org/r/23188/diff/ Testing --- gradle -Pq clean build Thanks, Maxim Khutornenko
Re: Review Request 23188: Adding getPendingReason RPC to expose scheduling vetos in the UI/client.
On July 1, 2014, 6:14 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java, line 89 https://reviews.apache.org/r/23188/diff/1/?file=620711#file620711line89 I much prefer the previous signature. Why not push the Set-String translation to the call site? Maxim Khutornenko wrote: That would require making Veto public, which I did not quite like. Besides, there is only one user of that API and no other use cases that would require a SetVeto. I don't quite like String either but that seemed like a reasonable tradeoff given the current semantic. I am ok reverting it if you think Veto can be made public. Yeah, i don't see much harm in making Veto visible. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23188/#review47111 --- On July 1, 2014, 12:28 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23188/ --- (Updated July 1, 2014, 12:28 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-377 https://issues.apache.org/jira/browse/AURORA-377 Repository: aurora Description --- Adding getPendingReason RPC to expose scheduling vetos in the UI/client. Diffs - src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java c86f598e2ac226e0a356515b389e76f7c5efb67e src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 5d21e5ebe80cda75957475afa9c2b94e202b73b2 src/main/thrift/org/apache/aurora/gen/api.thrift 8ee43fa1f0e2e699b0f1a321e673e49221b528ad src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java b60b004adbd6753ec6fef125fd70286be5071c56 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 2b9edd41631b48dd729ec1dafcf0cd20808e9c7c src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java a746c48dd21a401b84ddcc610d7c99b4f35f8135 Diff: https://reviews.apache.org/r/23188/diff/ Testing --- gradle -Pq clean build Thanks, Maxim Khutornenko