aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Farner" <wfar...@apache.org>
Subject Re: Review Request 31754: Break out API servlet configuration into its own module.
Date Thu, 05 Mar 2015 03:18:36 GMT

-----------------------------------------------------------
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
<https://reviews.apache.org/r/31754/#comment122261>

    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
<https://reviews.apache.org/r/31754/#comment122258>

    line break style - arg per line



src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java
<https://reviews.apache.org/r/31754/#comment122257>

    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
<https://reviews.apache.org/r/31754/#comment122256>

    static final



src/test/java/org/apache/aurora/scheduler/http/JettyServerModuleTest.java
<https://reviews.apache.org/r/31754/#comment122262>

    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
<https://reviews.apache.org/r/31754/#comment122263>

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message