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