Re: Review Request 17153: Implement restart command for client v2.
On Jan. 21, 2014, 9:16 p.m., Jonathan Boulle wrote: Mark - want to put some time in my calendar tomorrow to sit down together and go through all the outstanding reviews to get them merged? The patches are in a bit of a confusing state, every time I look at a review it seems like it's reverting something inadvertently :-( I've been trying, as much as possible, to make the changes independent, against master, to make them easier to review. (Past experience with trying to pile changes with dependency relationships has taught me that I'm not good at keeping the dependency relations in my head!) So I'd suggest not to worry too much about seeming regressions; these changes were sent for review before other changes were pushed to master. If I were in SF, getting together to merge into one big change would work, but I've never had a good experience doing that over skype. Just too hard to coordinate! I'm happy to push all of these into one big change on my own if Brian (the other reviewer) agrees that would make it easier. - Mark --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17153/#review32454 --- On Jan. 21, 2014, 5:03 p.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17153/ --- (Updated Jan. 21, 2014, 5:03 p.m.) Review request for Aurora, Jonathan Boulle and Brian Wickman. Bugs: aurora-54 https://issues.apache.org/jira/browse/aurora-54 Repository: aurora Description --- Implement restart command for client v2. Diffs - src/main/python/apache/aurora/client/cli/context.py 78e54a2233b825ab057a1fb2397c91b50ad1aec0 src/main/python/apache/aurora/client/cli/jobs.py f60d7e9fa5367f43829957e3485e43e76c84bf48 src/main/python/apache/aurora/client/cli/options.py 1b7155409505b46451df072edd196dd7e4c88f1c src/test/python/apache/aurora/client/cli/BUILD f9ebe0cf626a040aa67654faea07b8902e558282 src/test/python/apache/aurora/client/cli/test_restart.py PRE-CREATION src/test/python/apache/aurora/client/cli/test_update.py PRE-CREATION Diff: https://reviews.apache.org/r/17153/diff/ Testing --- [sun-wukong aurora (restart)]$ ./pants src/test/python/apache/aurora/client/cli:all Build operating on targets: OrderedSet([PythonTestSuite(src/test/python/apache/aurora/client/cli/BUILD:all)]) == test session starts == platform darwin -- Python 2.7.2 -- pytest-2.5.1 collected 23 items src/test/python/apache/aurora/client/cli/test_create.py src/test/python/apache/aurora/client/cli/test_kill.py . src/test/python/apache/aurora/client/cli/test_status.py . src/test/python/apache/aurora/client/cli/test_update.py ... src/test/python/apache/aurora/client/cli/test_diff.py ... src/test/python/apache/aurora/client/cli/test_restart.py ... === 23 passed in 1.39 seconds === src.test.python.apache.aurora.client.cli.job . SUCCESS [sun-wukong aurora (restart)]$ Thanks, Mark Chu-Carroll
Re: Review Request 17153: Implement restart command for client v2.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17153/#review32493 --- Superseded by https://reviews.apache.org/r/17184/ - Mark Chu-Carroll On Jan. 21, 2014, 5:03 p.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17153/ --- (Updated Jan. 21, 2014, 5:03 p.m.) Review request for Aurora, Jonathan Boulle and Brian Wickman. Bugs: aurora-54 https://issues.apache.org/jira/browse/aurora-54 Repository: aurora Description --- Implement restart command for client v2. Diffs - src/main/python/apache/aurora/client/cli/context.py 78e54a2233b825ab057a1fb2397c91b50ad1aec0 src/main/python/apache/aurora/client/cli/jobs.py f60d7e9fa5367f43829957e3485e43e76c84bf48 src/main/python/apache/aurora/client/cli/options.py 1b7155409505b46451df072edd196dd7e4c88f1c src/test/python/apache/aurora/client/cli/BUILD f9ebe0cf626a040aa67654faea07b8902e558282 src/test/python/apache/aurora/client/cli/test_restart.py PRE-CREATION src/test/python/apache/aurora/client/cli/test_update.py PRE-CREATION Diff: https://reviews.apache.org/r/17153/diff/ Testing --- [sun-wukong aurora (restart)]$ ./pants src/test/python/apache/aurora/client/cli:all Build operating on targets: OrderedSet([PythonTestSuite(src/test/python/apache/aurora/client/cli/BUILD:all)]) == test session starts == platform darwin -- Python 2.7.2 -- pytest-2.5.1 collected 23 items src/test/python/apache/aurora/client/cli/test_create.py src/test/python/apache/aurora/client/cli/test_kill.py . src/test/python/apache/aurora/client/cli/test_status.py . src/test/python/apache/aurora/client/cli/test_update.py ... src/test/python/apache/aurora/client/cli/test_diff.py ... src/test/python/apache/aurora/client/cli/test_restart.py ... === 23 passed in 1.39 seconds === src.test.python.apache.aurora.client.cli.job . SUCCESS [sun-wukong aurora (restart)]$ Thanks, Mark Chu-Carroll
Review Request 17185: Merged all of the open clientv2 reviews into one unified change.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17185/ --- Review request for Aurora, Jonathan Boulle and Brian Wickman. Bugs: aurora-53 and aurora-54 https://issues.apache.org/jira/browse/aurora-53 https://issues.apache.org/jira/browse/aurora-54 Repository: aurora Description --- Merged all of the open clientv2 reviews into one unified change. - Add clientv2 implementation of job status with jobkey wildcard support. - Add clientv2 implementations of job update and job list - Add clientv2 implementation of job restart - Add clientv2 implementation of job cancel_update - Extract more common options into options.py. - General code cleanup. This supersedes the following reviews: - https://reviews.apache.org/r/17153/ - https://reviews.apache.org/r/17051/ - https://reviews.apache.org/r/17154/ - https://reviews.apache.org/r/16306/ Diffs - src/main/python/apache/aurora/client/cli/context.py 78e54a2233b825ab057a1fb2397c91b50ad1aec0 src/main/python/apache/aurora/client/cli/jobs.py f60d7e9fa5367f43829957e3485e43e76c84bf48 src/main/python/apache/aurora/client/cli/options.py 1b7155409505b46451df072edd196dd7e4c88f1c src/main/python/apache/aurora/client/commands/core.py 13657c4827072c4f67e0b05034e575616f02338b src/test/python/apache/aurora/client/cli/BUILD f9ebe0cf626a040aa67654faea07b8902e558282 src/test/python/apache/aurora/client/cli/test_cancel_update.py PRE-CREATION src/test/python/apache/aurora/client/cli/test_create.py 64eb51be32f33de7d67962ff9300e64820a37baf src/test/python/apache/aurora/client/cli/test_kill.py 714d5fbeebaaba6cede438c40b3b370d0ee99934 src/test/python/apache/aurora/client/cli/test_restart.py PRE-CREATION src/test/python/apache/aurora/client/cli/test_status.py efcf164682a56294863a2aec916b9382a50032b7 src/test/python/apache/aurora/client/cli/test_update.py PRE-CREATION src/test/python/apache/aurora/client/cli/util.py 76f954377c154d2a7ab3292a2d772b0566ea06fa Diff: https://reviews.apache.org/r/17185/diff/ Testing --- = test session starts == platform darwin -- Python 2.7.2 -- pytest-2.5.1 collected 3 items src/test/python/apache/aurora/admin/test_mesos_maintenance.py ... === 3 passed in 0.29 seconds === = test session starts == platform darwin -- Python 2.7.2 -- pytest-2.5.1 collected 2 items src/test/python/apache/aurora/client/test_binding_helper.py .. === 2 passed in 0.30 seconds === = test session starts == platform darwin -- Python 2.7.2 -- pytest-2.5.1 collected 6 items src/test/python/apache/aurora/client/test_config.py .. === 6 passed in 0.40 seconds === = test session starts == platform darwin -- Python 2.7.2 -- pytest-2.5.1 collected 6 items src/test/python/apache/aurora/client/api/test_disambiguator.py .. === 6 passed in 0.25 seconds === = test session starts == platform darwin -- Python 2.7.2 -- pytest-2.5.1 collected 1 items src/test/python/apache/aurora/client/api/test_job_monitor.py . === 1 passed in 0.22 seconds === = test session starts == platform darwin -- Python 2.7.2 -- pytest-2.5.1 collected 6 items src/test/python/apache/aurora/client/api/test_restarter.py .. === 6 passed in 0.21 seconds === = test session starts == platform darwin -- Python 2.7.2 -- pytest-2.5.1 collected 47 items / 1 skipped src/test/python/apache/aurora/client/api/test_scheduler_client.py ... = 47 passed, 1 skipped in 0.59 seconds = = test session starts == platform darwin -- Python 2.7.2 -- pytest-2.5.1 collected 20 items src/test/python/apache/aurora/client/api/test_instance_watcher.py src/test/python/apache/aurora/client/api/test_health_check.py == 20 passed in 0.24 seconds === = test session starts == platform darwin -- Python 2.7.2 -- pytest-2.5.1 collected 26 items src/test/python/apache/aurora/client/api/test_updater.py .. == 26 passed in 0.43 seconds
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
Re: Review Request 17088: Cache hashCode in generated immutable classes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17088/#review32503 --- Kevin — just waiting on a review from you. - Bill Farner On Jan. 18, 2014, 7:19 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17088/ --- (Updated Jan. 18, 2014, 7:19 p.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Bugs: AURORA-58 https://issues.apache.org/jira/browse/AURORA-58 Repository: aurora Description --- I've also silenced the code generator output by default, cleaning up the output when building via gradle. Diffs - src/main/python/apache/aurora/tools/java/thrift_wrapper_codegen.py ec3ac20ad9e81ace7db370fc54bed796b90758ca Diff: https://reviews.apache.org/r/17088/diff/ Testing --- ./gradlew build I also did a quick benchmark using the yet-to-be-released storage benchmark. Graphs attached for before and after this change. File Attachments by_id_non_cached.png https://reviews.apache.org/media/uploaded/files/2014/01/18/2d9aab73-4ae2-4b9d-bac1-8b0e5f893695__by_id_non_cached.png by_id_cached.png https://reviews.apache.org/media/uploaded/files/2014/01/18/d05f057e-7a88-496e-b65f-a0bd21190b71__by_id_cached.png by_job_non_cached.png https://reviews.apache.org/media/uploaded/files/2014/01/18/e18e5c98-2ecd-4579-8f6a-7053b31c2019__by_job_non_cached.png by_job_cached.png https://reviews.apache.org/media/uploaded/files/2014/01/18/59bed06f-f97f-4ea3-9a1d-aa670cb0d37f__by_job_cached.png by_role_non_cached.png https://reviews.apache.org/media/uploaded/files/2014/01/18/d92fb94d-b69b-460f-8194-ea95834a5ac8__by_role_non_cached.png by_role_cached.png https://reviews.apache.org/media/uploaded/files/2014/01/18/89be7e97-87fa-4476-86f5-7720868a3964__by_role_cached.png Thanks, Bill Farner
Re: Review Request 17131: Improve test coverage for CronJobManager.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17131/#review32516 --- Ship it! Ship It! - Suman Karumuri On Jan. 20, 2014, 9:01 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17131/ --- (Updated Jan. 20, 2014, 9:01 p.m.) Review request for Aurora, Suman Karumuri and Maxim Khutornenko. Bugs: AURORA-62 https://issues.apache.org/jira/browse/AURORA-62 Repository: aurora Description --- This raises instruction test coverage from 76% to 95%, and branch coverage from 75% to 96%. There are only two things not currently covered: - Handling when catching InterruptedException (this only logs) - Handling unknown CollisionPolicy (only logs) Diffs - src/main/java/org/apache/aurora/scheduler/state/CronJobManager.java 5a56a701a6a355f9de3f05fbb95013b96b06b171 src/test/java/org/apache/aurora/scheduler/state/CronJobManagerTest.java e9886cdb279cc42a961d6c964e2cfae3c4c13f61 Diff: https://reviews.apache.org/r/17131/diff/ Testing --- ./gradlew build Thanks, Bill Farner
Re: Review Request 17131: Improve test coverage for CronJobManager.
On Jan. 21, 2014, 3:29 p.m., Suman Karumuri wrote: src/main/java/org/apache/aurora/scheduler/state/CronJobManager.java, line 456 https://reviews.apache.org/r/17131/diff/1/?file=432900#file432900line456 The name CronJob sounds too generic. Consider changing it to SanitizedCronJob or a similar name that captures intent. Done. However, i generally favor brevity/readability when scope is as limited as it is here. The real intention was to put a type in the way to ensure that sanitization is performed. On Jan. 21, 2014, 3:29 p.m., Suman Karumuri wrote: src/main/java/org/apache/aurora/scheduler/state/CronJobManager.java, line 461 https://reviews.apache.org/r/17131/diff/1/?file=432900#file432900line461 inline this variable? Thanks, done. On Jan. 21, 2014, 3:29 p.m., Suman Karumuri wrote: src/test/java/org/apache/aurora/scheduler/state/CronJobManagerTest.java, line 192 https://reviews.apache.org/r/17131/diff/1/?file=432901#file432901line192 Would it be better if these comments are changed to JavaDoc comments? I'm not a fan of that idea since javadoc is really intended to be run through the javadoc tool for formal API documentation, which this is not. It would also force me to document the exception thrown, which wouldn't offer any value. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17131/#review32365 --- On Jan. 20, 2014, 9:01 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17131/ --- (Updated Jan. 20, 2014, 9:01 p.m.) Review request for Aurora, Suman Karumuri and Maxim Khutornenko. Bugs: AURORA-62 https://issues.apache.org/jira/browse/AURORA-62 Repository: aurora Description --- This raises instruction test coverage from 76% to 95%, and branch coverage from 75% to 96%. There are only two things not currently covered: - Handling when catching InterruptedException (this only logs) - Handling unknown CollisionPolicy (only logs) Diffs - src/main/java/org/apache/aurora/scheduler/state/CronJobManager.java 5a56a701a6a355f9de3f05fbb95013b96b06b171 src/test/java/org/apache/aurora/scheduler/state/CronJobManagerTest.java e9886cdb279cc42a961d6c964e2cfae3c4c13f61 Diff: https://reviews.apache.org/r/17131/diff/ Testing --- ./gradlew build Thanks, Bill Farner
Re: Review Request 17131: Improve test coverage for CronJobManager.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17131/ --- (Updated Jan. 22, 2014, 7:02 p.m.) Review request for Aurora, Suman Karumuri and Maxim Khutornenko. Changes --- CronJob - SanitizedCronJob. Bugs: AURORA-62 https://issues.apache.org/jira/browse/AURORA-62 Repository: aurora Description --- This raises instruction test coverage from 76% to 95%, and branch coverage from 75% to 96%. There are only two things not currently covered: - Handling when catching InterruptedException (this only logs) - Handling unknown CollisionPolicy (only logs) Diffs (updated) - src/main/java/org/apache/aurora/scheduler/state/CronJobManager.java 5a56a701a6a355f9de3f05fbb95013b96b06b171 src/test/java/org/apache/aurora/scheduler/state/CronJobManagerTest.java e9886cdb279cc42a961d6c964e2cfae3c4c13f61 Diff: https://reviews.apache.org/r/17131/diff/ Testing --- ./gradlew build Thanks, Bill Farner
Re: Review Request 17014: Show an error message in the UI when scheduler returns an invalid response.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17014/#review32520 --- Ship it! Ship It! - Bill Farner On Jan. 22, 2014, 7:22 p.m., Suman Karumuri wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17014/ --- (Updated Jan. 22, 2014, 7:22 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-18 https://issues.apache.org/jira/browse/AURORA-18 Repository: aurora Description --- Show an error message when a call to the thrift end point returns an invalid response. Diffs - src/main/resources/org/apache/aurora/scheduler/http/ui/index.html c6706c1d2b395966921c68a83acdef06c1bb8c53 src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js f1333f148970b5bc94f3ad77d077e90ebc09cf32 src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js 16f22a700abb7ba51559e3cfbb04d9218bf4f98a Diff: https://reviews.apache.org/r/17014/diff/ Testing --- gradle clean build. Tested with local scheduler. Scheduler shows an error message (ex: when storage is not ready) File Attachments error message https://reviews.apache.org/media/uploaded/files/2014/01/22/64224759-0596-4c06-a0c4-feb81ee260f4__Screen_Shot_2014-01-22_at_10.49.57_AM.png Thanks, Suman Karumuri
