Re: Review Request 23188: Adding getPendingReason RPC to expose scheduling vetos in the UI/client.

2014-07-21 Thread Maxim Khutornenko


 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.

2014-07-21 Thread Kevin Sweeney

---
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.

2014-07-07 Thread Bill Farner


 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.

2014-07-01 Thread Bill Farner

---
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.

2014-07-01 Thread Maxim Khutornenko


 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.

2014-07-01 Thread Bill Farner


 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