Re: Review Request 22790: Adding a getTasksStatusLight RPC to return null ExecutorConfig.

2014-06-20 Thread David McLaughlin 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22790/#review46300
---

Ship it!


Going to ship this because I know there's no great solution, but I feel like a 
new type is a lot more clean than just nulling the expensive value. 

- David McLaughlin


On June 19, 2014, 11:47 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22790/
> ---
> 
> (Updated June 19, 2014, 11:47 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Bill Farner.
> 
> 
> Bugs: AURORA-539
> https://issues.apache.org/jira/browse/AURORA-539
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a getTasksStatusLight RPC to return null ExecutorConfig.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  3616b3e93010c9482dc96d94bdeb42eddae41fb0 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> d115c74eaff2b02c00d3b53b1aa2ff5675436152 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  c93daebf429d8b6691e5e5bcaf88675335f49022 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> 1f1f05e3110b4ba3d83dc7aebc068f4d739e9958 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 255b3d9c881c9291fd3d54792e7bf7385313ed9e 
> 
> Diff: https://reviews.apache.org/r/22790/diff/
> 
> 
> Testing
> ---
> 
> gradle -Pq build
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 22790: Adding a getTasksStatusLight RPC to return null ExecutorConfig.

2014-06-19 Thread Bill Farner 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22790/#review46240
---

Ship it!


Ship It!

- Bill Farner


On June 19, 2014, 11:47 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22790/
> ---
> 
> (Updated June 19, 2014, 11:47 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Bill Farner.
> 
> 
> Bugs: AURORA-539
> https://issues.apache.org/jira/browse/AURORA-539
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a getTasksStatusLight RPC to return null ExecutorConfig.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  3616b3e93010c9482dc96d94bdeb42eddae41fb0 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> d115c74eaff2b02c00d3b53b1aa2ff5675436152 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  c93daebf429d8b6691e5e5bcaf88675335f49022 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> 1f1f05e3110b4ba3d83dc7aebc068f4d739e9958 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 255b3d9c881c9291fd3d54792e7bf7385313ed9e 
> 
> Diff: https://reviews.apache.org/r/22790/diff/
> 
> 
> Testing
> ---
> 
> gradle -Pq build
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 22790: Adding a getTasksStatusLight RPC to return null ExecutorConfig.

2014-06-19 Thread Maxim Khutornenko 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22790/
---

(Updated June 19, 2014, 11:47 p.m.)


Review request for Aurora, David McLaughlin and Bill Farner.


Changes
---

CR comments.


Bugs: AURORA-539
https://issues.apache.org/jira/browse/AURORA-539


Repository: aurora


Description
---

Adding a getTasksStatusLight RPC to return null ExecutorConfig.


Diffs (updated)
-

  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
3616b3e93010c9482dc96d94bdeb42eddae41fb0 
  src/main/thrift/org/apache/aurora/gen/api.thrift 
d115c74eaff2b02c00d3b53b1aa2ff5675436152 
  
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
 c93daebf429d8b6691e5e5bcaf88675335f49022 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
1f1f05e3110b4ba3d83dc7aebc068f4d739e9958 
  src/test/resources/org/apache/aurora/gen/api.thrift.md5 
255b3d9c881c9291fd3d54792e7bf7385313ed9e 

Diff: https://reviews.apache.org/r/22790/diff/


Testing
---

gradle -Pq build
./pants src/test/python:all


Thanks,

Maxim Khutornenko

Re: Review Request 22790: Adding a getTasksStatusLight RPC to return null ExecutorConfig.

2014-06-19 Thread Maxim Khutornenko 


> On June 19, 2014, 10:55 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
> >  line 423
> > 
> >
> > this should be indented past the Function declaration

Fixed.


> On June 19, 2014, 10:55 p.m., Bill Farner wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 490
> > 
> >
> > Please include a comment that this is a stopgap until we have a better 
> > way to reference TaskConfigs so that the client may decide whether to fetch 
> > them.

Done.


> On June 19, 2014, 10:55 p.m., Bill Farner wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 491
> > 
> >
> > I'm not crazy about "light" as it's unclear and open for overloading.  
> > I don't have a great suggestion, but even getTasksWithoutConfigs() would be 
> > somewhat better.

No preference here. Used your suggestion.


> On June 19, 2014, 10:55 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java,
> >  line 1694
> > 
> >
> > Rather than the additional flag (making for difficult to read calls 
> > like the above defaulttask(true, false)), Can you leave the old signature 
> > and make the one caller invoke defaultConfig(true).setExecutorConfig(null)?

Done.


- Maxim


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22790/#review46236
---


On June 19, 2014, 9:33 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22790/
> ---
> 
> (Updated June 19, 2014, 9:33 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Bill Farner.
> 
> 
> Bugs: AURORA-539
> https://issues.apache.org/jira/browse/AURORA-539
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a getTasksStatusLight RPC to return null ExecutorConfig.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  3616b3e93010c9482dc96d94bdeb42eddae41fb0 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> d115c74eaff2b02c00d3b53b1aa2ff5675436152 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  c93daebf429d8b6691e5e5bcaf88675335f49022 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> 1f1f05e3110b4ba3d83dc7aebc068f4d739e9958 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 255b3d9c881c9291fd3d54792e7bf7385313ed9e 
> 
> Diff: https://reviews.apache.org/r/22790/diff/
> 
> 
> Testing
> ---
> 
> gradle -Pq build
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 22790: Adding a getTasksStatusLight RPC to return null ExecutorConfig.

2014-06-19 Thread Bill Farner 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22790/#review46236
---



src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java


this should be indented past the Function declaration



src/main/thrift/org/apache/aurora/gen/api.thrift


Please include a comment that this is a stopgap until we have a better way 
to reference TaskConfigs so that the client may decide whether to fetch them.



src/main/thrift/org/apache/aurora/gen/api.thrift


I'm not crazy about "light" as it's unclear and open for overloading.  I 
don't have a great suggestion, but even getTasksWithoutConfigs() would be 
somewhat better.



src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java


Rather than the additional flag (making for difficult to read calls like 
the above defaulttask(true, false)), Can you leave the old signature and make 
the one caller invoke defaultConfig(true).setExecutorConfig(null)?


- Bill Farner


On June 19, 2014, 9:33 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22790/
> ---
> 
> (Updated June 19, 2014, 9:33 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Bill Farner.
> 
> 
> Bugs: AURORA-539
> https://issues.apache.org/jira/browse/AURORA-539
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a getTasksStatusLight RPC to return null ExecutorConfig.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  3616b3e93010c9482dc96d94bdeb42eddae41fb0 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> d115c74eaff2b02c00d3b53b1aa2ff5675436152 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  c93daebf429d8b6691e5e5bcaf88675335f49022 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> 1f1f05e3110b4ba3d83dc7aebc068f4d739e9958 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 255b3d9c881c9291fd3d54792e7bf7385313ed9e 
> 
> Diff: https://reviews.apache.org/r/22790/diff/
> 
> 
> Testing
> ---
> 
> gradle -Pq build
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>