Re: Review Request 16873: Refactor StateManagerImpl and TaskStateMachine for less code and better readability.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16873/#review33054 --- Kevin — ping? Presumably you can review while i fix up Maxim's non-ship-blocks. - Bill Farner On Jan. 14, 2014, 11:27 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16873/ --- (Updated Jan. 14, 2014, 11:27 p.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Repository: aurora Description --- Sorry for the monster diff. The intertwined nature of StateManagerImpl and TaskStateMachine made it pretty challenging to do this partially. I hope the end result is much easier to comprehend. The big picture for this change is that the closures inside TaskStateMachine no longer drop items onto a work queue that feeds back into StateManagerImpl. Instead, it returns these actions in a TransitionResult. I intend to improve this further in the future by exposing only a helper function in TaskStateMachine, to guarantee the one-time-use semantic. Diffs - src/main/java/org/apache/aurora/scheduler/state/SideEffect.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/state/SideEffectStorage.java 2bdd4591f2182fe6c44d46f778be562d30eb2392 src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 2b8ca095d8f108d516a43af8de4ff451ed9a8924 src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java 11d283d31883b865b224d5af169dd5c42875021d src/main/java/org/apache/aurora/scheduler/state/WorkCommand.java aff74d535eb1237beafbcdf936d5ccc7101377c9 src/main/java/org/apache/aurora/scheduler/storage/Storage.java 79f56052a25ba756208e747dc5d198f30f0c4900 src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 7fe297a37aee0cb1be495e6a568b66271ee7bc3d src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 53793000de08fe80c0334241d332e3a50fca222a src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java f44ee589430c2d4c0e014a705fd24b1f2fe08f36 Diff: https://reviews.apache.org/r/16873/diff/ Testing --- ./gradlew build Thanks, Bill Farner
Re: Review Request 16873: Refactor StateManagerImpl and TaskStateMachine for less code and better readability.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16873/#review32551 --- Ship it! src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java https://reviews.apache.org/r/16873/#comment61431 This seems like a big enough gotcha to warrant filing a ticket. - Kevin Sweeney On Jan. 14, 2014, 3:27 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16873/ --- (Updated Jan. 14, 2014, 3:27 p.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Repository: aurora Description --- Sorry for the monster diff. The intertwined nature of StateManagerImpl and TaskStateMachine made it pretty challenging to do this partially. I hope the end result is much easier to comprehend. The big picture for this change is that the closures inside TaskStateMachine no longer drop items onto a work queue that feeds back into StateManagerImpl. Instead, it returns these actions in a TransitionResult. I intend to improve this further in the future by exposing only a helper function in TaskStateMachine, to guarantee the one-time-use semantic. Diffs - src/main/java/org/apache/aurora/scheduler/state/SideEffect.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/state/SideEffectStorage.java 2bdd4591f2182fe6c44d46f778be562d30eb2392 src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 2b8ca095d8f108d516a43af8de4ff451ed9a8924 src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java 11d283d31883b865b224d5af169dd5c42875021d src/main/java/org/apache/aurora/scheduler/state/WorkCommand.java aff74d535eb1237beafbcdf936d5ccc7101377c9 src/main/java/org/apache/aurora/scheduler/storage/Storage.java 79f56052a25ba756208e747dc5d198f30f0c4900 src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 7fe297a37aee0cb1be495e6a568b66271ee7bc3d src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 53793000de08fe80c0334241d332e3a50fca222a src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java f44ee589430c2d4c0e014a705fd24b1f2fe08f36 Diff: https://reviews.apache.org/r/16873/diff/ Testing --- ./gradlew build Thanks, Bill Farner
Re: Review Request 16873: Refactor StateManagerImpl and TaskStateMachine for less code and better readability.
On Jan. 22, 2014, 8:43 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/state/SideEffect.java, line 42 https://reviews.apache.org/r/16873/diff/1/?file=423020#file423020line42 Missing comments for public methods in this class? We typically don't bother including a javadoc for one-liner methods. If you feel strongly about this, you might want to push for the checkstyle config to require it. On Jan. 22, 2014, 8:43 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java, line 371 https://reviews.apache.org/r/16873/diff/1/?file=423022#file423022line371 s/SideEffect.Action/Action Thanks, fixed. On Jan. 22, 2014, 8:43 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java, line 384 https://reviews.apache.org/r/16873/diff/1/?file=423022#file423022line384 \n Done. On Jan. 22, 2014, 8:43 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java, line 439 https://reviews.apache.org/r/16873/diff/1/?file=423022#file423022line439 Might be more consistent and easier to follow if case statements are ordered by ACTIONS_IN_ORDER. Sounds good, done. On Jan. 22, 2014, 8:43 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java, line 223 https://reviews.apache.org/r/16873/diff/1/?file=423023#file423023line223 It's hard to follow the logic here. Are both of these conditions desired here? false == false true == true Mind splitting it into two statements and adding proper error messages for each? It's more than two statements, but i believe the desired effect is achieved in the new diff. On Jan. 22, 2014, 8:43 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java, line 553 https://reviews.apache.org/r/16873/diff/1/?file=423023#file423023line553 Better name for a parameter here to avoid confusion with the overload? Done. On Jan. 22, 2014, 8:43 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java, line 566 https://reviews.apache.org/r/16873/diff/1/?file=423023#file423023line566 Since it's fully public, might make sense moving it into a separate file? Done. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16873/#review32511 --- On Jan. 14, 2014, 11:27 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16873/ --- (Updated Jan. 14, 2014, 11:27 p.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Repository: aurora Description --- Sorry for the monster diff. The intertwined nature of StateManagerImpl and TaskStateMachine made it pretty challenging to do this partially. I hope the end result is much easier to comprehend. The big picture for this change is that the closures inside TaskStateMachine no longer drop items onto a work queue that feeds back into StateManagerImpl. Instead, it returns these actions in a TransitionResult. I intend to improve this further in the future by exposing only a helper function in TaskStateMachine, to guarantee the one-time-use semantic. Diffs - src/main/java/org/apache/aurora/scheduler/state/SideEffect.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/state/SideEffectStorage.java 2bdd4591f2182fe6c44d46f778be562d30eb2392 src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 2b8ca095d8f108d516a43af8de4ff451ed9a8924 src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java 11d283d31883b865b224d5af169dd5c42875021d src/main/java/org/apache/aurora/scheduler/state/WorkCommand.java aff74d535eb1237beafbcdf936d5ccc7101377c9 src/main/java/org/apache/aurora/scheduler/storage/Storage.java 79f56052a25ba756208e747dc5d198f30f0c4900 src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 7fe297a37aee0cb1be495e6a568b66271ee7bc3d src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 53793000de08fe80c0334241d332e3a50fca222a src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java f44ee589430c2d4c0e014a705fd24b1f2fe08f36 Diff: https://reviews.apache.org/r/16873/diff/ Testing --- ./gradlew build Thanks, Bill Farner
Re: Review Request 16873: Refactor StateManagerImpl and TaskStateMachine for less code and better readability.
On Jan. 28, 2014, 9:50 p.m., Kevin Sweeney wrote: src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java, lines 543-546 https://reviews.apache.org/r/16873/diff/1/?file=423022#file423022line543 This seems like a big enough gotcha to warrant filing a ticket. AURORA-138 - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16873/#review32551 --- On Jan. 14, 2014, 11:27 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16873/ --- (Updated Jan. 14, 2014, 11:27 p.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Repository: aurora Description --- Sorry for the monster diff. The intertwined nature of StateManagerImpl and TaskStateMachine made it pretty challenging to do this partially. I hope the end result is much easier to comprehend. The big picture for this change is that the closures inside TaskStateMachine no longer drop items onto a work queue that feeds back into StateManagerImpl. Instead, it returns these actions in a TransitionResult. I intend to improve this further in the future by exposing only a helper function in TaskStateMachine, to guarantee the one-time-use semantic. Diffs - src/main/java/org/apache/aurora/scheduler/state/SideEffect.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/state/SideEffectStorage.java 2bdd4591f2182fe6c44d46f778be562d30eb2392 src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 2b8ca095d8f108d516a43af8de4ff451ed9a8924 src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java 11d283d31883b865b224d5af169dd5c42875021d src/main/java/org/apache/aurora/scheduler/state/WorkCommand.java aff74d535eb1237beafbcdf936d5ccc7101377c9 src/main/java/org/apache/aurora/scheduler/storage/Storage.java 79f56052a25ba756208e747dc5d198f30f0c4900 src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 7fe297a37aee0cb1be495e6a568b66271ee7bc3d src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 53793000de08fe80c0334241d332e3a50fca222a src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java f44ee589430c2d4c0e014a705fd24b1f2fe08f36 Diff: https://reviews.apache.org/r/16873/diff/ Testing --- ./gradlew build Thanks, Bill Farner
Re: Review Request 16873: Refactor StateManagerImpl and TaskStateMachine for less code and better readability.
On Jan. 22, 2014, 8:43 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/state/SideEffect.java, line 77 https://reviews.apache.org/r/16873/diff/1/?file=423020#file423020line77 Should these be javadoc comments instead? Good call, done. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16873/#review32511 --- On Jan. 14, 2014, 11:27 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16873/ --- (Updated Jan. 14, 2014, 11:27 p.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Repository: aurora Description --- Sorry for the monster diff. The intertwined nature of StateManagerImpl and TaskStateMachine made it pretty challenging to do this partially. I hope the end result is much easier to comprehend. The big picture for this change is that the closures inside TaskStateMachine no longer drop items onto a work queue that feeds back into StateManagerImpl. Instead, it returns these actions in a TransitionResult. I intend to improve this further in the future by exposing only a helper function in TaskStateMachine, to guarantee the one-time-use semantic. Diffs - src/main/java/org/apache/aurora/scheduler/state/SideEffect.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/state/SideEffectStorage.java 2bdd4591f2182fe6c44d46f778be562d30eb2392 src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 2b8ca095d8f108d516a43af8de4ff451ed9a8924 src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java 11d283d31883b865b224d5af169dd5c42875021d src/main/java/org/apache/aurora/scheduler/state/WorkCommand.java aff74d535eb1237beafbcdf936d5ccc7101377c9 src/main/java/org/apache/aurora/scheduler/storage/Storage.java 79f56052a25ba756208e747dc5d198f30f0c4900 src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 7fe297a37aee0cb1be495e6a568b66271ee7bc3d src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 53793000de08fe80c0334241d332e3a50fca222a src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java f44ee589430c2d4c0e014a705fd24b1f2fe08f36 Diff: https://reviews.apache.org/r/16873/diff/ Testing --- ./gradlew build Thanks, Bill Farner
Re: Review Request 16873: Refactor StateManagerImpl and TaskStateMachine for less code and better readability.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16873/ --- (Updated Jan. 29, 2014, 12:09 a.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Repository: aurora Description --- Sorry for the monster diff. The intertwined nature of StateManagerImpl and TaskStateMachine made it pretty challenging to do this partially. I hope the end result is much easier to comprehend. The big picture for this change is that the closures inside TaskStateMachine no longer drop items onto a work queue that feeds back into StateManagerImpl. Instead, it returns these actions in a TransitionResult. I intend to improve this further in the future by exposing only a helper function in TaskStateMachine, to guarantee the one-time-use semantic. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/state/SideEffect.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/state/SideEffectStorage.java 2bdd4591f2182fe6c44d46f778be562d30eb2392 src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 2b8ca095d8f108d516a43af8de4ff451ed9a8924 src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java 11d283d31883b865b224d5af169dd5c42875021d src/main/java/org/apache/aurora/scheduler/state/TransitionResult.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/state/WorkCommand.java aff74d535eb1237beafbcdf936d5ccc7101377c9 src/main/java/org/apache/aurora/scheduler/storage/Storage.java 53d0c852e174557a5ffbe4d48175803ab98546a4 src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 7fe297a37aee0cb1be495e6a568b66271ee7bc3d src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 53793000de08fe80c0334241d332e3a50fca222a src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java f44ee589430c2d4c0e014a705fd24b1f2fe08f36 Diff: https://reviews.apache.org/r/16873/diff/ Testing --- ./gradlew build Thanks, Bill Farner
Re: Review Request 16873: Refactor StateManagerImpl and TaskStateMachine for less code and better readability.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16873/#review32502 --- Ping - Bill Farner On Jan. 14, 2014, 11:27 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16873/ --- (Updated Jan. 14, 2014, 11:27 p.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Repository: aurora Description --- Sorry for the monster diff. The intertwined nature of StateManagerImpl and TaskStateMachine made it pretty challenging to do this partially. I hope the end result is much easier to comprehend. The big picture for this change is that the closures inside TaskStateMachine no longer drop items onto a work queue that feeds back into StateManagerImpl. Instead, it returns these actions in a TransitionResult. I intend to improve this further in the future by exposing only a helper function in TaskStateMachine, to guarantee the one-time-use semantic. Diffs - src/main/java/org/apache/aurora/scheduler/state/SideEffect.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/state/SideEffectStorage.java 2bdd4591f2182fe6c44d46f778be562d30eb2392 src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 2b8ca095d8f108d516a43af8de4ff451ed9a8924 src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java 11d283d31883b865b224d5af169dd5c42875021d src/main/java/org/apache/aurora/scheduler/state/WorkCommand.java aff74d535eb1237beafbcdf936d5ccc7101377c9 src/main/java/org/apache/aurora/scheduler/storage/Storage.java 79f56052a25ba756208e747dc5d198f30f0c4900 src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 7fe297a37aee0cb1be495e6a568b66271ee7bc3d src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 53793000de08fe80c0334241d332e3a50fca222a src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java f44ee589430c2d4c0e014a705fd24b1f2fe08f36 Diff: https://reviews.apache.org/r/16873/diff/ Testing --- ./gradlew build Thanks, Bill Farner
