Mailing-List: contact reviews-help@aurora.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@aurora.incubator.apache.org
Content-Type: multipart/alternative;
 boundary="===============7535403741208327363=="
MIME-Version: 1.0
Subject: Re: Review Request 31754: Break out API servlet configuration into
 its own module.
From: "Kevin Sweeney" <kevints@apache.org>
To: "Bill Farner" <wfarner@apache.org>,
 "Joshua Cohen" <jcohen@twopensource.com>
Cc: "Aurora" <reviews@aurora.incubator.apache.org>,
 "Kevin Sweeney" <kevints@apache.org>
Date: Thu, 05 Mar 2015 19:51:59 -0000
Message-ID: <20150305195159.12263.62809@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Kevin Sweeney" <noreply@reviews.apache.org>
References: <20150305031836.5896.55783@reviews.apache.org>
In-Reply-To: <20150305031836.5896.55783@reviews.apache.org>
Reply-To: "Kevin Sweeney" <kevints@apache.org>

--===============7535403741208327363==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


> On March 4, 2015, 7:18 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/app/AppModule.java, line 141
> > <https://reviews.apache.org/r/31754/diff/1/?file=885268#file885268line141>
> >
> >     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
> > <https://reviews.apache.org/r/31754/diff/1/?file=885269#file885269line178>
> >
> >     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
> > <https://reviews.apache.org/r/31754/diff/1/?file=885270#file885270line26>
> >
> >     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
> > <https://reviews.apache.org/r/31754/diff/1/?file=885273#file885273line107>
> >
> >     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
> 
>


--===============7535403741208327363==--