Re: Review Request 33612: Add a task store implementation that uses a relational database.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33612/#review82623 --- api/src/main/thrift/org/apache/aurora/gen/api.thrift https://reviews.apache.org/r/33612/#comment133390 This seems unrelated to the description in this diff. src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java https://reviews.apache.org/r/33612/#comment133392 Presumably eager cleanup is expensive performance-wise. If that's the case would you mind calling that out explicitly in the comment? src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java https://reviews.apache.org/r/33612/#comment133393 Pull this CmdLine arg up to a module? src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java https://reviews.apache.org/r/33612/#comment133396 Inject a Clock here? src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java https://reviews.apache.org/r/33612/#comment133397 Is there an explicit design choice driving an DELETE+INSERT combination rather than an UPDATE+INSERT? If so can you call it out? If not can you drop a TODO to evaluate the tradeoffs of the switch? src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java https://reviews.apache.org/r/33612/#comment133399 Same question as above here - why DELETE+INSERT instead of UPDATE? src/main/java/org/apache/aurora/scheduler/storage/db/shims/ContainerShim.java https://reviews.apache.org/r/33612/#comment134454 This will lock in our thrift version when these become final and seems brittle (easy to add a new field to Container and have it compile fine) - can you file a ticket to investigate alternatives to this pattern? src/main/java/org/apache/aurora/scheduler/storage/db/shims/ContainerShim.java https://reviews.apache.org/r/33612/#comment134455 `isSet(_Fields.DOCKER)` src/main/java/org/apache/aurora/scheduler/storage/db/shims/ContainerShim.java https://reviews.apache.org/r/33612/#comment134456 Use isSet src/main/java/org/apache/aurora/scheduler/storage/db/shims/TaskConstraintShim.java https://reviews.apache.org/r/33612/#comment134457 isSet src/main/java/org/apache/aurora/scheduler/storage/db/shims/TaskConstraintShim.java https://reviews.apache.org/r/33612/#comment134458 isSet - Kevin Sweeney On May 9, 2015, 10:53 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33612/ --- (Updated May 9, 2015, 10:53 a.m.) Review request for Aurora, Joshua Cohen, Kevin Sweeney, and Maxim Khutornenko. Bugs: AURORA-556 https://issues.apache.org/jira/browse/AURORA-556 Repository: aurora Description --- Add a task store implementation that uses a relational database. The meat of this review is in `DbTaskStore`, `TaskConfigManager`, and the mapper xml files. Some supporting actors include files under views/, which serve DB business objects. I suggest reviewers start by skimming `DbTaskStore` and digesting the comments in there. There are some known loose ends here, most notably being continued performance enhancements and cleanup of relations in different tables. I've included several relevant TODOs, ~all of which must be addressed before this becomes the default task store. Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 0182ecb3942cfa2d9dd21138779815f4500339be examples/vagrant/upstart/aurora-scheduler.conf cc4864c4d954d58e9bfaaaf5fc5afc8d0e032e78 src/jmh/java/org/apache/aurora/benchmark/ThriftApiBenchmarks.java 88d27dd729fd004d1510917a031591addba51816 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 239c61625bc49e53be918c59056f071b3b6b86b9 src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java ea5600725d5dd84d21ca8d37b560c6d41541d016 src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java c8df88b77b9a2017c48bdc0c9a0477927ba0b179 src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 1a6c3f21d4fcb476539f588facecd8ef37332837 src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/TaskMapper.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/shims/ContainerShim.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/shims/TaskConstraintShim.java PRE-CREATION
Re: Review Request 33612: Add a task store implementation that uses a relational database.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33612/#review83494 --- Ship it! Master (759ef68) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On May 12, 2015, 9:17 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33612/ --- (Updated May 12, 2015, 9:17 p.m.) Review request for Aurora, Joshua Cohen, Kevin Sweeney, and Maxim Khutornenko. Bugs: AURORA-556 https://issues.apache.org/jira/browse/AURORA-556 Repository: aurora Description --- Add a task store implementation that uses a relational database. The meat of this review is in `DbTaskStore`, `TaskConfigManager`, and the mapper xml files. Some supporting actors include files under views/, which serve DB business objects. I suggest reviewers start by skimming `DbTaskStore` and digesting the comments in there. There are some known loose ends here, most notably being continued performance enhancements and cleanup of relations in different tables. I've included several relevant TODOs, ~all of which must be addressed before this becomes the default task store. Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 0182ecb3942cfa2d9dd21138779815f4500339be examples/vagrant/upstart/aurora-scheduler.conf cc4864c4d954d58e9bfaaaf5fc5afc8d0e032e78 src/jmh/java/org/apache/aurora/benchmark/ThriftApiBenchmarks.java 88d27dd729fd004d1510917a031591addba51816 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 239c61625bc49e53be918c59056f071b3b6b86b9 src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java ea5600725d5dd84d21ca8d37b560c6d41541d016 src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 8859ca47088896a1814321147c6b4c31828b3db9 src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 1a6c3f21d4fcb476539f588facecd8ef37332837 src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/DbUtil.java 7b4067cfe10d8aa1e9e0e8bfbcd0e83c9beb4e7a src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/TaskMapper.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/shims/ContainerShim.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/shims/TaskConstraintShim.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/ScheduleStatusTypeHandler.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java 4d0c10d60037a3310226a6fd8936b84ae4135e7e src/main/java/org/apache/aurora/scheduler/storage/db/views/AssignedPort.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/views/ScheduledTaskWrapper.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/views/TaskConfigRow.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/views/TaskLink.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/mem/InMemStoresModule.java 21f7d4db821930d2c5b52648e1996aa1ef12a85c src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml f76f9a988669dab598e9d19f849018c6f55ce03e src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml PRE-CREATION src/main/resources/org/apache/aurora/scheduler/storage/db/TaskMapper.xml PRE-CREATION src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java afb7db8eefa63b84d370877742870acdec58899c src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 6a6ff27d8d0f1e537a74c1df981b97e5d8b2f2e8 src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java 7d856d020da854c125c037f01357e81de93895e1 src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java 8f139fc987a98ef0d7f2969720134729411b8b84 src/test/java/org/apache/aurora/scheduler/storage/mem/StorageTransactionTest.java bad9eb56b33c3e649c3b173e83d9c30da8f9317d Diff: https://reviews.apache.org/r/33612/diff/ Testing --- Unit tests and end-to-end tests, both using the new task store by default with
Re: Review Request 34121: Disable state transition logging for unknown tasks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34121/#review83484 --- Ship it! Master (759ef68) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On May 12, 2015, 8:40 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34121/ --- (Updated May 12, 2015, 8:40 p.m.) Review request for Aurora and Bill Farner. Repository: aurora Description --- Quieting down state transition logging in preparation for task state reconciliation. Diffs - src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 2a943cf51d0a41260ada6965cea5f55db4c3f846 src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java 4a7ca6267ae6c4c062f941c78a4a3496d110e8c2 src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java afb7db8eefa63b84d370877742870acdec58899c src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java b30a0737bb0d60cd8b58f7be0fff5db20f808347 Diff: https://reviews.apache.org/r/34121/diff/ Testing --- ./gradlew -Pq build ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh Thanks, Maxim Khutornenko
Re: Review Request 33612: Add a task store implementation that uses a relational database.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33612/#review83487 --- Ship it! src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java https://reviews.apache.org/r/33612/#comment134494 module - Kevin Sweeney On May 12, 2015, 2:17 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33612/ --- (Updated May 12, 2015, 2:17 p.m.) Review request for Aurora, Joshua Cohen, Kevin Sweeney, and Maxim Khutornenko. Bugs: AURORA-556 https://issues.apache.org/jira/browse/AURORA-556 Repository: aurora Description --- Add a task store implementation that uses a relational database. The meat of this review is in `DbTaskStore`, `TaskConfigManager`, and the mapper xml files. Some supporting actors include files under views/, which serve DB business objects. I suggest reviewers start by skimming `DbTaskStore` and digesting the comments in there. There are some known loose ends here, most notably being continued performance enhancements and cleanup of relations in different tables. I've included several relevant TODOs, ~all of which must be addressed before this becomes the default task store. Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 0182ecb3942cfa2d9dd21138779815f4500339be examples/vagrant/upstart/aurora-scheduler.conf cc4864c4d954d58e9bfaaaf5fc5afc8d0e032e78 src/jmh/java/org/apache/aurora/benchmark/ThriftApiBenchmarks.java 88d27dd729fd004d1510917a031591addba51816 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 239c61625bc49e53be918c59056f071b3b6b86b9 src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java ea5600725d5dd84d21ca8d37b560c6d41541d016 src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 8859ca47088896a1814321147c6b4c31828b3db9 src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 1a6c3f21d4fcb476539f588facecd8ef37332837 src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/DbUtil.java 7b4067cfe10d8aa1e9e0e8bfbcd0e83c9beb4e7a src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/TaskMapper.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/shims/ContainerShim.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/shims/TaskConstraintShim.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/ScheduleStatusTypeHandler.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java 4d0c10d60037a3310226a6fd8936b84ae4135e7e src/main/java/org/apache/aurora/scheduler/storage/db/views/AssignedPort.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/views/ScheduledTaskWrapper.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/views/TaskConfigRow.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/views/TaskLink.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/mem/InMemStoresModule.java 21f7d4db821930d2c5b52648e1996aa1ef12a85c src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml f76f9a988669dab598e9d19f849018c6f55ce03e src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml PRE-CREATION src/main/resources/org/apache/aurora/scheduler/storage/db/TaskMapper.xml PRE-CREATION src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java afb7db8eefa63b84d370877742870acdec58899c src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 6a6ff27d8d0f1e537a74c1df981b97e5d8b2f2e8 src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java 7d856d020da854c125c037f01357e81de93895e1 src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java 8f139fc987a98ef0d7f2969720134729411b8b84 src/test/java/org/apache/aurora/scheduler/storage/mem/StorageTransactionTest.java bad9eb56b33c3e649c3b173e83d9c30da8f9317d Diff: https://reviews.apache.org/r/33612/diff/ Testing --- Unit tests and end-to-end tests, both using the new task store by default with this change. Thanks, Bill
Review Request 34124: Removing dead code from StateManagerImpl.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34124/ --- Review request for Aurora and Bill Farner. Repository: aurora Description --- Removing dead code from StateManagerImpl. Diffs - src/main/java/org/apache/aurora/scheduler/state/SideEffect.java 180852e56734f5b03cf52bd8324db36612b33ade src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 2a943cf51d0a41260ada6965cea5f55db4c3f846 src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java b30a0737bb0d60cd8b58f7be0fff5db20f808347 Diff: https://reviews.apache.org/r/34124/diff/ Testing --- Thanks, Maxim Khutornenko
Re: Review Request 34121: Disable state transition logging for unknown tasks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34121/#review83477 --- @ReviewBot retry - Maxim Khutornenko On May 12, 2015, 8:40 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34121/ --- (Updated May 12, 2015, 8:40 p.m.) Review request for Aurora and Bill Farner. Repository: aurora Description --- Quieting down state transition logging in preparation for task state reconciliation. Diffs - src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 2a943cf51d0a41260ada6965cea5f55db4c3f846 src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java 4a7ca6267ae6c4c062f941c78a4a3496d110e8c2 src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java afb7db8eefa63b84d370877742870acdec58899c src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java b30a0737bb0d60cd8b58f7be0fff5db20f808347 Diff: https://reviews.apache.org/r/34121/diff/ Testing --- ./gradlew -Pq build ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh Thanks, Maxim Khutornenko
Re: Review Request 33612: Add a task store implementation that uses a relational database.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33612/ --- (Updated May 12, 2015, 9:17 p.m.) Review request for Aurora, Joshua Cohen, Kevin Sweeney, and Maxim Khutornenko. Changes --- Address comments + rebase Bugs: AURORA-556 https://issues.apache.org/jira/browse/AURORA-556 Repository: aurora Description --- Add a task store implementation that uses a relational database. The meat of this review is in `DbTaskStore`, `TaskConfigManager`, and the mapper xml files. Some supporting actors include files under views/, which serve DB business objects. I suggest reviewers start by skimming `DbTaskStore` and digesting the comments in there. There are some known loose ends here, most notably being continued performance enhancements and cleanup of relations in different tables. I've included several relevant TODOs, ~all of which must be addressed before this becomes the default task store. Diffs (updated) - api/src/main/thrift/org/apache/aurora/gen/api.thrift 0182ecb3942cfa2d9dd21138779815f4500339be examples/vagrant/upstart/aurora-scheduler.conf cc4864c4d954d58e9bfaaaf5fc5afc8d0e032e78 src/jmh/java/org/apache/aurora/benchmark/ThriftApiBenchmarks.java 88d27dd729fd004d1510917a031591addba51816 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 239c61625bc49e53be918c59056f071b3b6b86b9 src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java ea5600725d5dd84d21ca8d37b560c6d41541d016 src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 8859ca47088896a1814321147c6b4c31828b3db9 src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 1a6c3f21d4fcb476539f588facecd8ef37332837 src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/DbUtil.java 7b4067cfe10d8aa1e9e0e8bfbcd0e83c9beb4e7a src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/TaskMapper.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/shims/ContainerShim.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/shims/TaskConstraintShim.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/ScheduleStatusTypeHandler.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java 4d0c10d60037a3310226a6fd8936b84ae4135e7e src/main/java/org/apache/aurora/scheduler/storage/db/views/AssignedPort.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/views/ScheduledTaskWrapper.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/views/TaskConfigRow.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/db/views/TaskLink.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/storage/mem/InMemStoresModule.java 21f7d4db821930d2c5b52648e1996aa1ef12a85c src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml f76f9a988669dab598e9d19f849018c6f55ce03e src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml PRE-CREATION src/main/resources/org/apache/aurora/scheduler/storage/db/TaskMapper.xml PRE-CREATION src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java afb7db8eefa63b84d370877742870acdec58899c src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 6a6ff27d8d0f1e537a74c1df981b97e5d8b2f2e8 src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java 7d856d020da854c125c037f01357e81de93895e1 src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java 8f139fc987a98ef0d7f2969720134729411b8b84 src/test/java/org/apache/aurora/scheduler/storage/mem/StorageTransactionTest.java bad9eb56b33c3e649c3b173e83d9c30da8f9317d Diff: https://reviews.apache.org/r/33612/diff/ Testing --- Unit tests and end-to-end tests, both using the new task store by default with this change. Thanks, Bill Farner
Re: Review Request 33612: Add a task store implementation that uses a relational database.
On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote: api/src/main/thrift/org/apache/aurora/gen/api.thrift, line 265 https://reviews.apache.org/r/33612/diff/2/?file=950455#file950455line265 This seems unrelated to the description in this diff. It is related, as we don't have a `getContainer()` method to use in the diff without this change (or a fix to the linked bug). On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote: src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 131 https://reviews.apache.org/r/33612/diff/2/?file=950460#file950460line131 Presumably eager cleanup is expensive performance-wise. If that's the case would you mind calling that out explicitly in the comment? Are you assuming i added a close delay because of perf? If so, that's not the reason - please see the previous comment rounds with Maxim. On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote: src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, line 154 https://reviews.apache.org/r/33612/diff/2/?file=950462#file950462line154 Is there an explicit design choice driving an DELETE+INSERT combination rather than an UPDATE+INSERT? If so can you call it out? If not can you drop a TODO to evaluate the tradeoffs of the switch? This was discussed in previous rounds with Maxim, and a TODO exists on line 148. Please let me know if there's something more you're after. On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote: src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, lines 259-262 https://reviews.apache.org/r/33612/diff/2/?file=950462#file950462line259 Same question as above here - why DELETE+INSERT instead of UPDATE? ^^ On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote: src/main/java/org/apache/aurora/scheduler/storage/db/shims/ContainerShim.java, line 24 https://reviews.apache.org/r/33612/diff/2/?file=950466#file950466line24 This will lock in our thrift version when these become final and seems brittle (easy to add a new field to Container and have it compile fine) - can you file a ticket to investigate alternatives to this pattern? To be honest, i feel as though i already exhausted the options before getting here. AFAICT the alternatives are non-trivial: change the thrift codegen and upgrade, or avoid use of thrift for database model objects. I won't coerce you against filing a ticket, but i'd prefer not to since i doubt it will be addressed independently. On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote: src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, line 77 https://reviews.apache.org/r/33612/diff/2/?file=950462#file950462line77 Pull this CmdLine arg up to a module? Ok On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote: src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, line 99 https://reviews.apache.org/r/33612/diff/2/?file=950462#file950462line99 Inject a Clock here? Ok On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote: src/main/java/org/apache/aurora/scheduler/storage/db/shims/ContainerShim.java, line 27 https://reviews.apache.org/r/33612/diff/2/?file=950466#file950466line27 `isSet(_Fields.DOCKER)` Done. On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote: src/main/java/org/apache/aurora/scheduler/storage/db/shims/ContainerShim.java, line 36 https://reviews.apache.org/r/33612/diff/2/?file=950466#file950466line36 Use isSet Done. On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote: src/main/java/org/apache/aurora/scheduler/storage/db/shims/TaskConstraintShim.java, line 36 https://reviews.apache.org/r/33612/diff/2/?file=950467#file950467line36 isSet Done. On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote: src/main/java/org/apache/aurora/scheduler/storage/db/shims/TaskConstraintShim.java, line 27 https://reviews.apache.org/r/33612/diff/2/?file=950467#file950467line27 isSet Done. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33612/#review82623 --- On May 9, 2015, 5:53 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33612/ --- (Updated May 9, 2015, 5:53 p.m.) Review request for Aurora, Joshua Cohen, Kevin Sweeney, and Maxim Khutornenko. Bugs: AURORA-556 https://issues.apache.org/jira/browse/AURORA-556 Repository: aurora Description --- Add a task store implementation that uses a relational database. The meat of this review is in `DbTaskStore`, `TaskConfigManager`, and the mapper xml files. Some supporting actors include files under views/, which serve DB
Re: Review Request 34121: Disable state transition logging for unknown tasks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34121/#review83481 --- What's the motivation here? Seems like attempted state transitions for unknown tasks should be transient and/or represent a bug. In those cases, i would assume we definitely want logging. Is there somthing i'm overlooking? src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java https://reviews.apache.org/r/34121/#comment134492 This is pretty weird behavior - 'no logging unless you ask really nicely'. Rather than the enableLogging flag, how about we let the caller pass a Logger, and for the finest-only logging, we have a logger that has its level set appropriately? - Bill Farner On May 12, 2015, 8:40 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34121/ --- (Updated May 12, 2015, 8:40 p.m.) Review request for Aurora and Bill Farner. Repository: aurora Description --- Quieting down state transition logging in preparation for task state reconciliation. Diffs - src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 2a943cf51d0a41260ada6965cea5f55db4c3f846 src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java 4a7ca6267ae6c4c062f941c78a4a3496d110e8c2 src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java afb7db8eefa63b84d370877742870acdec58899c src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java b30a0737bb0d60cd8b58f7be0fff5db20f808347 Diff: https://reviews.apache.org/r/34121/diff/ Testing --- ./gradlew -Pq build ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh Thanks, Maxim Khutornenko
Re: Review Request 34121: Disable state transition logging for unknown tasks.
On May 12, 2015, 9:22 p.m., Bill Farner wrote: What's the motivation here? Seems like attempted state transitions for unknown tasks should be transient and/or represent a bug. In those cases, i would assume we definitely want logging. Is there somthing i'm overlooking? The idea is to avoid any logging for any state transitions coming from task reconciliation. I just realized though that the current approach addresses only implicit reconciliation and will still log explicit state transitions. I am going to rework this diff to support a higher level switch. Ignore it for now. On May 12, 2015, 9:22 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java, line 567 https://reviews.apache.org/r/34121/diff/1/?file=956801#file956801line567 This is pretty weird behavior - 'no logging unless you ask really nicely'. Rather than the enableLogging flag, how about we let the caller pass a Logger, and for the finest-only logging, we have a logger that has its level set appropriately? If the logger is passed from the caller, we will have no way to enable logging when needed (e.g. for debugging state reconciliation task transitions). Having a static logger let's us dynamically override (reenable) TaskStateMachine logging via /logconfig endpoint. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34121/#review83481 --- On May 12, 2015, 8:40 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34121/ --- (Updated May 12, 2015, 8:40 p.m.) Review request for Aurora and Bill Farner. Repository: aurora Description --- Quieting down state transition logging in preparation for task state reconciliation. Diffs - src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 2a943cf51d0a41260ada6965cea5f55db4c3f846 src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java 4a7ca6267ae6c4c062f941c78a4a3496d110e8c2 src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java afb7db8eefa63b84d370877742870acdec58899c src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java b30a0737bb0d60cd8b58f7be0fff5db20f808347 Diff: https://reviews.apache.org/r/34121/diff/ Testing --- ./gradlew -Pq build ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh Thanks, Maxim Khutornenko
Re: Review Request 34148: Enhancing the StateManager.changeState result.
On May 13, 2015, 1:53 a.m., Aurora ReviewBot wrote: Master (bf7f9b7) is red with this patch. ./build-support/jenkins/build.sh :distZip :assemble :compileJmhJavaNote: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java uses or overrides a deprecated API. Note: Recompile with -Xlint:deprecation for details. :processJmhResources UP-TO-DATE :jmhClasses :checkstyleJmh :jsHint :checkstyleMain :compileTestJava/home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/java/org/apache/aurora/scheduler/SchedulerLifecycleTest.java:81: error: method addAction in interface ShutdownRegistry cannot be applied to given types; shutdownRegistry.addAction(capture(shutdownCommand)); ^ required: T#1 found: ExceptionalCommandCAP#1 reason: inference variable T#2 has incompatible bounds equality constraints: ExceptionalCommand? upper bounds: ExceptionalCommandCAP#2,T#1,Object where T#1,E,T#2 are type-variables: T#1 extends ExceptionalCommandE declared in method E,T#1addAction(T#1) E extends Exception declared in method E,T#1addAction(T#1) T#2 extends Object declared in method T#2capture(CaptureT#2) where CAP#1,CAP#2 are fresh type-variables: CAP#1 extends Exception from capture of ? CAP#2 extends Exception from capture of ? 1 error FAILED FAILURE: Build failed with an exception. * What went wrong: Execution failed for task ':compileTestJava'. Compilation failed; see the compiler error output for details. * Try: Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. BUILD FAILED Total time: 1 mins 51.943 secs I will refresh this build result if you post a review containing @ReviewBot retry Review bot still using JDK 7? - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34148/#review83545 --- On May 13, 2015, 1:43 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34148/ --- (Updated May 13, 2015, 1:43 a.m.) Review request for Aurora and Bill Farner. Repository: aurora Description --- Adding more details into task state change result to facilitate task reconciliation data collection. Diffs - src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 90e614958dfa992921e05cff86ddcc434efdd112 src/main/java/org/apache/aurora/scheduler/state/StateChangeResult.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/state/StateManager.java 71bfefb8cff3e9ad1fa9566ba55c0e3541fb01f3 src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java d87bb3818ae950125a54ff63d2ba52bfc67e6708 src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java 4a7ca6267ae6c4c062f941c78a4a3496d110e8c2 src/main/java/org/apache/aurora/scheduler/state/TransitionResult.java 874c554f84ea9290aa0d3874241da1b23dd453a7 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 160db129578365e0dd67d3354d98497f567dd621 src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 32432322753799562d671db39c0d7fa308d962ff src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java f17c43475a09bf0bbbcc49a3b372484e7937c27f src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java a637101d0f01865dc2b3f0ee00aca81d0fbf0490 src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java 88fc172be6c24fefb6f708ce757487082542 src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java 32d18a9b8af2ac04b0f82fe866c3eed7e923584d src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java 831803f0bf8000bd88fe870b6151ceca59c620fa src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java 7b101bc2bb5f6f1854187aa33406049a94fbb2fd src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 15e4d388795b2ab2723373a73b419878b6346456 src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java afbca61cb6a4b0a81346c496fa21077bda3c13de src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 1ac1a2838a374383d3190d1fc5b4782d03f1d826 src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 4e7ff3b3b2e12b43df157b1af6548db306c141da Diff: https://reviews.apache.org/r/34148/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 34124: Removing dead code from StateManagerImpl.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34124/#review83501 --- Ship it! Master (759ef68) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On May 12, 2015, 9:42 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34124/ --- (Updated May 12, 2015, 9:42 p.m.) Review request for Aurora and Bill Farner. Repository: aurora Description --- Removing dead code from StateManagerImpl. Diffs - src/main/java/org/apache/aurora/scheduler/state/SideEffect.java 180852e56734f5b03cf52bd8324db36612b33ade src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 2a943cf51d0a41260ada6965cea5f55db4c3f846 src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java b30a0737bb0d60cd8b58f7be0fff5db20f808347 Diff: https://reviews.apache.org/r/34124/diff/ Testing --- Thanks, Maxim Khutornenko
Review Request 34126: Use JDK 8 language features.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34126/ --- Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Bugs: AURORA-274 https://issues.apache.org/jira/browse/AURORA-274 Repository: aurora Description --- Two code changes were required: - a unit test made an assumption about iteration order over a `HashSet` - a new compiler warning showed up on possible dereference of a null pointer Note the first introduction of a lambda as a PoC while modifying a unit test. Diffs - NEWS PRE-CREATION build.gradle ec9b52206ce602d0f4a0a6d4b7fe55d055a787dc examples/vagrant/provision-dev-cluster.sh 2c5ce970446f8849b66d7035b8b10b253827db83 src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java 218ae0da80f9129107da17004f23098b9507b33d src/test/java/org/apache/aurora/scheduler/storage/backup/StorageBackupTest.java d1301e0ea3c255563709048247772648052e8041 Diff: https://reviews.apache.org/r/34126/diff/ Testing --- ``` ./build-support/jenkins/build.sh ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh # TODO(wfarner): Report back, still executing. ``` Thanks, Bill Farner
Re: Review Request 34126: Use JDK 8 language features.
On May 12, 2015, 11:14 p.m., Aurora ReviewBot wrote: Master (c1b0dce) is red with this patch. ./build-support/jenkins/build.sh make[4]: Entering directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift/thrift-0.9.1/tutorial' ../compiler/cpp/thrift --gen html -r ../tutorial/tutorial.thrift make[4]: Leaving directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift/thrift-0.9.1/tutorial' make[3]: Leaving directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift/thrift-0.9.1/tutorial' Making all in tutorial make[3]: Entering directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift/thrift-0.9.1/tutorial' make[4]: Entering directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift/thrift-0.9.1/tutorial' ../compiler/cpp/thrift --gen html -r ../tutorial/tutorial.thrift make[4]: Leaving directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift/thrift-0.9.1/tutorial' make[3]: Leaving directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift/thrift-0.9.1/tutorial' make[3]: Entering directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift/thrift-0.9.1' make[3]: Leaving directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift/thrift-0.9.1' make[2]: Leaving directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift/thrift-0.9.1' make[1]: Leaving directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift/thrift-0.9.1' make: Leaving directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift' :api:classesThriftNote: Some input files use unchecked or unsafe operations. Note: Recompile with -Xlint:unchecked for details. :api:checkPython :api:generateThriftEntitiesJava :api:classesThriftEntities :api:compileJava UP-TO-DATE :api:generateThriftResources :api:processResources UP-TO-DATE :api:classes :api:jar :compileJava FAILED FAILURE: Build failed with an exception. * What went wrong: Execution failed for task ':compileJava'. invalid source release: 1.8 * Try: Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. BUILD FAILED Total time: 1 mins 22.51 secs I will refresh this build result if you post a review containing @ReviewBot retry Note: this is expected while we straddle the line between 7 and 8. After this lands, i will update the ReviewBot build to use 8. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34126/#review83515 --- On May 12, 2015, 10:54 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34126/ --- (Updated May 12, 2015, 10:54 p.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Bugs: AURORA-274 https://issues.apache.org/jira/browse/AURORA-274 Repository: aurora Description --- Two code changes were required: - a unit test made an assumption about iteration order over a `HashSet` - a new compiler warning showed up on possible dereference of a null pointer Note the first introduction of a lambda as a PoC while modifying a unit test. Diffs - NEWS PRE-CREATION build.gradle ec9b52206ce602d0f4a0a6d4b7fe55d055a787dc examples/vagrant/provision-dev-cluster.sh 2c5ce970446f8849b66d7035b8b10b253827db83 src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java 218ae0da80f9129107da17004f23098b9507b33d src/test/java/org/apache/aurora/scheduler/storage/backup/StorageBackupTest.java d1301e0ea3c255563709048247772648052e8041 Diff: https://reviews.apache.org/r/34126/diff/ Testing --- ``` ./build-support/jenkins/build.sh ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh # TODO(wfarner): Report back, still executing. ``` Thanks, Bill Farner
Re: Review Request 34126: Use JDK 8 language features.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34126/#review83516 --- Ship it! src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java https://reviews.apache.org/r/34126/#comment134512 Can't wait! - Maxim Khutornenko On May 12, 2015, 10:54 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34126/ --- (Updated May 12, 2015, 10:54 p.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Bugs: AURORA-274 https://issues.apache.org/jira/browse/AURORA-274 Repository: aurora Description --- Two code changes were required: - a unit test made an assumption about iteration order over a `HashSet` - a new compiler warning showed up on possible dereference of a null pointer Note the first introduction of a lambda as a PoC while modifying a unit test. Diffs - NEWS PRE-CREATION build.gradle ec9b52206ce602d0f4a0a6d4b7fe55d055a787dc examples/vagrant/provision-dev-cluster.sh 2c5ce970446f8849b66d7035b8b10b253827db83 src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java 218ae0da80f9129107da17004f23098b9507b33d src/test/java/org/apache/aurora/scheduler/storage/backup/StorageBackupTest.java d1301e0ea3c255563709048247772648052e8041 Diff: https://reviews.apache.org/r/34126/diff/ Testing --- ``` ./build-support/jenkins/build.sh ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh # TODO(wfarner): Report back, still executing. ``` Thanks, Bill Farner
Re: Review Request 33689: Updated scheduler to process status updates asynchronously in batches.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33689/ --- (Updated May 12, 2015, 10:56 p.m.) Review request for Aurora, Maxim Khutornenko and Bill Farner. Changes --- Restore the interrupt per bill's comment. Bugs: AURORA-1228 https://issues.apache.org/jira/browse/AURORA-1228 Repository: aurora Description --- Now the processing of status updates is done asynchronously with batching to insulate throughput from the expensive storage resource. Updates are placed into a queue and consumed by another thread. If many updates arrive while we're storing a batch of updates, these will be processed together in batch rather than individually. Diffs (updated) - src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 7bb64dd913f0fe2fede95d50a061043dbb794ab4 src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 45de15a57baf7a2f7d437b590935714e28777f35 src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d3ac176e9402a33fd2074b0737313458120da9e2 src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 0ce9c9d4cf75f9add260f285115b1d60786ded57 src/main/java/org/apache/aurora/scheduler/async/GcExecutorLauncher.java 4d589a33a2933b0cb6caf85abfae45c5e635c3ce src/main/java/org/apache/aurora/scheduler/mesos/Driver.java c7e45a89ceaa2c310feb610091eec0b04187860e src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 9b8ab7c1027731f9d3f6cae77b85272ea63354d4 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java da2d5df2e053e6e1b8fb08d6813dff9eac9777f8 src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 32432322753799562d671db39c0d7fa308d962ff src/test/java/org/apache/aurora/scheduler/async/GcExecutorLauncherTest.java 422d5a9a42310979752eb7282658316c2b772419 src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java abdeee49858fc439c27911c4eb544bf8e8c931d4 Diff: https://reviews.apache.org/r/33689/diff/ Testing --- Ran the benchmark to confirm that this improves status update throughput substantially: Before: Around 100 updates per second for a 5ms storage latency. Much worse for higher latencies. After: Around 4k-5k updates per second for a 5ms storage latency, down to 3k updates per second for 100ms storage latency. Updated unit tests for the new invariants: * TaskLaunchers are responsible for acknowledging updates. * UserTaskLauncher processes updates asynchronously. Thanks, Ben Mahler
Re: Review Request 34126: Use JDK 8 language features.
On May 12, 2015, 11:08 p.m., Kevin Sweeney wrote: examples/vagrant/provision-dev-cluster.sh, line 33 https://reviews.apache.org/r/34126/diff/1/?file=957021#file957021line33 comment is a lie now, but it seems safe to remove as the update-alternatives line is self-explanatory Thanks, removed. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34126/#review83513 --- On May 12, 2015, 11:41 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34126/ --- (Updated May 12, 2015, 11:41 p.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Bugs: AURORA-274 https://issues.apache.org/jira/browse/AURORA-274 Repository: aurora Description --- Two code changes were required: - a unit test made an assumption about iteration order over a `HashSet` - a new compiler warning showed up on possible dereference of a null pointer Note the first introduction of a lambda as a PoC while modifying a unit test. Diffs - NEWS PRE-CREATION build.gradle ec9b52206ce602d0f4a0a6d4b7fe55d055a787dc examples/vagrant/provision-dev-cluster.sh 2c5ce970446f8849b66d7035b8b10b253827db83 src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java 218ae0da80f9129107da17004f23098b9507b33d src/test/java/org/apache/aurora/scheduler/storage/backup/StorageBackupTest.java d1301e0ea3c255563709048247772648052e8041 Diff: https://reviews.apache.org/r/34126/diff/ Testing --- ``` ./build-support/jenkins/build.sh ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh ``` Thanks, Bill Farner
Re: Review Request 34126: Use JDK 8 language features.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34126/ --- (Updated May 12, 2015, 11:42 p.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Bugs: AURORA-274 https://issues.apache.org/jira/browse/AURORA-274 Repository: aurora Description --- Two code changes were required: - a unit test made an assumption about iteration order over a `HashSet` - a new compiler warning showed up on possible dereference of a null pointer Note the first introduction of a lambda as a PoC while modifying a unit test. Diffs (updated) - NEWS PRE-CREATION build.gradle ec9b52206ce602d0f4a0a6d4b7fe55d055a787dc examples/vagrant/provision-dev-cluster.sh 2c5ce970446f8849b66d7035b8b10b253827db83 src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java 218ae0da80f9129107da17004f23098b9507b33d src/test/java/org/apache/aurora/scheduler/storage/backup/StorageBackupTest.java d1301e0ea3c255563709048247772648052e8041 Diff: https://reviews.apache.org/r/34126/diff/ Testing --- ``` ./build-support/jenkins/build.sh ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh ``` Thanks, Bill Farner
Re: Review Request 34126: Use JDK 8 language features.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34126/#review83513 --- Ship it! examples/vagrant/provision-dev-cluster.sh https://reviews.apache.org/r/34126/#comment134511 comment is a lie now, but it seems safe to remove as the update-alternatives line is self-explanatory - Kevin Sweeney On May 12, 2015, 3:54 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34126/ --- (Updated May 12, 2015, 3:54 p.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Bugs: AURORA-274 https://issues.apache.org/jira/browse/AURORA-274 Repository: aurora Description --- Two code changes were required: - a unit test made an assumption about iteration order over a `HashSet` - a new compiler warning showed up on possible dereference of a null pointer Note the first introduction of a lambda as a PoC while modifying a unit test. Diffs - NEWS PRE-CREATION build.gradle ec9b52206ce602d0f4a0a6d4b7fe55d055a787dc examples/vagrant/provision-dev-cluster.sh 2c5ce970446f8849b66d7035b8b10b253827db83 src/test/java/org/apache/aurora/scheduler/async/preemptor/PendingTaskProcessorTest.java 218ae0da80f9129107da17004f23098b9507b33d src/test/java/org/apache/aurora/scheduler/storage/backup/StorageBackupTest.java d1301e0ea3c255563709048247772648052e8041 Diff: https://reviews.apache.org/r/34126/diff/ Testing --- ``` ./build-support/jenkins/build.sh ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh # TODO(wfarner): Report back, still executing. ``` Thanks, Bill Farner
Re: Review Request 33689: Updated scheduler to process status updates asynchronously in batches.
On May 12, 2015, 12:18 a.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java, line 172 https://reviews.apache.org/r/33689/diff/4/?file=955534#file955534line172 The histogram would be interesting, but possibly overkill. We should be able to get good signal with stats we already have - number of status updates vs number of log writes Good to know about the log write counter, I'll remove the TODO. On May 12, 2015, 12:18 a.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java, line 156 https://reviews.apache.org/r/33689/diff/4/?file=955534#file955534line156 This comment was slightly misleading, and probably belongs down near `continue;`, to communicate that the timeout was met and we are going to try again if the service is still running. However, i don't see why you can't get access to the thread. You could get it from `Thread.currentThread()` here, and use an `AtomicReference` to keep a ref. Great suggestion, thanks Bill! - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33689/#review83304 --- On May 11, 2015, 6:55 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33689/ --- (Updated May 11, 2015, 6:55 p.m.) Review request for Aurora, Maxim Khutornenko and Bill Farner. Bugs: AURORA-1228 https://issues.apache.org/jira/browse/AURORA-1228 Repository: aurora Description --- Now the processing of status updates is done asynchronously with batching to insulate throughput from the expensive storage resource. Updates are placed into a queue and consumed by another thread. If many updates arrive while we're storing a batch of updates, these will be processed together in batch rather than individually. Diffs - src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 7bb64dd913f0fe2fede95d50a061043dbb794ab4 src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 45de15a57baf7a2f7d437b590935714e28777f35 src/main/java/org/apache/aurora/scheduler/SchedulerModule.java d3ac176e9402a33fd2074b0737313458120da9e2 src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 0ce9c9d4cf75f9add260f285115b1d60786ded57 src/main/java/org/apache/aurora/scheduler/async/GcExecutorLauncher.java 4d589a33a2933b0cb6caf85abfae45c5e635c3ce src/main/java/org/apache/aurora/scheduler/mesos/Driver.java c7e45a89ceaa2c310feb610091eec0b04187860e src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 9b8ab7c1027731f9d3f6cae77b85272ea63354d4 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java da2d5df2e053e6e1b8fb08d6813dff9eac9777f8 src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 32432322753799562d671db39c0d7fa308d962ff src/test/java/org/apache/aurora/scheduler/async/GcExecutorLauncherTest.java 422d5a9a42310979752eb7282658316c2b772419 src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java abdeee49858fc439c27911c4eb544bf8e8c931d4 Diff: https://reviews.apache.org/r/33689/diff/ Testing --- Ran the benchmark to confirm that this improves status update throughput substantially: Before: Around 100 updates per second for a 5ms storage latency. Much worse for higher latencies. After: Around 4k-5k updates per second for a 5ms storage latency, down to 3k updates per second for 100ms storage latency. Updated unit tests for the new invariants: * TaskLaunchers are responsible for acknowledging updates. * UserTaskLauncher processes updates asynchronously. Thanks, Ben Mahler
