Mailing-List: contact reviews-help@aurora.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@aurora.incubator.apache.org
Content-Type: multipart/alternative;
 boundary="===============3763375339095791485=="
MIME-Version: 1.0
Subject: Re: Review Request 23247: Refactoring out SchedulerCore in favor of
 SchedulerThriftInterface.
From: "Bill Farner" <wfarner@apache.org>
To: "Bill Farner" <wfarner@apache.org>, "Kevin Sweeney" <kevints@apache.org>
Cc: "Aurora" <reviews@aurora.incubator.apache.org>,
 "Maxim Khutornenko" <maxim@apache.org>
Date: Wed, 02 Jul 2014 23:24:27 -0000
Message-ID: <20140702232427.5195.3420@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Bill Farner" <noreply@reviews.apache.org>
References: <20140702230815.5196.83684@reviews.apache.org>
In-Reply-To: <20140702230815.5196.83684@reviews.apache.org>
Reply-To: "Bill Farner" <wfarner@apache.org>

--===============3763375339095791485==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


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


--===============3763375339095791485==--