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.
--- 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.
--- 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
Re: Review Request 23247: Refactoring out SchedulerCore in favor of SchedulerThriftInterface.
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.
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.
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
