Re: Review Request 31754: Break out API servlet configuration into its own module.

2015-03-05 Thread Aurora ReviewBot 

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


This patch does not apply cleanly on master (70494a1), do you need to rebase?

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On March 5, 2015, 8:03 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31754/
> ---
> 
> (Updated March 5, 2015, 8:03 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Break out API servlet configuration into its own module.
> 
> This is necessary to make a follow-up patch introducing HTTP Basic auth 
> testable.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 8a59d89c07b406ce98076ca7ee51b958599a39ec 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerAPIServlet.java 
> 33ad43b3202e5e9ef5be919b6abc5cbc7f62b660 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
> d1ab9b18394ad37fe9dcb131816fcfb2952bf8b6 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java 
> 9804bf5549cd2589fc68c57b3895efd89923169b 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  8ec5f9a3810b4deae981988487c6a46a20db72a2 
>   src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 
> 80beb258d9f2786668d29db85b1295163a402d42 
>   src/test/java/org/apache/aurora/scheduler/http/ServletFilterTest.java 
> 47d54e3c3bb1ba5e0fb26379792f515f25316480 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java 
> 5019094333f9807c64a49c29569ade191ee61824 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/31754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 31754: Break out API servlet configuration into its own module.

2015-03-05 Thread Joshua Cohen 

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

Ship it!


Ship It!

- Joshua Cohen


On March 5, 2015, 8:03 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31754/
> ---
> 
> (Updated March 5, 2015, 8:03 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Break out API servlet configuration into its own module.
> 
> This is necessary to make a follow-up patch introducing HTTP Basic auth 
> testable.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 8a59d89c07b406ce98076ca7ee51b958599a39ec 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerAPIServlet.java 
> 33ad43b3202e5e9ef5be919b6abc5cbc7f62b660 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
> d1ab9b18394ad37fe9dcb131816fcfb2952bf8b6 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java 
> 9804bf5549cd2589fc68c57b3895efd89923169b 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  8ec5f9a3810b4deae981988487c6a46a20db72a2 
>   src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 
> 80beb258d9f2786668d29db85b1295163a402d42 
>   src/test/java/org/apache/aurora/scheduler/http/ServletFilterTest.java 
> 47d54e3c3bb1ba5e0fb26379792f515f25316480 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java 
> 5019094333f9807c64a49c29569ade191ee61824 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/31754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 31754: Break out API servlet configuration into its own module.

2015-03-05 Thread Bill Farner 

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

Ship it!


Ship It!

- Bill Farner


On March 5, 2015, 8:03 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31754/
> ---
> 
> (Updated March 5, 2015, 8:03 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Break out API servlet configuration into its own module.
> 
> This is necessary to make a follow-up patch introducing HTTP Basic auth 
> testable.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 8a59d89c07b406ce98076ca7ee51b958599a39ec 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerAPIServlet.java 
> 33ad43b3202e5e9ef5be919b6abc5cbc7f62b660 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
> d1ab9b18394ad37fe9dcb131816fcfb2952bf8b6 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java 
> 9804bf5549cd2589fc68c57b3895efd89923169b 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  8ec5f9a3810b4deae981988487c6a46a20db72a2 
>   src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 
> 80beb258d9f2786668d29db85b1295163a402d42 
>   src/test/java/org/apache/aurora/scheduler/http/ServletFilterTest.java 
> 47d54e3c3bb1ba5e0fb26379792f515f25316480 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java 
> 5019094333f9807c64a49c29569ade191ee61824 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/31754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 31754: Break out API servlet configuration into its own module.

2015-03-05 Thread Kevin Sweeney 

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

(Updated March 5, 2015, 12:03 p.m.)


Review request for Aurora, Joshua Cohen and Bill Farner.


Changes
---

Fix JettyServerModuleTest to use production=false.


Repository: aurora


Description
---

Break out API servlet configuration into its own module.

This is necessary to make a follow-up patch introducing HTTP Basic auth 
testable.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
8a59d89c07b406ce98076ca7ee51b958599a39ec 
  src/main/java/org/apache/aurora/scheduler/http/SchedulerAPIServlet.java 
33ad43b3202e5e9ef5be919b6abc5cbc7f62b660 
  src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
d1ab9b18394ad37fe9dcb131816fcfb2952bf8b6 
  src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java 
9804bf5549cd2589fc68c57b3895efd89923169b 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
8ec5f9a3810b4deae981988487c6a46a20db72a2 
  src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 
80beb258d9f2786668d29db85b1295163a402d42 
  src/test/java/org/apache/aurora/scheduler/http/ServletFilterTest.java 
47d54e3c3bb1ba5e0fb26379792f515f25316480 
  src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java 
5019094333f9807c64a49c29569ade191ee61824 
  src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java PRE-CREATION 

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


Testing
---


Thanks,

Kevin Sweeney

Re: Review Request 31754: Break out API servlet configuration into its own module.

2015-03-05 Thread Kevin Sweeney 


> On March 4, 2015, 7:18 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/app/AppModule.java, line 141
> > 
> >
> > There seems to be implementation detail leaking here.  Can you make 
> > JettyServerModule hide this away, and force the leak into the unit test 
> > instead?
> 
> Kevin Sweeney wrote:
> Any thoughts on how that should look? Possibly an @VisibleForTesting 
> constructor to JettyServerModule with a boolean production? The test needs to 
> replace the binding for ServletContextListener, which needs a parent Injector 
> handle, since binding a Module isn't allowed due to guice internals.
> 
> Bill Farner wrote:
> +1 to @VisibleForTesting constructor overload.

Done.


> On March 4, 2015, 7:18 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java, line 
> > 178
> > 
> >
> > line break style - arg per line

done.


> On March 4, 2015, 7:18 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/SchedulerAPIServlet.java, 
> > line 26
> > 
> >
> > static final

good catch, done.


> On March 4, 2015, 7:18 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java, 
> > line 107
> > 
> >
> > Do you think 'child module' is sufficiently descriptive?  It threw me 
> > off at first since parent/child already has a meaning in guice, so i 
> > thought there was some real black magic here.
> 
> Kevin Sweeney wrote:
> It is actually a child module in the guice sense (it's a module used to 
> seed the ServletContextListener's child injector). So there is some black 
> magic.

made it `getChildServletModule`.


- Kevin


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


On March 4, 2015, 5:55 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31754/
> ---
> 
> (Updated March 4, 2015, 5:55 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Break out API servlet configuration into its own module.
> 
> This is necessary to make a follow-up patch introducing HTTP Basic auth 
> testable.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 5f6a019e4d6401e1efd075b72c049fa245cc0d0a 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 8a59d89c07b406ce98076ca7ee51b958599a39ec 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerAPIServlet.java 
> 33ad43b3202e5e9ef5be919b6abc5cbc7f62b660 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
> d1ab9b18394ad37fe9dcb131816fcfb2952bf8b6 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 
> 80beb258d9f2786668d29db85b1295163a402d42 
>   src/test/java/org/apache/aurora/scheduler/http/ServletFilterTest.java 
> 47d54e3c3bb1ba5e0fb26379792f515f25316480 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java 
> 5019094333f9807c64a49c29569ade191ee61824 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiTServletTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/31754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 31754: Break out API servlet configuration into its own module.

2015-03-05 Thread Kevin Sweeney 

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

(Updated March 5, 2015, 11:52 a.m.)


Review request for Aurora, Joshua Cohen and Bill Farner.


Changes
---

Addressed review feedback.


Repository: aurora


Description
---

Break out API servlet configuration into its own module.

This is necessary to make a follow-up patch introducing HTTP Basic auth 
testable.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
8a59d89c07b406ce98076ca7ee51b958599a39ec 
  src/main/java/org/apache/aurora/scheduler/http/SchedulerAPIServlet.java 
33ad43b3202e5e9ef5be919b6abc5cbc7f62b660 
  src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
d1ab9b18394ad37fe9dcb131816fcfb2952bf8b6 
  src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java 
9804bf5549cd2589fc68c57b3895efd89923169b 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
8ec5f9a3810b4deae981988487c6a46a20db72a2 
  src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 
80beb258d9f2786668d29db85b1295163a402d42 
  src/test/java/org/apache/aurora/scheduler/http/ServletFilterTest.java 
47d54e3c3bb1ba5e0fb26379792f515f25316480 
  src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java 
5019094333f9807c64a49c29569ade191ee61824 
  src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java PRE-CREATION 

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


Testing
---


Thanks,

Kevin Sweeney

Re: Review Request 31754: Break out API servlet configuration into its own module.

2015-03-05 Thread Kevin Sweeney 


> On March 5, 2015, 10:40 a.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/http/SchedulerAPIServlet.java, 
> > line 25
> > 
> >
> > Any reason not to call this `ApiThriftServlet` to be more explicit?

Done. Also noticed the subclass was unnecessary - changed to Provider.


> On March 5, 2015, 10:40 a.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java, line 57
> > 
> >
> > one arg per line.

Fixed.


> On March 5, 2015, 10:40 a.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/app/AppModule.java, lines 66-81
> > 
> >
> > Is this import move correct? We should treat com.twitter just like any 
> > other com.* import, right?

fixed IDE settings.


> On March 5, 2015, 10:40 a.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java, line 
> > 44
> > 
> >
> > Same here re: import re-ordering.

same.


- Kevin


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


On March 4, 2015, 5:55 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31754/
> ---
> 
> (Updated March 4, 2015, 5:55 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Break out API servlet configuration into its own module.
> 
> This is necessary to make a follow-up patch introducing HTTP Basic auth 
> testable.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 5f6a019e4d6401e1efd075b72c049fa245cc0d0a 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 8a59d89c07b406ce98076ca7ee51b958599a39ec 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerAPIServlet.java 
> 33ad43b3202e5e9ef5be919b6abc5cbc7f62b660 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
> d1ab9b18394ad37fe9dcb131816fcfb2952bf8b6 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 
> 80beb258d9f2786668d29db85b1295163a402d42 
>   src/test/java/org/apache/aurora/scheduler/http/ServletFilterTest.java 
> 47d54e3c3bb1ba5e0fb26379792f515f25316480 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java 
> 5019094333f9807c64a49c29569ade191ee61824 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiTServletTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/31754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 31754: Break out API servlet configuration into its own module.

2015-03-05 Thread Bill Farner 


> On March 5, 2015, 3:18 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/app/AppModule.java, line 141
> > 
> >
> > There seems to be implementation detail leaking here.  Can you make 
> > JettyServerModule hide this away, and force the leak into the unit test 
> > instead?
> 
> Kevin Sweeney wrote:
> Any thoughts on how that should look? Possibly an @VisibleForTesting 
> constructor to JettyServerModule with a boolean production? The test needs to 
> replace the binding for ServletContextListener, which needs a parent Injector 
> handle, since binding a Module isn't allowed due to guice internals.

+1 to @VisibleForTesting constructor overload.


- Bill


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


On March 5, 2015, 1:55 a.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31754/
> ---
> 
> (Updated March 5, 2015, 1:55 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Break out API servlet configuration into its own module.
> 
> This is necessary to make a follow-up patch introducing HTTP Basic auth 
> testable.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 5f6a019e4d6401e1efd075b72c049fa245cc0d0a 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 8a59d89c07b406ce98076ca7ee51b958599a39ec 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerAPIServlet.java 
> 33ad43b3202e5e9ef5be919b6abc5cbc7f62b660 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
> d1ab9b18394ad37fe9dcb131816fcfb2952bf8b6 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 
> 80beb258d9f2786668d29db85b1295163a402d42 
>   src/test/java/org/apache/aurora/scheduler/http/ServletFilterTest.java 
> 47d54e3c3bb1ba5e0fb26379792f515f25316480 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java 
> 5019094333f9807c64a49c29569ade191ee61824 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiTServletTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/31754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 31754: Break out API servlet configuration into its own module.

2015-03-05 Thread Kevin Sweeney 


> On March 4, 2015, 7:18 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java, line 
> > 294
> > 
> >
> > ISTR this being necessary for static asset serving.  Is this no longer 
> > necesary just because of two ServletModules?

No longer needed - we now pass an instance of DefaultServlet to serve() instead 
with different init-params. Changed this elsewhere as well - we only need to 
bind the class form of a servlet or filter if it has an Inject-constructor.


> On March 4, 2015, 7:18 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java, 
> > line 107
> > 
> >
> > Do you think 'child module' is sufficiently descriptive?  It threw me 
> > off at first since parent/child already has a meaning in guice, so i 
> > thought there was some real black magic here.

It is actually a child module in the guice sense (it's a module used to seed 
the ServletContextListener's child injector). So there is some black magic.


> On March 4, 2015, 7:18 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/http/api/ApiTServletTest.java, 
> > lines 42-49
> > 
> >
> > I assume this is a demonstration of how servlet tests should look going 
> > forward - where they provide the specific bindings needed?  If so, i like 
> > it!

Yes, individual servlets can provide just their bindings (plus mock 
dependencies). This is the major purpose of this diff.


- Kevin


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


On March 4, 2015, 5:55 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31754/
> ---
> 
> (Updated March 4, 2015, 5:55 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Break out API servlet configuration into its own module.
> 
> This is necessary to make a follow-up patch introducing HTTP Basic auth 
> testable.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 5f6a019e4d6401e1efd075b72c049fa245cc0d0a 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 8a59d89c07b406ce98076ca7ee51b958599a39ec 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerAPIServlet.java 
> 33ad43b3202e5e9ef5be919b6abc5cbc7f62b660 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
> d1ab9b18394ad37fe9dcb131816fcfb2952bf8b6 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 
> 80beb258d9f2786668d29db85b1295163a402d42 
>   src/test/java/org/apache/aurora/scheduler/http/ServletFilterTest.java 
> 47d54e3c3bb1ba5e0fb26379792f515f25316480 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java 
> 5019094333f9807c64a49c29569ade191ee61824 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiTServletTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/31754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 31754: Break out API servlet configuration into its own module.

2015-03-05 Thread Kevin Sweeney 


> On March 4, 2015, 7:18 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/app/AppModule.java, line 141
> > 
> >
> > There seems to be implementation detail leaking here.  Can you make 
> > JettyServerModule hide this away, and force the leak into the unit test 
> > instead?

Any thoughts on how that should look? Possibly an @VisibleForTesting 
constructor to JettyServerModule with a boolean production? The test needs to 
replace the binding for ServletContextListener, which needs a parent Injector 
handle, since binding a Module isn't allowed due to guice internals.


- Kevin


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


On March 4, 2015, 5:55 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31754/
> ---
> 
> (Updated March 4, 2015, 5:55 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Break out API servlet configuration into its own module.
> 
> This is necessary to make a follow-up patch introducing HTTP Basic auth 
> testable.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 5f6a019e4d6401e1efd075b72c049fa245cc0d0a 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 8a59d89c07b406ce98076ca7ee51b958599a39ec 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerAPIServlet.java 
> 33ad43b3202e5e9ef5be919b6abc5cbc7f62b660 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
> d1ab9b18394ad37fe9dcb131816fcfb2952bf8b6 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 
> 80beb258d9f2786668d29db85b1295163a402d42 
>   src/test/java/org/apache/aurora/scheduler/http/ServletFilterTest.java 
> 47d54e3c3bb1ba5e0fb26379792f515f25316480 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java 
> 5019094333f9807c64a49c29569ade191ee61824 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiTServletTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/31754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 31754: Break out API servlet configuration into its own module.

2015-03-05 Thread Joshua Cohen 


> On March 5, 2015, 1:58 a.m., Kevin Sweeney wrote:
> > Looks like my IDE was a little overzealous applying a different styleguide 
> > - updated diff to follow.

Oh, missed this comment ;).


- Joshua


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


On March 5, 2015, 1:55 a.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31754/
> ---
> 
> (Updated March 5, 2015, 1:55 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Break out API servlet configuration into its own module.
> 
> This is necessary to make a follow-up patch introducing HTTP Basic auth 
> testable.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 5f6a019e4d6401e1efd075b72c049fa245cc0d0a 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 8a59d89c07b406ce98076ca7ee51b958599a39ec 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerAPIServlet.java 
> 33ad43b3202e5e9ef5be919b6abc5cbc7f62b660 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
> d1ab9b18394ad37fe9dcb131816fcfb2952bf8b6 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 
> 80beb258d9f2786668d29db85b1295163a402d42 
>   src/test/java/org/apache/aurora/scheduler/http/ServletFilterTest.java 
> 47d54e3c3bb1ba5e0fb26379792f515f25316480 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java 
> 5019094333f9807c64a49c29569ade191ee61824 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiTServletTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/31754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 31754: Break out API servlet configuration into its own module.

2015-03-05 Thread Joshua Cohen 

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



src/main/java/org/apache/aurora/scheduler/app/AppModule.java


Is this import move correct? We should treat com.twitter just like any 
other com.* import, right?



src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java


Same here re: import re-ordering.



src/main/java/org/apache/aurora/scheduler/http/SchedulerAPIServlet.java


Any reason not to call this `ApiThriftServlet` to be more explicit?



src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java


one arg per line.


- Joshua Cohen


On March 5, 2015, 1:55 a.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31754/
> ---
> 
> (Updated March 5, 2015, 1:55 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Break out API servlet configuration into its own module.
> 
> This is necessary to make a follow-up patch introducing HTTP Basic auth 
> testable.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 5f6a019e4d6401e1efd075b72c049fa245cc0d0a 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 8a59d89c07b406ce98076ca7ee51b958599a39ec 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerAPIServlet.java 
> 33ad43b3202e5e9ef5be919b6abc5cbc7f62b660 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
> d1ab9b18394ad37fe9dcb131816fcfb2952bf8b6 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 
> 80beb258d9f2786668d29db85b1295163a402d42 
>   src/test/java/org/apache/aurora/scheduler/http/ServletFilterTest.java 
> 47d54e3c3bb1ba5e0fb26379792f515f25316480 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java 
> 5019094333f9807c64a49c29569ade191ee61824 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiTServletTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/31754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 31754: Break out API servlet configuration into its own module.

2015-03-04 Thread Bill Farner 

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


Looks great overall, it's nice to see the path towards more sane servlet unit 
tests.  No real ship blockers, but i'll hold on a ship until some of the noise 
is gone from the diff.


src/main/java/org/apache/aurora/scheduler/app/AppModule.java


There seems to be implementation detail leaking here.  Can you make 
JettyServerModule hide this away, and force the leak into the unit test instead?



src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java


line break style - arg per line



src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java


ISTR this being necessary for static asset serving.  Is this no longer 
necesary just because of two ServletModules?



src/main/java/org/apache/aurora/scheduler/http/SchedulerAPIServlet.java


static final



src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java


Do you think 'child module' is sufficiently descriptive?  It threw me off 
at first since parent/child already has a meaning in guice, so i thought there 
was some real black magic here.



src/test/java/org/apache/aurora/scheduler/http/api/ApiTServletTest.java


I assume this is a demonstration of how servlet tests should look going 
forward - where they provide the specific bindings needed?  If so, i like it!


- Bill Farner


On March 5, 2015, 1:55 a.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31754/
> ---
> 
> (Updated March 5, 2015, 1:55 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Break out API servlet configuration into its own module.
> 
> This is necessary to make a follow-up patch introducing HTTP Basic auth 
> testable.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 5f6a019e4d6401e1efd075b72c049fa245cc0d0a 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 8a59d89c07b406ce98076ca7ee51b958599a39ec 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerAPIServlet.java 
> 33ad43b3202e5e9ef5be919b6abc5cbc7f62b660 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
> d1ab9b18394ad37fe9dcb131816fcfb2952bf8b6 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 
> 80beb258d9f2786668d29db85b1295163a402d42 
>   src/test/java/org/apache/aurora/scheduler/http/ServletFilterTest.java 
> 47d54e3c3bb1ba5e0fb26379792f515f25316480 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java 
> 5019094333f9807c64a49c29569ade191ee61824 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiTServletTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/31754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 31754: Break out API servlet configuration into its own module.

2015-03-04 Thread Kevin Sweeney 

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


Looks like my IDE was a little overzealous applying a different styleguide - 
updated diff to follow.

- Kevin Sweeney


On March 4, 2015, 5:55 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31754/
> ---
> 
> (Updated March 4, 2015, 5:55 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Break out API servlet configuration into its own module.
> 
> This is necessary to make a follow-up patch introducing HTTP Basic auth 
> testable.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 5f6a019e4d6401e1efd075b72c049fa245cc0d0a 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 8a59d89c07b406ce98076ca7ee51b958599a39ec 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerAPIServlet.java 
> 33ad43b3202e5e9ef5be919b6abc5cbc7f62b660 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
> d1ab9b18394ad37fe9dcb131816fcfb2952bf8b6 
>   src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java 
> 80beb258d9f2786668d29db85b1295163a402d42 
>   src/test/java/org/apache/aurora/scheduler/http/ServletFilterTest.java 
> 47d54e3c3bb1ba5e0fb26379792f515f25316480 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java 
> 5019094333f9807c64a49c29569ade191ee61824 
>   src/test/java/org/apache/aurora/scheduler/http/api/ApiTServletTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/31754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>