You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Bill Farner <wf...@apache.org> on 2015/04/28 22:11:46 UTC
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/
-----------------------------------------------------------
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 82ad42fd0a626672dca80a5362fc07d804b3e412
src/jmh/java/org/apache/aurora/benchmark/ThriftApiBenchmarks.java ed1171d52655fef643330f34913c256f77300fa2
src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3d19831ea0eb85032172b096ac87e126701aa218
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 94ce5c3499ced1b63abf19787acc21b2cd4d0c75
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
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 e3b13407cb6875489e50cf93212845eab7aacb89
src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java f18619a27eeb2aea8dcf01e54c23ed7d1c7d3d87
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.
Posted by Bill Farner <wf...@apache.org>.
> On May 3, 2015, 2:34 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, line 147
> > <https://reviews.apache.org/r/33612/diff/1/?file=944389#file944389line147>
> >
> > Copy paste error. Should be `db_storage_save_tasks`.
Good catch! Fixed.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review82344
-----------------------------------------------------------
On April 28, 2015, 8:11 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33612/
> -----------------------------------------------------------
>
> (Updated April 28, 2015, 8:11 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 82ad42fd0a626672dca80a5362fc07d804b3e412
> src/jmh/java/org/apache/aurora/benchmark/ThriftApiBenchmarks.java ed1171d52655fef643330f34913c256f77300fa2
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3d19831ea0eb85032172b096ac87e126701aa218
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 94ce5c3499ced1b63abf19787acc21b2cd4d0c75
> 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
> 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 e3b13407cb6875489e50cf93212845eab7aacb89
> src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java f18619a27eeb2aea8dcf01e54c23ed7d1c7d3d87
> 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.
Posted by Stephan Erb <st...@dev.static-void.de>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review82344
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java
<https://reviews.apache.org/r/33612/#comment133061>
Copy paste error. Should be `db_storage_save_tasks`.
- Stephan Erb
On April 28, 2015, 10:11 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33612/
> -----------------------------------------------------------
>
> (Updated April 28, 2015, 10:11 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 82ad42fd0a626672dca80a5362fc07d804b3e412
> src/jmh/java/org/apache/aurora/benchmark/ThriftApiBenchmarks.java ed1171d52655fef643330f34913c256f77300fa2
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3d19831ea0eb85032172b096ac87e126701aa218
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 94ce5c3499ced1b63abf19787acc21b2cd4d0c75
> 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
> 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 e3b13407cb6875489e50cf93212845eab7aacb89
> src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java f18619a27eeb2aea8dcf01e54c23ed7d1c7d3d87
> 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.
Posted by Joshua Cohen <jc...@apache.org>.
> On May 7, 2015, 10:22 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, lines 102-106
> > <https://reviews.apache.org/r/33612/diff/2/?file=950462#file950462line102>
> >
> > It'd be nice if this was extracted out to a higher level so it was reusable across DB store impls without needing to be repeated in each method.
> >
> > Maybe replace the usage of the TimedInterceptor with a new DBTimedInterceptor or some such?
> >
> > (If you think this is a good idea I'm fine with just dropping a TODO and revisiting this in a follow up).
>
> Bill Farner wrote:
> Unless you feel strongly, i would like to punt. I'm not too keen on building out abstractions for multiple store implementations, since multiple implementations is intended to be temporary.
I think it will be very useful in the future to have a generic mechanism for identifying slow queries, but I don't feel strongly that it needs to be part of this change (other than the fact that you've implemented it in one place). I'm fine with a ticket tracking the change for the future if you agree that it would be good to have?
- Joshua
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review82915
-----------------------------------------------------------
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 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
> 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.
Posted by Bill Farner <wf...@apache.org>.
> On May 7, 2015, 10:22 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, lines 207-208
> > <https://reviews.apache.org/r/33612/diff/2/?file=950460#file950460line207>
> >
> > Doesn't need to be javadoc.
Filled out with a comment.
> On May 7, 2015, 10:22 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, lines 102-106
> > <https://reviews.apache.org/r/33612/diff/2/?file=950462#file950462line102>
> >
> > It'd be nice if this was extracted out to a higher level so it was reusable across DB store impls without needing to be repeated in each method.
> >
> > Maybe replace the usage of the TimedInterceptor with a new DBTimedInterceptor or some such?
> >
> > (If you think this is a good idea I'm fine with just dropping a TODO and revisiting this in a follow up).
Unless you feel strongly, i would like to punt. I'm not too keen on building out abstractions for multiple store implementations, since multiple implementations is intended to be temporary.
> On May 7, 2015, 10:22 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, lines 169-174
> > <https://reviews.apache.org/r/33612/diff/2/?file=950462#file950462line169>
> >
> > Would it make sense to use a LoadingCache for this to simplify it a bit, rather than explicitly inserting on a cache miss, we could rely on the LoadingCache to either return the existing config id or insert one and return the new id?
I went down this path because of the runtime checks offered by `BiMap`, but you've convinced me that it's not worth it. Switched to LoadingCache, thanks for the suggestion!
> On May 7, 2015, 10:22 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, line 264
> > <https://reviews.apache.org/r/33612/diff/2/?file=950462#file950462line264>
> >
> > nit: kill else
Done.
> On May 7, 2015, 10:22 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java, line 127
> > <https://reviews.apache.org/r/33612/diff/2/?file=950463#file950463line127>
> >
> > This seems like it may be important to have (without it we may leave stale data behind)? Are you comfortable shipping this without properly cleaning up?
I'm not comfortable using it in production as-is, but wanted to draw the line somewhere w.r.t. patch size.
I'd also like to have a scoped discussion about what our strategy should be here, since it's a pattern that applies to several other tables. The review addressing this TODO will be a great forum for that discussion.
> On May 7, 2015, 10:22 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/TaskMapper.java, line 30
> > <https://reviews.apache.org/r/33612/diff/2/?file=950465#file950465line30>
> >
> > I'm totally unclear on our guidelines for javadoc. Usually on interface methods we'd require javadoc, does that not hold here for some reason since it's a MyBatis mapper?
Added docs. TBH i'm not certain they're valuable in this context, but we have them on other mappers so i will choose to conform.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review82915
-----------------------------------------------------------
On May 5, 2015, 6:21 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33612/
> -----------------------------------------------------------
>
> (Updated May 5, 2015, 6:21 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 ed1171d52655fef643330f34913c256f77300fa2
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3d19831ea0eb85032172b096ac87e126701aa218
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 94ce5c3499ced1b63abf19787acc21b2cd4d0c75
> 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
> 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 e3b13407cb6875489e50cf93212845eab7aacb89
> src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java f18619a27eeb2aea8dcf01e54c23ed7d1c7d3d87
> 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.
Posted by Joshua Cohen <jc...@apache.org>.
> On May 7, 2015, 10:22 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, lines 102-106
> > <https://reviews.apache.org/r/33612/diff/2/?file=950462#file950462line102>
> >
> > It'd be nice if this was extracted out to a higher level so it was reusable across DB store impls without needing to be repeated in each method.
> >
> > Maybe replace the usage of the TimedInterceptor with a new DBTimedInterceptor or some such?
> >
> > (If you think this is a good idea I'm fine with just dropping a TODO and revisiting this in a follow up).
>
> Bill Farner wrote:
> Unless you feel strongly, i would like to punt. I'm not too keen on building out abstractions for multiple store implementations, since multiple implementations is intended to be temporary.
>
> Joshua Cohen wrote:
> I think it will be very useful in the future to have a generic mechanism for identifying slow queries, but I don't feel strongly that it needs to be part of this change (other than the fact that you've implemented it in one place). I'm fine with a ticket tracking the change for the future if you agree that it would be good to have?
>
> Bill Farner wrote:
> I'm not sure i follow - you mean slow queries _in a task store_, or slow method execution?
I mean slow queries in a task store, or, more specifically what I *actually* mean is slow DB queries.
- Joshua
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review82915
-----------------------------------------------------------
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 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
> 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.
Posted by Bill Farner <wf...@apache.org>.
> On May 7, 2015, 10:22 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, lines 102-106
> > <https://reviews.apache.org/r/33612/diff/2/?file=950462#file950462line102>
> >
> > It'd be nice if this was extracted out to a higher level so it was reusable across DB store impls without needing to be repeated in each method.
> >
> > Maybe replace the usage of the TimedInterceptor with a new DBTimedInterceptor or some such?
> >
> > (If you think this is a good idea I'm fine with just dropping a TODO and revisiting this in a follow up).
>
> Bill Farner wrote:
> Unless you feel strongly, i would like to punt. I'm not too keen on building out abstractions for multiple store implementations, since multiple implementations is intended to be temporary.
>
> Joshua Cohen wrote:
> I think it will be very useful in the future to have a generic mechanism for identifying slow queries, but I don't feel strongly that it needs to be part of this change (other than the fact that you've implemented it in one place). I'm fine with a ticket tracking the change for the future if you agree that it would be good to have?
I'm not sure i follow - you mean slow queries _in a task store_, or slow method execution?
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review82915
-----------------------------------------------------------
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 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
> 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.
Posted by Bill Farner <wf...@apache.org>.
> On May 7, 2015, 10:22 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, lines 102-106
> > <https://reviews.apache.org/r/33612/diff/2/?file=950462#file950462line102>
> >
> > It'd be nice if this was extracted out to a higher level so it was reusable across DB store impls without needing to be repeated in each method.
> >
> > Maybe replace the usage of the TimedInterceptor with a new DBTimedInterceptor or some such?
> >
> > (If you think this is a good idea I'm fine with just dropping a TODO and revisiting this in a follow up).
>
> Bill Farner wrote:
> Unless you feel strongly, i would like to punt. I'm not too keen on building out abstractions for multiple store implementations, since multiple implementations is intended to be temporary.
>
> Joshua Cohen wrote:
> I think it will be very useful in the future to have a generic mechanism for identifying slow queries, but I don't feel strongly that it needs to be part of this change (other than the fact that you've implemented it in one place). I'm fine with a ticket tracking the change for the future if you agree that it would be good to have?
>
> Bill Farner wrote:
> I'm not sure i follow - you mean slow queries _in a task store_, or slow method execution?
>
> Joshua Cohen wrote:
> I mean slow queries in a task store, or, more specifically what I *actually* mean is slow DB queries.
Thanks, i've left a TODO.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review82915
-----------------------------------------------------------
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 this change.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 33612: Add a task store implementation that uses a
relational database.
Posted by Joshua Cohen <jc...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review82915
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java
<https://reviews.apache.org/r/33612/#comment133744>
Doesn't need to be javadoc.
src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java
<https://reviews.apache.org/r/33612/#comment133745>
It'd be nice if this was extracted out to a higher level so it was reusable across DB store impls without needing to be repeated in each method.
Maybe replace the usage of the TimedInterceptor with a new DBTimedInterceptor or some such?
(If you think this is a good idea I'm fine with just dropping a TODO and revisiting this in a follow up).
src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java
<https://reviews.apache.org/r/33612/#comment133757>
Would it make sense to use a LoadingCache for this to simplify it a bit, rather than explicitly inserting on a cache miss, we could rely on the LoadingCache to either return the existing config id or insert one and return the new id?
src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java
<https://reviews.apache.org/r/33612/#comment133760>
nit: kill else
src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java
<https://reviews.apache.org/r/33612/#comment133770>
This seems like it may be important to have (without it we may leave stale data behind)? Are you comfortable shipping this without properly cleaning up?
src/main/java/org/apache/aurora/scheduler/storage/db/TaskMapper.java
<https://reviews.apache.org/r/33612/#comment133772>
I'm totally unclear on our guidelines for javadoc. Usually on interface methods we'd require javadoc, does that not hold here for some reason since it's a MyBatis mapper?
- Joshua Cohen
On May 5, 2015, 6:21 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33612/
> -----------------------------------------------------------
>
> (Updated May 5, 2015, 6:21 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 ed1171d52655fef643330f34913c256f77300fa2
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3d19831ea0eb85032172b096ac87e126701aa218
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 94ce5c3499ced1b63abf19787acc21b2cd4d0c75
> 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
> 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 e3b13407cb6875489e50cf93212845eab7aacb89
> src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java f18619a27eeb2aea8dcf01e54c23ed7d1c7d3d87
> 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.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review82914
-----------------------------------------------------------
Ping?
- Bill Farner
On May 5, 2015, 6:21 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33612/
> -----------------------------------------------------------
>
> (Updated May 5, 2015, 6:21 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 ed1171d52655fef643330f34913c256f77300fa2
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3d19831ea0eb85032172b096ac87e126701aa218
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 94ce5c3499ced1b63abf19787acc21b2cd4d0c75
> 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
> 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 e3b13407cb6875489e50cf93212845eab7aacb89
> src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java f18619a27eeb2aea8dcf01e54c23ed7d1c7d3d87
> 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.
Posted by Joshua Cohen <jc...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review83229
-----------------------------------------------------------
Ship it!
src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java
<https://reviews.apache.org/r/33612/#comment134133>
s/mobule/module
- Joshua Cohen
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 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
> 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.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
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 this change.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 33612: Add a task store implementation that uses a
relational database.
Posted by Bill Farner <wf...@apache.org>.
> On May 12, 2015, 9:24 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 203
> > <https://reviews.apache.org/r/33612/diff/2-4/?file=950460#file950460line203>
> >
> > module
Doh, missed that from a previous round. Fixed.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review83487
-----------------------------------------------------------
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 this change.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 33612: Add a task store implementation that uses a
relational database.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
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 Farner
>
>
Re: Review Request 33612: Add a task store implementation that uses a
relational database.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review83525
-----------------------------------------------------------
Master (be75c36) 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: ExceptionalCommand<CAP#1>
reason: inference variable T#2 has incompatible bounds
equality constraints: ExceptionalCommand<?>
upper bounds: ExceptionalCommand<CAP#2>,T#1,Object
where T#1,E,T#2 are type-variables:
T#1 extends ExceptionalCommand<E> declared in method <E,T#1>addAction(T#1)
E extends Exception declared in method <E,T#1>addAction(T#1)
T#2 extends Object declared in method <T#2>capture(Capture<T#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: 2 mins 2.311 secs
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On May 12, 2015, 11:49 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, 11:49 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 Farner
>
>
Re: Review Request 33612: Add a task store implementation that uses a
relational database.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/
-----------------------------------------------------------
(Updated May 12, 2015, 11:49 p.m.)
Review request for Aurora, Joshua Cohen, Kevin Sweeney, and Maxim Khutornenko.
Changes
-------
Addressed comments.
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.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review83158
-----------------------------------------------------------
Ship it!
Master (1c09d58) 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 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 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
> 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.
Posted by Bill Farner <wf...@apache.org>.
> 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 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
> 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.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
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
> 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review83306
-----------------------------------------------------------
Ship it!
- Maxim Khutornenko
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 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
> 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.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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.
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 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
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.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review82558
-----------------------------------------------------------
Ship it!
Master (8a1c8bb) 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 5, 2015, 6:21 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33612/
> -----------------------------------------------------------
>
> (Updated May 5, 2015, 6:21 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 ed1171d52655fef643330f34913c256f77300fa2
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3d19831ea0eb85032172b096ac87e126701aa218
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 94ce5c3499ced1b63abf19787acc21b2cd4d0c75
> 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
> 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 e3b13407cb6875489e50cf93212845eab7aacb89
> src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java f18619a27eeb2aea8dcf01e54c23ed7d1c7d3d87
> 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.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/
-----------------------------------------------------------
(Updated May 5, 2015, 6:21 p.m.)
Review request for Aurora, Joshua Cohen, Kevin Sweeney, and Maxim Khutornenko.
Changes
-------
Fixed copy-paste error + 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 ed1171d52655fef643330f34913c256f77300fa2
src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3d19831ea0eb85032172b096ac87e126701aa218
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 94ce5c3499ced1b63abf19787acc21b2cd4d0c75
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
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 e3b13407cb6875489e50cf93212845eab7aacb89
src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java f18619a27eeb2aea8dcf01e54c23ed7d1c7d3d87
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.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review81875
-----------------------------------------------------------
Ship it!
Master (32cd1d5) 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 April 28, 2015, 8:11 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33612/
> -----------------------------------------------------------
>
> (Updated April 28, 2015, 8:11 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 82ad42fd0a626672dca80a5362fc07d804b3e412
> src/jmh/java/org/apache/aurora/benchmark/ThriftApiBenchmarks.java ed1171d52655fef643330f34913c256f77300fa2
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3d19831ea0eb85032172b096ac87e126701aa218
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 94ce5c3499ced1b63abf19787acc21b2cd4d0c75
> 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
> 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 e3b13407cb6875489e50cf93212845eab7aacb89
> src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java f18619a27eeb2aea8dcf01e54c23ed7d1c7d3d87
> 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 131
> > <https://reviews.apache.org/r/33612/diff/1/?file=944387#file944387line131>
> >
> > This feels like a potential for flakiness. Why 5? Any reason against setting it to -1 to let instances die with the tests?
>
> Bill Farner wrote:
> -1 would cause the DBs to stick around until the JVM exits, and we'd accrue one for every test **case**. I share your concern of flakiness, though. An alternative would be to push `NonVolatileStorage.stop()` up to `VolatileStorage`, and establish the pattern of invoking that during tear down. Does that sound better to you?
An explicit teardown seems more predictable and easy to reason. I recognize the ovehead it may bring to unit tests though, so I wonder what others think.
> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, line 154
> > <https://reviews.apache.org/r/33612/diff/1/?file=944389#file944389line154>
> >
> > One perf improvement here could be avoiding deleting TaskConfigs as they are going to be re-inserted right away by the following loop (a very likely scenario for something like 'killall'). In fact, I don't see how `saveTasks` could require deleting any configs at all. Perhaps leave a TODO to carve out a `deleteTasksWithoutConfigs` method?
>
> Bill Farner wrote:
> This would break behavior alignment with MemTaskStore, which i would really prefer to avoid at this phase (`saveTasks()` can technically be used as an upsert).
It's still not clear to me how upsert of a scheduled task can ever require deleting any task configs. It's an additive operation by definition that cannot possiblty render any TaskConfigs irrelevant.
> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml, line 302
> > <https://reviews.apache.org/r/33612/diff/1/?file=944402#file944402line302>
> >
> > This seems unrelated to this diff.
>
> Bill Farner wrote:
> Would you prefer i open a new review? I used this pattern in another mapper in the diff, and wanted to align on style.
Up to you. I was expecting to see some job update relevant changes when I spot this file though.
> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java, line 27
> > <https://reviews.apache.org/r/33612/diff/1/?file=944407#file944407line27>
> >
> > Can you add tests (or TODOs) for all other TaskStore methods?
>
> Bill Farner wrote:
> You mean in addition to those provided in `AbstractTaskStoreTest`?
Aha, makes sense, I did not think we had these.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review81885
-----------------------------------------------------------
On April 28, 2015, 8:11 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33612/
> -----------------------------------------------------------
>
> (Updated April 28, 2015, 8:11 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 82ad42fd0a626672dca80a5362fc07d804b3e412
> src/jmh/java/org/apache/aurora/benchmark/ThriftApiBenchmarks.java ed1171d52655fef643330f34913c256f77300fa2
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3d19831ea0eb85032172b096ac87e126701aa218
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 94ce5c3499ced1b63abf19787acc21b2cd4d0c75
> 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
> 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 e3b13407cb6875489e50cf93212845eab7aacb89
> src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java f18619a27eeb2aea8dcf01e54c23ed7d1c7d3d87
> 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.
Posted by Bill Farner <wf...@apache.org>.
> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, line 154
> > <https://reviews.apache.org/r/33612/diff/1/?file=944389#file944389line154>
> >
> > One perf improvement here could be avoiding deleting TaskConfigs as they are going to be re-inserted right away by the following loop (a very likely scenario for something like 'killall'). In fact, I don't see how `saveTasks` could require deleting any configs at all. Perhaps leave a TODO to carve out a `deleteTasksWithoutConfigs` method?
>
> Bill Farner wrote:
> This would break behavior alignment with MemTaskStore, which i would really prefer to avoid at this phase (`saveTasks()` can technically be used as an upsert).
>
> Maxim Khutornenko wrote:
> It's still not clear to me how upsert of a scheduled task can ever require deleting any task configs. It's an additive operation by definition that cannot possiblty render any TaskConfigs irrelevant.
It's effectively an upsert in practice because MemTaskStore just does a `Map.putAll`, possibly replacing values.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review81885
-----------------------------------------------------------
On April 28, 2015, 8:11 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33612/
> -----------------------------------------------------------
>
> (Updated April 28, 2015, 8:11 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 82ad42fd0a626672dca80a5362fc07d804b3e412
> src/jmh/java/org/apache/aurora/benchmark/ThriftApiBenchmarks.java ed1171d52655fef643330f34913c256f77300fa2
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3d19831ea0eb85032172b096ac87e126701aa218
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 94ce5c3499ced1b63abf19787acc21b2cd4d0c75
> 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
> 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 e3b13407cb6875489e50cf93212845eab7aacb89
> src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java f18619a27eeb2aea8dcf01e54c23ed7d1c7d3d87
> 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.
Posted by Bill Farner <wf...@apache.org>.
> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 131
> > <https://reviews.apache.org/r/33612/diff/1/?file=944387#file944387line131>
> >
> > This feels like a potential for flakiness. Why 5? Any reason against setting it to -1 to let instances die with the tests?
-1 would cause the DBs to stick around until the JVM exits, and we'd accrue one for every test **case**. I share your concern of flakiness, though. An alternative would be to push `NonVolatileStorage.stop()` up to `VolatileStorage`, and establish the pattern of invoking that during tear down. Does that sound better to you?
> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, line 154
> > <https://reviews.apache.org/r/33612/diff/1/?file=944389#file944389line154>
> >
> > One perf improvement here could be avoiding deleting TaskConfigs as they are going to be re-inserted right away by the following loop (a very likely scenario for something like 'killall'). In fact, I don't see how `saveTasks` could require deleting any configs at all. Perhaps leave a TODO to carve out a `deleteTasksWithoutConfigs` method?
This would break behavior alignment with MemTaskStore, which i would really prefer to avoid at this phase (`saveTasks()` can technically be used as an upsert).
> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java, line 119
> > <https://reviews.apache.org/r/33612/diff/1/?file=944390#file944390line119>
> >
> > I'd vote for explicit control. If `deleteTasks` is not called from `saveTasks` (see my comment above), the only caller is going to be the TaskHistoryPruner. Given that's a background task, perf impact here is less of an issue.
Yeah, i plan to have this discussion more formally, as i'd like to agree on the pattern. I'm much less worried about performance than correctness - managing all these reference counts is going to be a sizeable chore.
> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml, line 302
> > <https://reviews.apache.org/r/33612/diff/1/?file=944402#file944402line302>
> >
> > This seems unrelated to this diff.
Would you prefer i open a new review? I used this pattern in another mapper in the diff, and wanted to align on style.
> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml, lines 161-163
> > <https://reviews.apache.org/r/33612/diff/1/?file=944403#file944403line161>
> >
> > Would it make sense to explore collection sub-selects instead? Having executor_data repeated many times per match could potentially damage perf. If you agree, please leave a TODO.
I agree that it's reasonably likely to be an outcome, but i'd like to avoid dropping too many perf TODOs at this point so as to let the benchmarks speak for themselves.
> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java, line 27
> > <https://reviews.apache.org/r/33612/diff/1/?file=944407#file944407line27>
> >
> > Can you add tests (or TODOs) for all other TaskStore methods?
You mean in addition to those provided in `AbstractTaskStoreTest`?
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review81885
-----------------------------------------------------------
On April 28, 2015, 8:11 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33612/
> -----------------------------------------------------------
>
> (Updated April 28, 2015, 8:11 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 82ad42fd0a626672dca80a5362fc07d804b3e412
> src/jmh/java/org/apache/aurora/benchmark/ThriftApiBenchmarks.java ed1171d52655fef643330f34913c256f77300fa2
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3d19831ea0eb85032172b096ac87e126701aa218
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 94ce5c3499ced1b63abf19787acc21b2cd4d0c75
> 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
> 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 e3b13407cb6875489e50cf93212845eab7aacb89
> src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java f18619a27eeb2aea8dcf01e54c23ed7d1c7d3d87
> 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33612/#review81885
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java
<https://reviews.apache.org/r/33612/#comment132404>
This feels like a potential for flakiness. Why 5? Any reason against setting it to -1 to let instances die with the tests?
src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java
<https://reviews.apache.org/r/33612/#comment132417>
One perf improvement here could be avoiding deleting TaskConfigs as they are going to be re-inserted right away by the following loop (a very likely scenario for something like 'killall'). In fact, I don't see how `saveTasks` could require deleting any configs at all. Perhaps leave a TODO to carve out a `deleteTasksWithoutConfigs` method?
src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java
<https://reviews.apache.org/r/33612/#comment132437>
I'd vote for explicit control. If `deleteTasks` is not called from `saveTasks` (see my comment above), the only caller is going to be the TaskHistoryPruner. Given that's a background task, perf impact here is less of an issue.
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml
<https://reviews.apache.org/r/33612/#comment132430>
This seems unrelated to this diff.
src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
<https://reviews.apache.org/r/33612/#comment132434>
Would it make sense to explore collection sub-selects instead? Having executor_data repeated many times per match could potentially damage perf. If you agree, please leave a TODO.
src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java
<https://reviews.apache.org/r/33612/#comment132438>
Can you add tests (or TODOs) for all other TaskStore methods?
- Maxim Khutornenko
On April 28, 2015, 8:11 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33612/
> -----------------------------------------------------------
>
> (Updated April 28, 2015, 8:11 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 82ad42fd0a626672dca80a5362fc07d804b3e412
> src/jmh/java/org/apache/aurora/benchmark/ThriftApiBenchmarks.java ed1171d52655fef643330f34913c256f77300fa2
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3d19831ea0eb85032172b096ac87e126701aa218
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 94ce5c3499ced1b63abf19787acc21b2cd4d0c75
> 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
> 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 e3b13407cb6875489e50cf93212845eab7aacb89
> src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java f18619a27eeb2aea8dcf01e54c23ed7d1c7d3d87
> 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
>
>