> On July 2, 2014, 11:08 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/http/ServletModule.java, lines 
> > 79-89
> > <https://reviews.apache.org/r/23247/diff/1/?file=623081#file623081line79>
> >
> >     requireBinding is a DRY violation IMO - any @Inject-annotated 
> > constructor will require bindings for all of its arguments. I'd like to 
> > push for removing its use rather than the current piecemeal approach.
> 
> Maxim Khutornenko wrote:
>     I was split between removing and updating it and decided in favor of the 
> latter. I'd be happy to kill it though if everyone agrees.
> 
> Kevin Sweeney wrote:
>     I haven't seen it add value yet (unless we wind up doing something like 
> guaranteeing the presence of bindings for plugins to use that the main code 
> doesn't use itself). Otherwise it just confuses your IDE into thinking a 
> class is used that really isn't.

There are 2 main benefits to requireBinding: documentation in the module, and 
early failure.  If a binding requirement fails, you can get an injector, which 
means the application can abort much earlier.  The risk of not doing this is 
that your application could start doing work, only to fail late; or you could 
request injection from a non-main thread, and not initiate application 
shutdown.  In practice, i don't think we're vulnerable to either of these 
(through design and best practices).  I do feel that requireBinding is of 
dubious value if it's not used anywhere.  In fact, i wish it was mandatory.  
Lacking that, i'm okay to kill it.


- Bill


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


On July 2, 2014, 9:46 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23247/
> -----------------------------------------------------------
> 
> (Updated July 2, 2014, 9:46 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-94
>     https://issues.apache.org/jira/browse/AURORA-94
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Moving all SchedulerCore logic into SchedulerThriftInterface.
> 
> Unit tests in BaseSchedulerCoreImplTest.java:
> - redundant/duplicate tests are dropped (e.g. state machine-related tests);
> - unique unit tests not directly related to SchedulerCore are moved into 
> StorageBackfillTest(new), StateManagerImplTest and ConfigurationManagerTest;
> - core rpc-related tests are replicated in SchedulerThriftInterfaceTest.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/base/ScheduleException.java 
> e060e5ec88a0ab311415eaa638cf693c99c40049 
>   src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 
> 27599f75603542069084631baf9195b8ad75e902 
>   src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 
> 8befecaf4f13c0b890b452bc7b9c0618725b8c41 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCore.java 
> 628464de0032db4eb5884e3b74346b2390408993 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
> b6d06f39ac39570eac4495d97d3adaabe8357bf7 
>   src/main/java/org/apache/aurora/scheduler/state/StateModule.java 
> 2c712eff097c3334bfcf2559a52214367748d08a 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  2549dd33d08dfc6058d985127a3f0c1f3984eaa7 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  2298971ffac45a284f9130e2122aeea8b39dc852 
>   
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java
>  d2bd65eef5a8ceea92dd62044e5e2c621c4a4f3e 
>   src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 
> 94eb0a22037187625636b24707d4ac6741b55f22 
>   src/test/java/org/apache/aurora/scheduler/storage/StorageBackfillTest.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/storage/mem/MemStorageSchedulerCoreImplTest.java
>  35bed104d838596abcbb5abd5cad29592b384dfa 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  2cffa74ba36e2afda3340658d6b1afd6cb50cf2c 
>   src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 
> 2562ff944b7cb304ce5a60d3f74beee22f6cc7bc 
> 
> Diff: https://reviews.apache.org/r/23247/diff/
> 
> 
> Testing
> -------
> 
> gradle -Pq clean build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Reply via email to