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 32806: Remove use of LocalServiceRegistry, simplify plumbing of HTTP address.
Date Fri, 03 Apr 2015 22:40:05 GMT


> On April 3, 2015, 5:15 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/http/HttpService.java, line 21
> > <https://reviews.apache.org/r/32806/diff/2/?file=914532#file914532line21>
> >
> >     This seems like it's a Qualifier for HostAndPort rather than an interface. As
an interface this appears to fall into the pseudo-typedef antipattern: http://www.ibm.com/developerworks/library/j-jtp02216/.
> 
> Bill Farner wrote:
>     I could drop this for `Provider<HostAndPort>` to a similar end, but i find
back-tracking to concrete implementations significantly harder when bindings are made against
vague interfaces.  Can you see a path forward that doesn't make it difficult to map from consumer
to implementer?
> 
> Kevin Sweeney wrote:
>     I don't find the path for Qualifier implementaitons hard to find
>     
>     Injection sites will typically have
>     
>     ```java
>     @Inject
>     InjectionSite(@Http HostAndPort httpHostAndPort) {
>     }
>     ```
>     
>     Find Usages on Http will lead to a section "usages in .class" which will lead to
something like
>     
>     ```java
>     class ConfigModule extends AbstractModule {
>        @Override
>        protected void configure() {
>          bind(HostAndPort.class).annotatedWith(Http.class).toInstance(HostAndPort.createFrom("127.0.0.1:80"));
>        }
>     }
>     ```
>     
>     or a section Annotation that will lead to
>     
>     ```java
>     class ConfigModule extends AbstractModule {
>       @Override protected void configure() {}
>     
>       @Provides
>       @Http
>       HostAndPort provideHttpHostAndPort() {
>         //...
>       }
>     }
>     ```
>     
>     and the declaration of the qualifier itself has information about what makes the
thing special in its javadoc
>     
>     ```java
>     /**
>      * The main HTTP port.
>      */
>     @Qualifier
>     @Retention(RUNTIME)
>     @Target({FIELD, METHOD, PARAMETER})
>     @Documented
>     public @interface Http {}
>     ```

Right, i'm familiar with the moving parts involved.  One detail not addressed in your comment
above is that this must be a `Provider`, since the port number is not known during binding
(it could be an ephemeral port, and `== 0` at binding time).  IMHO this is the point where
implementing `Provider` in `HttpServerLauncher` and introducing a binding annotation seemed
worse than the custom interface.


- Bill


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


On April 3, 2015, 3:06 a.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32806/
> -----------------------------------------------------------
> 
> (Updated April 3, 2015, 3:06 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Kevin Sweeney.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Remove use of LocalServiceRegistry, simplify plumbing of HTTP address.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 5f6a019e4d6401e1efd075b72c049fa245cc0d0a

>   src/main/java/org/apache/aurora/scheduler/app/LocalServiceRegistryWithOverrides.java
6d92ae3c8ec46e7964e81e507a2f2a7f2db68cfd 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 2af009d3d9ec44a70659225d0c18de9fda3a6f7a

>   src/main/java/org/apache/aurora/scheduler/http/HttpService.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 50f377587ac05dbb72063ea02502e6d980148d3e

>   src/main/java/org/apache/aurora/scheduler/http/LeaderRedirect.java e03009c12de5a09761c1f444c6439ef3144b0b17

>   src/test/java/org/apache/aurora/scheduler/app/LocalServiceRegistryWithOverridesTest.java
21fd027976f75acc427c6d9265a7c7a91895d53d 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java d212bbdf3c37be8e1e00eb169cf40b90fe69ed5f

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

>   src/test/java/org/apache/aurora/scheduler/http/LeaderRedirectTest.java 7f80757cb40af7dde042f1d39355eadf2b3b1aee

>   src/test/java/org/apache/aurora/scheduler/http/ServletFilterTest.java c5c5f789de6bf7693520081d0c1acc5165603242

>   src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java 76cb691e6d7d4fada3a18fde73aceed7039bcaa4

> 
> Diff: https://reviews.apache.org/r/32806/diff/
> 
> 
> Testing
> -------
> 
> Test suite + end-to-end tests.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>


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