You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2014/07/02 23:46:48 UTC
Review Request 23247: Refactoring out SchedulerCore in favor of
SchedulerThriftInterface.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23247/
-----------------------------------------------------------
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
Re: Review Request 23247: Refactoring out SchedulerCore in favor of
SchedulerThriftInterface.
Posted by Bill Farner <wf...@apache.org>.
> On July 2, 2014, 10:01 p.m., Bill Farner wrote:
> > How would you feel about doing this more piecemeal? This is changing a lot of important code, making it more likely that we'll miss something and introduce bugs (this is made more likely since tests are changing a lot too). Would it be possible to pull out one method of SchedulerCoreImpl at a time? Or maybe start by repurposing it as a helper object to SchedulerThriftInterface, rather than something that passes calls through to other components?
>
> Maxim Khutornenko wrote:
> What, really? :( I was so happy to finally get rid of that code and now I have to look at it again...
Sorry about that, i didn't realize such an overhaul was underway. I'm happy to hear other opinions, but i'm not convinced i can give a reliable code review given the amount of change here. At the very least, it would be easier if there were minimal simultaneous changes to logic and tests.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23247/#review47238
-----------------------------------------------------------
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
>
>
Re: Review Request 23247: Refactoring out SchedulerCore in favor of
SchedulerThriftInterface.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On July 2, 2014, 10:01 p.m., Bill Farner wrote:
> > How would you feel about doing this more piecemeal? This is changing a lot of important code, making it more likely that we'll miss something and introduce bugs (this is made more likely since tests are changing a lot too). Would it be possible to pull out one method of SchedulerCoreImpl at a time? Or maybe start by repurposing it as a helper object to SchedulerThriftInterface, rather than something that passes calls through to other components?
What, really? :( I was so happy to finally get rid of that code and now I have to look at it again...
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23247/#review47238
-----------------------------------------------------------
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
>
>
Re: Review Request 23247: Refactoring out SchedulerCore in favor of
SchedulerThriftInterface.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23247/#review47238
-----------------------------------------------------------
How would you feel about doing this more piecemeal? This is changing a lot of important code, making it more likely that we'll miss something and introduce bugs (this is made more likely since tests are changing a lot too). Would it be possible to pull out one method of SchedulerCoreImpl at a time? Or maybe start by repurposing it as a helper object to SchedulerThriftInterface, rather than something that passes calls through to other components?
- Bill Farner
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
>
>
Re: Review Request 23247: Refactoring out SchedulerCore in favor of
SchedulerThriftInterface.
Posted by Bill Farner <wf...@apache.org>.
> 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
>
>
Re: Review Request 23247: Refactoring out SchedulerCore in favor of
SchedulerThriftInterface.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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.
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.
- Maxim
-----------------------------------------------------------
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
>
>
Re: Review Request 23247: Refactoring out SchedulerCore in favor of
SchedulerThriftInterface.
Posted by Kevin Sweeney <ke...@apache.org>.
> 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
>
>
Re: Review Request 23247: Refactoring out SchedulerCore in favor of
SchedulerThriftInterface.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23247/#review47243
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/http/ServletModule.java
<https://reviews.apache.org/r/23247/#comment82890>
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.
- Kevin Sweeney
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
>
>