aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kevin Sweeney" <kevi...@apache.org>
Subject Re: Review Request 23247: Refactoring out SchedulerCore in favor of SchedulerThriftInterface.
Date Wed, 02 Jul 2014 23:17:41 GMT


> On July 2, 2014, 4: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.

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.


- Kevin


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


On July 2, 2014, 2: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, 2: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
> 
>


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