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 2014/09/11 06:53:05 UTC
Review Request 25529: Add a controller for job updates.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/
-----------------------------------------------------------
Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji.
Bugs: AURORA-613
https://issues.apache.org/jira/browse/AURORA-613
Repository: aurora
Description
-------
There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
Diffs
-----
build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 8befecaf4f13c0b890b452bc7b9c0618725b8c41
src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java b894a71348d2d71c077f35bb21a80ba88a6b4c42
src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 599dbd8bb762d051b75aa1a1e4d6a4c90ca6eb87
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java ec9b37ccaf03651e986aab9b4541f17ff0540008
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
src/main/thrift/org/apache/aurora/gen/api.thrift c00f94371a27ffab41188b22f81bb1c8ec7a57e9
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 0be4d78dd737b34f8a58276be9ffd7aeaa7f95bb
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e
src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/InstanceActionHandlerTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
Diff: https://reviews.apache.org/r/25529/diff/
Testing
-------
./gradlew build -Pq
Jacoco reports 98% line coverage, 96% branch coverage for the updater package.
Thanks,
Bill Farner
Re: Review Request 25529: Add a controller for job updates.
Posted by Bill Farner <wf...@apache.org>.
> On Sept. 15, 2014, 11:13 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java, line 61
> > <https://reviews.apache.org/r/25529/diff/3/?file=689761#file689761line61>
> >
> > TODO to use AssistedInject to generate this factory here? https://github.com/google/guice/wiki/AssistedInject
Comment was dropped here - TODO added.
> On Sept. 15, 2014, 11:13 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java, line 95
> > <https://reviews.apache.org/r/25529/diff/3/?file=689756#file689756line95>
> >
> > Is this just Guava Service#start()? Maybe implement that interface (by extending AbstractIdleService) here instead so that we get state monitoring and error detection for free, as well as future integration with AURORA-324.
>
> Bill Farner wrote:
> Sure.
I take that back - tried this out, and there are some complications that i'd like to establish a pattern for before proceeding.
The biggest issue is that an exception surfacing from Service.startUp() results in a hang (if you follow the startAsync().awaitRunning() pattern). This makes for difficult unit testing where you want things like AssertionErrors to surface. I've left a TODO for now.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/#review53389
-----------------------------------------------------------
On Sept. 15, 2014, 7:40 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25529/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2014, 7:40 p.m.)
>
>
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji.
>
>
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
>
> The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
>
> As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
>
>
> Diffs
> -----
>
> build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
> src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
> src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
> src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
> src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java f9521f9599e2d26cf594d62fc6b6f6ca3d5fb108
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 6a97f36a69c7723ec1c3730085d0df70f94dcac8
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c520e1378c4b7947bb866011027b79b4ab65547c
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 87b773ba80784e4af8cdbbbb78790c373c08e26e
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fedba15792ff2ecf55922ce5c38b85ada2a9edf4
> src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
> src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
> src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
> src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
> src/main/thrift/org/apache/aurora/gen/api.thrift a6dc548c804bfcb9166573496023bad80b2a2c91
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java af644a967983b8da66231390d29839a034697852
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java cf9805155f0e17b75db9ef8edd4b805826107868
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/EnumsTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
> src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
> src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
>
> Diff: https://reviews.apache.org/r/25529/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> Jacoco reports 100% line coverage, 98% branch coverage for the updater package.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 25529: Add a controller for job updates.
Posted by Bill Farner <wf...@apache.org>.
> On Sept. 15, 2014, 11:13 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1368
> > <https://reviews.apache.org/r/25529/diff/3/?file=689752#file689752line1368>
> >
> > If I'm reading correctly there's no need to make this final
Fixed.
> On Sept. 15, 2014, 11:13 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 30
> > <https://reviews.apache.org/r/25529/diff/3/?file=689754#file689754line30>
> >
> > Historically we've injected Storage and used the transaction API (and storeProvider.getTaskStore()) here, any reason to deviate from that?
Two primary reasons:
- it minimizes the dependency exposure to InstanceActionHandler
- it clarifies that the function is already called in the context of a transaction
> On Sept. 15, 2014, 11:13 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java, line 95
> > <https://reviews.apache.org/r/25529/diff/3/?file=689756#file689756line95>
> >
> > Is this just Guava Service#start()? Maybe implement that interface (by extending AbstractIdleService) here instead so that we get state monitoring and error detection for free, as well as future integration with AURORA-324.
Sure.
> On Sept. 15, 2014, 11:13 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 225
> > <https://reviews.apache.org/r/25529/diff/3/?file=689757#file689757line225>
> >
> > Factor out an InstanceKeys.canonicalString?
Went with InstanceKeys.toString, since canonicalString bears more weight than i'm interested in here.
> On Sept. 15, 2014, 11:13 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java, line 104
> > <https://reviews.apache.org/r/25529/diff/3/?file=689758#file689758line104>
> >
> > This seems like a fatal startup error - should the scheduler process abort here?
I'm torn here. While i generally agree, this would mean hitting this condition is always dire, and likely requires code or data forensics to fix. However, if we handle the situation gracefully, it's far less critical.
In other words, if i'm carrying the pager, i want this graceful degradation.
I've added stats to at least enable alerting here.
> On Sept. 15, 2014, 11:13 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java, line 86
> > <https://reviews.apache.org/r/25529/diff/3/?file=689760#file689760line86>
> >
> > redundant use of toString
Removed.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/#review53389
-----------------------------------------------------------
On Sept. 15, 2014, 7:40 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25529/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2014, 7:40 p.m.)
>
>
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji.
>
>
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
>
> The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
>
> As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
>
>
> Diffs
> -----
>
> build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
> src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
> src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
> src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
> src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java f9521f9599e2d26cf594d62fc6b6f6ca3d5fb108
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 6a97f36a69c7723ec1c3730085d0df70f94dcac8
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c520e1378c4b7947bb866011027b79b4ab65547c
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 87b773ba80784e4af8cdbbbb78790c373c08e26e
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fedba15792ff2ecf55922ce5c38b85ada2a9edf4
> src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
> src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
> src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
> src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
> src/main/thrift/org/apache/aurora/gen/api.thrift a6dc548c804bfcb9166573496023bad80b2a2c91
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java af644a967983b8da66231390d29839a034697852
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java cf9805155f0e17b75db9ef8edd4b805826107868
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/EnumsTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
> src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
> src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
>
> Diff: https://reviews.apache.org/r/25529/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> Jacoco reports 100% line coverage, 98% branch coverage for the updater package.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 25529: Add a controller for job updates.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/#review53389
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
<https://reviews.apache.org/r/25529/#comment93034>
If I'm reading correctly there's no need to make this final
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java
<https://reviews.apache.org/r/25529/#comment93074>
Historically we've injected Storage and used the transaction API (and storeProvider.getTaskStore()) here, any reason to deviate from that?
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java
<https://reviews.apache.org/r/25529/#comment93084>
Is this just Guava Service#start()? Maybe implement that interface (by extending AbstractIdleService) here instead so that we get state monitoring and error detection for free, as well as future integration with AURORA-324.
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment93076>
Factor out an InstanceKeys.canonicalString?
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java
<https://reviews.apache.org/r/25529/#comment93082>
This seems like a fatal startup error - should the scheduler process abort here?
src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java
<https://reviews.apache.org/r/25529/#comment93085>
redundant use of toString
src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java
<https://reviews.apache.org/r/25529/#comment93086>
TODO to use AssistedInject to generate this factory here? https://github.com/google/guice/wiki/AssistedInject
- Kevin Sweeney
On Sept. 15, 2014, 12:40 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25529/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2014, 12:40 p.m.)
>
>
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji.
>
>
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
>
> The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
>
> As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
>
>
> Diffs
> -----
>
> build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
> src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
> src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
> src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
> src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java f9521f9599e2d26cf594d62fc6b6f6ca3d5fb108
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 6a97f36a69c7723ec1c3730085d0df70f94dcac8
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c520e1378c4b7947bb866011027b79b4ab65547c
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 87b773ba80784e4af8cdbbbb78790c373c08e26e
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fedba15792ff2ecf55922ce5c38b85ada2a9edf4
> src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
> src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
> src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
> src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
> src/main/thrift/org/apache/aurora/gen/api.thrift a6dc548c804bfcb9166573496023bad80b2a2c91
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java af644a967983b8da66231390d29839a034697852
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java cf9805155f0e17b75db9ef8edd4b805826107868
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/EnumsTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
> src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
> src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
>
> Diff: https://reviews.apache.org/r/25529/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> Jacoco reports 100% line coverage, 98% branch coverage for the updater package.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 25529: Add a controller for job updates.
Posted by Bill Farner <wf...@apache.org>.
> On Sept. 15, 2014, 11:01 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1368
> > <https://reviews.apache.org/r/25529/diff/3/?file=689752#file689752line1368>
> >
> > Is this a leftover from some intermediate work?
Not sure how that made it in. Removed.
> On Sept. 15, 2014, 11:01 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 81
> > <https://reviews.apache.org/r/25529/diff/3/?file=689754#file689754line81>
> >
> > Some more context to this log message might be helpful (akin to the KillTask log? `"Adding " + instance + " while " + status`
Done.
> On Sept. 15, 2014, 11:01 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, lines 123-124
> > <https://reviews.apache.org/r/25529/diff/3/?file=689757#file689757line123>
> >
> > Does it make sense to separate the config validation logic from the UpdateFactory creation?
I'm slightly hesitatnt here. There's non-trivial relationships between the fields in JobUpdateConfiguration, and i'm failing to extract the validation out in a way that doesn't leave me with 3 helper functions, some redundancy, and less certainty that the gates are in place.
I'd also like to see if the TODO about moving the logic to SchedulerThriftInterface pans out.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/#review53425
-----------------------------------------------------------
On Sept. 15, 2014, 7:40 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25529/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2014, 7:40 p.m.)
>
>
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji.
>
>
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
>
> The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
>
> As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
>
>
> Diffs
> -----
>
> build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
> src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
> src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
> src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
> src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java f9521f9599e2d26cf594d62fc6b6f6ca3d5fb108
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 6a97f36a69c7723ec1c3730085d0df70f94dcac8
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c520e1378c4b7947bb866011027b79b4ab65547c
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 87b773ba80784e4af8cdbbbb78790c373c08e26e
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fedba15792ff2ecf55922ce5c38b85ada2a9edf4
> src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
> src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
> src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
> src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
> src/main/thrift/org/apache/aurora/gen/api.thrift a6dc548c804bfcb9166573496023bad80b2a2c91
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java af644a967983b8da66231390d29839a034697852
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java cf9805155f0e17b75db9ef8edd4b805826107868
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/EnumsTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
> src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
> src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
>
> Diff: https://reviews.apache.org/r/25529/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> Jacoco reports 100% line coverage, 98% branch coverage for the updater package.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 25529: Add a controller for job updates.
Posted by Joshua Cohen <jc...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/#review53425
-----------------------------------------------------------
Ship it!
Looks good to me, just a few minor questions/nits:
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
<https://reviews.apache.org/r/25529/#comment93079>
Is this a leftover from some intermediate work?
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java
<https://reviews.apache.org/r/25529/#comment93080>
Some more context to this log message might be helpful (akin to the KillTask log? `"Adding " + instance + " while " + status`
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment93083>
Does it make sense to separate the config validation logic from the UpdateFactory creation?
- Joshua Cohen
On Sept. 15, 2014, 7:40 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25529/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2014, 7:40 p.m.)
>
>
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji.
>
>
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
>
> The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
>
> As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
>
>
> Diffs
> -----
>
> build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
> src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
> src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
> src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
> src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java f9521f9599e2d26cf594d62fc6b6f6ca3d5fb108
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 6a97f36a69c7723ec1c3730085d0df70f94dcac8
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c520e1378c4b7947bb866011027b79b4ab65547c
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 87b773ba80784e4af8cdbbbb78790c373c08e26e
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fedba15792ff2ecf55922ce5c38b85ada2a9edf4
> src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
> src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
> src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
> src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
> src/main/thrift/org/apache/aurora/gen/api.thrift a6dc548c804bfcb9166573496023bad80b2a2c91
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java af644a967983b8da66231390d29839a034697852
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java cf9805155f0e17b75db9ef8edd4b805826107868
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/EnumsTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
> src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
> src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
>
> Diff: https://reviews.apache.org/r/25529/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> Jacoco reports 100% line coverage, 98% branch coverage for the updater package.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 25529: Add a controller for job updates.
Posted by Bill Farner <wf...@apache.org>.
> On Sept. 16, 2014, 8:40 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java, line 22
> > <https://reviews.apache.org/r/25529/diff/4/?file=690154#file690154line22>
> >
> > Missing javadoc for this class and its public methods?
Done.
> On Sept. 16, 2014, 8:40 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java, line 29
> > <https://reviews.apache.org/r/25529/diff/4/?file=690154#file690154line29>
> >
> > checkArgument(instanceId >= 0)?
Done.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/#review53581
-----------------------------------------------------------
On Sept. 16, 2014, 9:10 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25529/
> -----------------------------------------------------------
>
> (Updated Sept. 16, 2014, 9:10 p.m.)
>
>
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, and Zameer Manji.
>
>
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
>
> The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
>
> As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
>
>
> Diffs
> -----
>
> build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
> src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
> src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
> src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
> src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java f9521f9599e2d26cf594d62fc6b6f6ca3d5fb108
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 6a97f36a69c7723ec1c3730085d0df70f94dcac8
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c520e1378c4b7947bb866011027b79b4ab65547c
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 87b773ba80784e4af8cdbbbb78790c373c08e26e
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fedba15792ff2ecf55922ce5c38b85ada2a9edf4
> src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
> src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
> src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
> src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
> src/main/thrift/org/apache/aurora/gen/api.thrift a6dc548c804bfcb9166573496023bad80b2a2c91
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java af644a967983b8da66231390d29839a034697852
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java cf9805155f0e17b75db9ef8edd4b805826107868
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/EnumsTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
> src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
> src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
>
> Diff: https://reviews.apache.org/r/25529/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> Jacoco reports 100% line coverage, 98% branch coverage for the updater package.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 25529: Add a controller for job updates.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/#review53581
-----------------------------------------------------------
Ship it!
src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java
<https://reviews.apache.org/r/25529/#comment93295>
Missing javadoc for this class and its public methods?
src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java
<https://reviews.apache.org/r/25529/#comment93296>
checkArgument(instanceId >= 0)?
- Kevin Sweeney
On Sept. 15, 2014, 5:39 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25529/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2014, 5:39 p.m.)
>
>
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji.
>
>
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
>
> The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
>
> As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
>
>
> Diffs
> -----
>
> build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
> src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
> src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
> src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
> src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java f9521f9599e2d26cf594d62fc6b6f6ca3d5fb108
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 6a97f36a69c7723ec1c3730085d0df70f94dcac8
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c520e1378c4b7947bb866011027b79b4ab65547c
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 87b773ba80784e4af8cdbbbb78790c373c08e26e
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fedba15792ff2ecf55922ce5c38b85ada2a9edf4
> src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
> src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
> src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
> src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
> src/main/thrift/org/apache/aurora/gen/api.thrift a6dc548c804bfcb9166573496023bad80b2a2c91
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java af644a967983b8da66231390d29839a034697852
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java cf9805155f0e17b75db9ef8edd4b805826107868
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/EnumsTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
> src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
> src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
>
> Diff: https://reviews.apache.org/r/25529/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> Jacoco reports 100% line coverage, 98% branch coverage for the updater package.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 25529: Add a controller for job updates.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/
-----------------------------------------------------------
(Updated Sept. 16, 2014, 9:27 p.m.)
Review request for Aurora, Joshua Cohen, Kevin Sweeney, and Zameer Manji.
Bugs: AURORA-613
https://issues.apache.org/jira/browse/AURORA-613
Repository: aurora
Description
-------
There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
Diffs (updated)
-----
build.gradle e92b27d734fb6dc9c81c39ec8cb680ef179c3695
src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/base/Jobs.java 744d2b694dbcf329e67e44db206b5f7381056dbe
src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java f9521f9599e2d26cf594d62fc6b6f6ca3d5fb108
src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 6a97f36a69c7723ec1c3730085d0df70f94dcac8
src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c520e1378c4b7947bb866011027b79b4ab65547c
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 87b773ba80784e4af8cdbbbb78790c373c08e26e
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java dc0169ac727190028fd87c640af0100d4e3ce694
src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
src/main/thrift/org/apache/aurora/gen/api.thrift 8396e3d247b82a205b599474ee02819b695bf3d8
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java af644a967983b8da66231390d29839a034697852
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 820a968672e33b18b7f00d7346c9c7d7963fce0e
src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/EnumsTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
Diff: https://reviews.apache.org/r/25529/diff/
Testing
-------
./gradlew build -Pq
Jacoco reports 100% line coverage, 98% branch coverage for the updater package.
Thanks,
Bill Farner
Re: Review Request 25529: Add a controller for job updates.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/
-----------------------------------------------------------
(Updated Sept. 16, 2014, 9:10 p.m.)
Review request for Aurora, Joshua Cohen, Kevin Sweeney, and Zameer Manji.
Changes
-------
People -= zmanji, who asked me to tap out of the review.
Bugs: AURORA-613
https://issues.apache.org/jira/browse/AURORA-613
Repository: aurora
Description
-------
There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
Diffs
-----
build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java f9521f9599e2d26cf594d62fc6b6f6ca3d5fb108
src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 6a97f36a69c7723ec1c3730085d0df70f94dcac8
src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c520e1378c4b7947bb866011027b79b4ab65547c
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 87b773ba80784e4af8cdbbbb78790c373c08e26e
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fedba15792ff2ecf55922ce5c38b85ada2a9edf4
src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
src/main/thrift/org/apache/aurora/gen/api.thrift a6dc548c804bfcb9166573496023bad80b2a2c91
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java af644a967983b8da66231390d29839a034697852
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java cf9805155f0e17b75db9ef8edd4b805826107868
src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/EnumsTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
Diff: https://reviews.apache.org/r/25529/diff/
Testing
-------
./gradlew build -Pq
Jacoco reports 100% line coverage, 98% branch coverage for the updater package.
Thanks,
Bill Farner
Re: Review Request 25529: Add a controller for job updates.
Posted by Zameer Manji <zm...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/#review53598
-----------------------------------------------------------
I'm tapping out of this review. I don't have enough knowledge to give a ship to this change.
- Zameer Manji
On Sept. 15, 2014, 5:39 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25529/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2014, 5:39 p.m.)
>
>
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji.
>
>
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
>
> The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
>
> As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
>
>
> Diffs
> -----
>
> build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
> src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
> src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
> src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
> src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java f9521f9599e2d26cf594d62fc6b6f6ca3d5fb108
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 6a97f36a69c7723ec1c3730085d0df70f94dcac8
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c520e1378c4b7947bb866011027b79b4ab65547c
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 87b773ba80784e4af8cdbbbb78790c373c08e26e
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fedba15792ff2ecf55922ce5c38b85ada2a9edf4
> src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
> src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
> src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
> src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
> src/main/thrift/org/apache/aurora/gen/api.thrift a6dc548c804bfcb9166573496023bad80b2a2c91
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java af644a967983b8da66231390d29839a034697852
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java cf9805155f0e17b75db9ef8edd4b805826107868
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/EnumsTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
> src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
> src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
>
> Diff: https://reviews.apache.org/r/25529/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> Jacoco reports 100% line coverage, 98% branch coverage for the updater package.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 25529: Add a controller for job updates.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/
-----------------------------------------------------------
(Updated Sept. 16, 2014, 12:39 a.m.)
Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji.
Bugs: AURORA-613
https://issues.apache.org/jira/browse/AURORA-613
Repository: aurora
Description
-------
There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
Diffs (updated)
-----
build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java f9521f9599e2d26cf594d62fc6b6f6ca3d5fb108
src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 6a97f36a69c7723ec1c3730085d0df70f94dcac8
src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c520e1378c4b7947bb866011027b79b4ab65547c
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 87b773ba80784e4af8cdbbbb78790c373c08e26e
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fedba15792ff2ecf55922ce5c38b85ada2a9edf4
src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
src/main/thrift/org/apache/aurora/gen/api.thrift a6dc548c804bfcb9166573496023bad80b2a2c91
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java af644a967983b8da66231390d29839a034697852
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java cf9805155f0e17b75db9ef8edd4b805826107868
src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/EnumsTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
Diff: https://reviews.apache.org/r/25529/diff/
Testing
-------
./gradlew build -Pq
Jacoco reports 100% line coverage, 98% branch coverage for the updater package.
Thanks,
Bill Farner
Re: Review Request 25529: Add a controller for job updates.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/
-----------------------------------------------------------
(Updated Sept. 15, 2014, 7:40 p.m.)
Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji.
Changes
-------
Rebased on master to address TODOs thanks to https://reviews.apache.org/r/25653/
Bugs: AURORA-613
https://issues.apache.org/jira/browse/AURORA-613
Repository: aurora
Description
-------
There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
Diffs (updated)
-----
build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java f9521f9599e2d26cf594d62fc6b6f6ca3d5fb108
src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 6a97f36a69c7723ec1c3730085d0df70f94dcac8
src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c520e1378c4b7947bb866011027b79b4ab65547c
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 87b773ba80784e4af8cdbbbb78790c373c08e26e
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fedba15792ff2ecf55922ce5c38b85ada2a9edf4
src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
src/main/thrift/org/apache/aurora/gen/api.thrift a6dc548c804bfcb9166573496023bad80b2a2c91
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java af644a967983b8da66231390d29839a034697852
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java cf9805155f0e17b75db9ef8edd4b805826107868
src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/EnumsTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
Diff: https://reviews.apache.org/r/25529/diff/
Testing
-------
./gradlew build -Pq
Jacoco reports 100% line coverage, 98% branch coverage for the updater package.
Thanks,
Bill Farner
Re: Review Request 25529: Add a controller for job updates.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/
-----------------------------------------------------------
(Updated Sept. 12, 2014, 10:54 p.m.)
Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji.
Bugs: AURORA-613
https://issues.apache.org/jira/browse/AURORA-613
Repository: aurora
Description
-------
There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
Diffs (updated)
-----
build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java f9521f9599e2d26cf594d62fc6b6f6ca3d5fb108
src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java b894a71348d2d71c077f35bb21a80ba88a6b4c42
src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 599dbd8bb762d051b75aa1a1e4d6a4c90ca6eb87
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java ec9b37ccaf03651e986aab9b4541f17ff0540008
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fedba15792ff2ecf55922ce5c38b85ada2a9edf4
src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
src/main/thrift/org/apache/aurora/gen/api.thrift a6dc548c804bfcb9166573496023bad80b2a2c91
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 0be4d78dd737b34f8a58276be9ffd7aeaa7f95bb
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java cf9805155f0e17b75db9ef8edd4b805826107868
src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/EnumsTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
Diff: https://reviews.apache.org/r/25529/diff/
Testing (updated)
-------
./gradlew build -Pq
Jacoco reports 100% line coverage, 98% branch coverage for the updater package.
Thanks,
Bill Farner
Re: Review Request 25529: Add a controller for job updates.
Posted by Bill Farner <wf...@apache.org>.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> >
>
> Maxim Khutornenko wrote:
> Non-blocking but I was still waiting on my comments/questions addressed.
Wow, sorry about that - i truly thought Kevin was the only remaining ship-it needed :-( Now that we have a straw-man put together, let's iterate on any warts (and feel free to call me out on things that still deserve attention).
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/#review53051
-----------------------------------------------------------
On Sept. 16, 2014, 9:27 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25529/
> -----------------------------------------------------------
>
> (Updated Sept. 16, 2014, 9:27 p.m.)
>
>
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, and Zameer Manji.
>
>
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
>
> The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
>
> As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
>
>
> Diffs
> -----
>
> build.gradle e92b27d734fb6dc9c81c39ec8cb680ef179c3695
> src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
> src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/base/Jobs.java 744d2b694dbcf329e67e44db206b5f7381056dbe
> src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
> src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java f9521f9599e2d26cf594d62fc6b6f6ca3d5fb108
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 6a97f36a69c7723ec1c3730085d0df70f94dcac8
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c520e1378c4b7947bb866011027b79b4ab65547c
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 87b773ba80784e4af8cdbbbb78790c373c08e26e
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java dc0169ac727190028fd87c640af0100d4e3ce694
> src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
> src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
> src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
> src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
> src/main/thrift/org/apache/aurora/gen/api.thrift 8396e3d247b82a205b599474ee02819b695bf3d8
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java af644a967983b8da66231390d29839a034697852
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 820a968672e33b18b7f00d7346c9c7d7963fce0e
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/EnumsTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
> src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
> src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
>
> Diff: https://reviews.apache.org/r/25529/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> Jacoco reports 100% line coverage, 98% branch coverage for the updater package.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 25529: Add a controller for job updates.
Posted by Bill Farner <wf...@apache.org>.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Query.java, line 104
> > <https://reviews.apache.org/r/25529/diff/1/?file=685004#file685004line104>
> >
> > typo
Better yet - addressed TODO :-)
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1428
> > <https://reviews.apache.org/r/25529/diff/1/?file=685010#file685010line1428>
> >
> > Consider discarding in favor of https://reviews.apache.org/r/25481/
Negatory - i still need the exception to flag when 'updateOnlyTheseInstances' is invalid. Though, i think all this validation should live together. Added a TODO.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 112
> > <https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line112>
> >
> > Should it rather be named ADD_TASK...? It's used for both update (replace) and add new instance but since KILL is separate feels like "ADD" should be more appropriate here (i.e. it only inserts pending and does not do any killing).
Sure, done.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 113
> > <https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line113>
> >
> > Replace with instanceName.
Obviated by other changes.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 118
> > <https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line118>
> >
> > Will quota checking come later or you'd rather see AURORA-686 fixed instead?
Good question. I've added a TODO, we should discuss later which should come first.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, lines 123-124
> > <https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line123>
> >
> > This will throw unnecesserily if the instance is not in active state. I'd rather see an inactive instance get killed.
That's a precondition - this action should not be taken if the instance is not active.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 131
> > <https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line131>
> >
> > This value is user-controlled and might be set too low for the kill to succeed. Would it rather make sense to fix it internally (as it is now on the client)?
I think a lower bound when validating is reasonable, but i don't like the idea of changing it silently.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java, line 79
> > <https://reviews.apache.org/r/25529/diff/1/?file=685013#file685013line79>
> >
> > Missing docs.
Added.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 122
> > <https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line122>
> >
> > typo
Fixed.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 187
> > <https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line187>
> >
> > Why not just inline Functions.forMap(ImmutableMap.of(ABORTED, ABORTED)) instead?
Because the output should _always_ be ABORTED. That map would mean "you can only go from aborted to aborted."
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 398
> > <https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line398>
> >
> > Spent quite a bit of time reading this method. Would definitely benefit from refactoring. How about at least moving everything down from here to something like maybeEvaluateUpdater()?
I wasn't able to follow your request, can you give some pseudocode to illustrate?
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 408
> > <https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line408>
> >
> > Mind leaving a TODO here to create a getJobUpdate() storage API instead?
Done.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 444
> > <https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line444>
> >
> > Is active() deliberate here? If so, perhaps rename the function to "getActiveInstance" to clearly state its purpose?
It's critical, we don't want to evaluate exited tasks. Changed the name.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 493
> > <https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line493>
> >
> > This seems unconditional. Where does rollbackOnFailure get evaluated?
This changed with a comment above, likely obviating the comment.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 496
> > <https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line496>
> >
> > Would it make sense to have a catch-all else block here to log in case actions is empty?
SGTM, done.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java, line 83
> > <https://reviews.apache.org/r/25529/diff/1/?file=685015#file685015line83>
> >
> > Logging try...catch around similar to taskChangedState()?
Done.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java, line 97
> > <https://reviews.apache.org/r/25529/diff/1/?file=685015#file685015line97>
> >
> > Same here. System resume is notoriously hard to troubleshoot and we probably don't want to let it out uncaught.
Done.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java, line 137
> > <https://reviews.apache.org/r/25529/diff/1/?file=685032#file685032line137>
> >
> > s/time.s/times.
Fixed.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/#review53051
-----------------------------------------------------------
On Sept. 11, 2014, 4:53 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25529/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2014, 4:53 a.m.)
>
>
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji.
>
>
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
>
> The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
>
> As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
>
>
> Diffs
> -----
>
> build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
> src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
> src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
> src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
> src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 8befecaf4f13c0b890b452bc7b9c0618725b8c41
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java b894a71348d2d71c077f35bb21a80ba88a6b4c42
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 599dbd8bb762d051b75aa1a1e4d6a4c90ca6eb87
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java ec9b37ccaf03651e986aab9b4541f17ff0540008
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
> src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
> src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
> src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
> src/main/thrift/org/apache/aurora/gen/api.thrift c00f94371a27ffab41188b22f81bb1c8ec7a57e9
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 0be4d78dd737b34f8a58276be9ffd7aeaa7f95bb
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceActionHandlerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
> src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
> src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
>
> Diff: https://reviews.apache.org/r/25529/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> Jacoco reports 98% line coverage, 96% branch coverage for the updater package.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 25529: Add a controller for job updates.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 398
> > <https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line398>
> >
> > Spent quite a bit of time reading this method. Would definitely benefit from refactoring. How about at least moving everything down from here to something like maybeEvaluateUpdater()?
>
> Bill Farner wrote:
> I wasn't able to follow your request, can you give some pseudocode to illustrate?
Everything down from here deals with processing a MonitorAction acquired on this line and can technically be moved into its own method. That would reduce the recordAndChangeJobUpdateStatus() length and make it a bit more readable (i.e. easier to reason about the recursive way this funciton may be invoked).
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 131
> > <https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line131>
> >
> > This value is user-controlled and might be set too low for the kill to succeed. Would it rather make sense to fix it internally (as it is now on the client)?
>
> Bill Farner wrote:
> I think a lower bound when validating is reasonable, but i don't like the idea of changing it silently.
A lower bound validation would not quite solve the problem as it still may not be enough to kill a task. I have routinely observed task kills taking longer than the default watch_secs value of 45 seconds. The current client timeout is set to 2.5 minutes of binary backoff countdown. I feel we should follow the same approach on the scheduler and perhaps expose an Arg to make it configurable.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, lines 123-124
> > <https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line123>
> >
> > This will throw unnecesserily if the instance is not in active state. I'd rather see an inactive instance get killed.
>
> Bill Farner wrote:
> That's a precondition - this action should not be taken if the instance is not active.
Is there anything upstream that ensures the instance is still active though? Is that check part of the same transaction? I am concerned about the race between instance state and this query. Perhaps it's just safer to proceed with instance kill, which will be a no-op in this case.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 493
> > <https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line493>
> >
> > This seems unconditional. Where does rollbackOnFailure get evaluated?
>
> Bill Farner wrote:
> This changed with a comment above, likely obviating the comment.
I meant to point that ROLLING_BACK should be conditional upon the user-specified rollbackOnFailure config value. I could not find where it is honored.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/#review53051
-----------------------------------------------------------
On Sept. 12, 2014, 10:54 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25529/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2014, 10:54 p.m.)
>
>
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji.
>
>
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
>
> The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
>
> As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
>
>
> Diffs
> -----
>
> build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
> src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
> src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
> src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
> src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java f9521f9599e2d26cf594d62fc6b6f6ca3d5fb108
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java b894a71348d2d71c077f35bb21a80ba88a6b4c42
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 599dbd8bb762d051b75aa1a1e4d6a4c90ca6eb87
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java ec9b37ccaf03651e986aab9b4541f17ff0540008
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fedba15792ff2ecf55922ce5c38b85ada2a9edf4
> src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
> src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
> src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
> src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
> src/main/thrift/org/apache/aurora/gen/api.thrift a6dc548c804bfcb9166573496023bad80b2a2c91
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 0be4d78dd737b34f8a58276be9ffd7aeaa7f95bb
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java cf9805155f0e17b75db9ef8edd4b805826107868
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/EnumsTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
> src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
> src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
>
> Diff: https://reviews.apache.org/r/25529/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> Jacoco reports 100% line coverage, 98% branch coverage for the updater package.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 25529: Add a controller for job updates.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> >
Non-blocking but I was still waiting on my comments/questions addressed.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/#review53051
-----------------------------------------------------------
On Sept. 16, 2014, 9:27 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25529/
> -----------------------------------------------------------
>
> (Updated Sept. 16, 2014, 9:27 p.m.)
>
>
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, and Zameer Manji.
>
>
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
>
> The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
>
> As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
>
>
> Diffs
> -----
>
> build.gradle e92b27d734fb6dc9c81c39ec8cb680ef179c3695
> src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
> src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/base/Jobs.java 744d2b694dbcf329e67e44db206b5f7381056dbe
> src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
> src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java f9521f9599e2d26cf594d62fc6b6f6ca3d5fb108
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 6a97f36a69c7723ec1c3730085d0df70f94dcac8
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c520e1378c4b7947bb866011027b79b4ab65547c
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 87b773ba80784e4af8cdbbbb78790c373c08e26e
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java dc0169ac727190028fd87c640af0100d4e3ce694
> src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
> src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
> src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
> src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
> src/main/thrift/org/apache/aurora/gen/api.thrift 8396e3d247b82a205b599474ee02819b695bf3d8
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java af644a967983b8da66231390d29839a034697852
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 820a968672e33b18b7f00d7346c9c7d7963fce0e
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/EnumsTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
> src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
> src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
>
> Diff: https://reviews.apache.org/r/25529/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> Jacoco reports 100% line coverage, 98% branch coverage for the updater package.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 25529: Add a controller for job updates.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/#review53051
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/base/Query.java
<https://reviews.apache.org/r/25529/#comment92433>
typo
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
<https://reviews.apache.org/r/25529/#comment92482>
Consider discarding in favor of https://reviews.apache.org/r/25481/
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java
<https://reviews.apache.org/r/25529/#comment92439>
Should it rather be named ADD_TASK...? It's used for both update (replace) and add new instance but since KILL is separate feels like "ADD" should be more appropriate here (i.e. it only inserts pending and does not do any killing).
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java
<https://reviews.apache.org/r/25529/#comment92438>
Replace with instanceName.
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java
<https://reviews.apache.org/r/25529/#comment92488>
Will quota checking come later or you'd rather see AURORA-686 fixed instead?
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java
<https://reviews.apache.org/r/25529/#comment92440>
This will throw unnecesserily if the instance is not in active state. I'd rather see an inactive instance get killed.
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java
<https://reviews.apache.org/r/25529/#comment92463>
This value is user-controlled and might be set too low for the kill to succeed. Would it rather make sense to fix it internally (as it is now on the client)?
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java
<https://reviews.apache.org/r/25529/#comment92473>
Missing docs.
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92480>
typo
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92496>
Why not just inline Functions.forMap(ImmutableMap.of(ABORTED, ABORTED)) instead?
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92495>
Spent quite a bit of time reading this method. Would definitely benefit from refactoring. How about at least moving everything down from here to something like maybeEvaluateUpdater()?
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92527>
Mind leaving a TODO here to create a getJobUpdate() storage API instead?
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92528>
+1
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92529>
Is active() deliberate here? If so, perhaps rename the function to "getActiveInstance" to clearly state its purpose?
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92571>
I a bit concerned about this circular dependency between evaluateUpdater and changeJobUpdateStatus but not sure what would be the best way to break it... Hopefully there is enough checking in place to prevent looping.
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92569>
This seems unconditional. Where does rollbackOnFailure get evaluated?
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92570>
Would it make sense to have a catch-all else block here to log in case actions is empty?
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java
<https://reviews.apache.org/r/25529/#comment92497>
Logging try...catch around similar to taskChangedState()?
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java
<https://reviews.apache.org/r/25529/#comment92498>
Same here. System resume is notoriously hard to troubleshoot and we probably don't want to let it out uncaught.
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java
<https://reviews.apache.org/r/25529/#comment92575>
s/time.s/times.
- Maxim Khutornenko
On Sept. 11, 2014, 4:53 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25529/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2014, 4:53 a.m.)
>
>
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji.
>
>
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
>
> The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
>
> As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
>
>
> Diffs
> -----
>
> build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
> src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
> src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
> src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
> src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 8befecaf4f13c0b890b452bc7b9c0618725b8c41
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java b894a71348d2d71c077f35bb21a80ba88a6b4c42
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 599dbd8bb762d051b75aa1a1e4d6a4c90ca6eb87
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java ec9b37ccaf03651e986aab9b4541f17ff0540008
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
> src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
> src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
> src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
> src/main/thrift/org/apache/aurora/gen/api.thrift c00f94371a27ffab41188b22f81bb1c8ec7a57e9
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 0be4d78dd737b34f8a58276be9ffd7aeaa7f95bb
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceActionHandlerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
> src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
> src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
>
> Diff: https://reviews.apache.org/r/25529/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> Jacoco reports 98% line coverage, 96% branch coverage for the updater package.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 25529: Add a controller for job updates.
Posted by Bill Farner <wf...@apache.org>.
> On Sept. 11, 2014, 10:13 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 111
> > <https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line111>
> >
> > Instead of embedding the logic into a switch here, would it make sense to make InstanceAction an actual class that encapsulates the logic/defines an interface around performing these actions?
A fine suggestion! It resulted in more code, but it's overall cleaner, thanks!
> On Sept. 11, 2014, 10:13 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 113
> > <https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line113>
> >
> > use instanceName here?
Nuked with switch to IInstanceKey.
> On Sept. 11, 2014, 10:13 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 141
> > <https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line141>
> >
> > Should we log (or assert?) here? Are there other actions that we're intentionally not processing, or is it an error if we get an action not covered?
Goes away now that switch statement is gone.
> On Sept. 11, 2014, 10:13 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java, line 119
> > <https://reviews.apache.org/r/25529/diff/1/?file=685012#file685012line119>
> >
> > Is there a ticket for this?
Whoops, that was a reminder for myself, fixed in this diff. Removed TODO.
> On Sept. 11, 2014, 10:13 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 141
> > <https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line141>
> >
> > I don't know if the amount of work done by the underlying calls warrants it, but you could DRY this up a bit by having instance vars for `storeProvider.getJobUpdateStore()` (used here, and above) and `update.getSummary()` (used twice below and once above). If you wanted to go completely overboard, `update.getSummary().getJobKey()` is used above and below.
They're all accessors. I've done a bit of collapsing, trying for a balance between more code and more DRY.
> On Sept. 11, 2014, 10:13 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 159
> > <https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line159>
> >
> > It seems like the logic of what states to transition to from a given state is somewhat spread out over the codebase (c.f. https://reviews.apache.org/r/25300/diff/#)? Is there any way we can centralize that?
Thanks, i wasn't happy with this. I've pushed more of this down into JobUpdateStateMachine.
> On Sept. 11, 2014, 10:13 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 300
> > <https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line300>
> >
> > Thoughts on changing the Function here to be `Function<JobUpdateStatus, Optional<JobUpdateStatus>>` to clean up the null dance below?
Sure, done. Downside is that we lose ability to use Functions.forMap().
> On Sept. 11, 2014, 10:13 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, lines 484-488
> > <https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line484>
> >
> > Do we need guards in place in case the updaterStatus here is not ROLLING_BACK or ROLLING_FORWARD (or is it guaranteed to be one of the two? If so, is there danger in the addition of additional statuses in the future rendering that assumption invalid?)
I've refactored this to eliminate the fall-through branch. Thanks for the nudge!
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/#review53101
-----------------------------------------------------------
On Sept. 11, 2014, 4:53 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25529/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2014, 4:53 a.m.)
>
>
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji.
>
>
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
>
> The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
>
> As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
>
>
> Diffs
> -----
>
> build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
> src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
> src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
> src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
> src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 8befecaf4f13c0b890b452bc7b9c0618725b8c41
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java b894a71348d2d71c077f35bb21a80ba88a6b4c42
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 599dbd8bb762d051b75aa1a1e4d6a4c90ca6eb87
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java ec9b37ccaf03651e986aab9b4541f17ff0540008
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
> src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
> src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
> src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
> src/main/thrift/org/apache/aurora/gen/api.thrift c00f94371a27ffab41188b22f81bb1c8ec7a57e9
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 0be4d78dd737b34f8a58276be9ffd7aeaa7f95bb
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceActionHandlerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
> src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
> src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
>
> Diff: https://reviews.apache.org/r/25529/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> Jacoco reports 98% line coverage, 96% branch coverage for the updater package.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 25529: Add a controller for job updates.
Posted by Joshua Cohen <jc...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/#review53101
-----------------------------------------------------------
Some of these questions are probably non-sensical due to my limited understanding of the scheduler internals, apologies for that ;).
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java
<https://reviews.apache.org/r/25529/#comment92509>
Instead of embedding the logic into a switch here, would it make sense to make InstanceAction an actual class that encapsulates the logic/defines an interface around performing these actions?
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java
<https://reviews.apache.org/r/25529/#comment92507>
use instanceName here?
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java
<https://reviews.apache.org/r/25529/#comment92508>
Should we log (or assert?) here? Are there other actions that we're intentionally not processing, or is it an error if we get an action not covered?
src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java
<https://reviews.apache.org/r/25529/#comment92513>
Is there a ticket for this?
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92526>
I don't know if the amount of work done by the underlying calls warrants it, but you could DRY this up a bit by having instance vars for `storeProvider.getJobUpdateStore()` (used here, and above) and `update.getSummary()` (used twice below and once above). If you wanted to go completely overboard, `update.getSummary().getJobKey()` is used above and below.
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92533>
It seems like the logic of what states to transition to from a given state is somewhat spread out over the codebase (c.f. https://reviews.apache.org/r/25300/diff/#)? Is there any way we can centralize that?
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92537>
Thoughts on changing the Function here to be `Function<JobUpdateStatus, Optional<JobUpdateStatus>>` to clean up the null dance below?
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92542>
Do we need guards in place in case the updaterStatus here is not ROLLING_BACK or ROLLING_FORWARD (or is it guaranteed to be one of the two? If so, is there danger in the addition of additional statuses in the future rendering that assumption invalid?)
- Joshua Cohen
On Sept. 11, 2014, 4:53 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25529/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2014, 4:53 a.m.)
>
>
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji.
>
>
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
>
> The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
>
> As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
>
>
> Diffs
> -----
>
> build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
> src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
> src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
> src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
> src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 8befecaf4f13c0b890b452bc7b9c0618725b8c41
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java b894a71348d2d71c077f35bb21a80ba88a6b4c42
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 599dbd8bb762d051b75aa1a1e4d6a4c90ca6eb87
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java ec9b37ccaf03651e986aab9b4541f17ff0540008
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
> src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
> src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
> src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
> src/main/thrift/org/apache/aurora/gen/api.thrift c00f94371a27ffab41188b22f81bb1c8ec7a57e9
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 0be4d78dd737b34f8a58276be9ffd7aeaa7f95bb
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceActionHandlerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
> src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
> src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
>
> Diff: https://reviews.apache.org/r/25529/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> Jacoco reports 98% line coverage, 96% branch coverage for the updater package.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 25529: Add a controller for job updates.
Posted by Bill Farner <wf...@apache.org>.
> On Sept. 11, 2014, 6:51 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 109
> > <https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line109>
> >
> > Would it be a bad idea to encapsulate this into a InstanceName class that contains the logic for constructing one from JobKey + instanceId and how to render it to a String?
> >
> > It seems like a bit of overkill but I dislike the + "/" in the code.
>
> Bill Farner wrote:
> Maybe a method rather than a class? The "/" is orthogonal to extracting a helper. I don't care about formatting - just that logs have necessary context.
I revisited this and started using IInstanceKey, which is ~exactly what you're describing. Thanks!
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/#review53075
-----------------------------------------------------------
On Sept. 11, 2014, 4:53 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25529/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2014, 4:53 a.m.)
>
>
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji.
>
>
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
>
> The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
>
> As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
>
>
> Diffs
> -----
>
> build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
> src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
> src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
> src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
> src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 8befecaf4f13c0b890b452bc7b9c0618725b8c41
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java b894a71348d2d71c077f35bb21a80ba88a6b4c42
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 599dbd8bb762d051b75aa1a1e4d6a4c90ca6eb87
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java ec9b37ccaf03651e986aab9b4541f17ff0540008
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
> src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
> src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
> src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
> src/main/thrift/org/apache/aurora/gen/api.thrift c00f94371a27ffab41188b22f81bb1c8ec7a57e9
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 0be4d78dd737b34f8a58276be9ffd7aeaa7f95bb
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceActionHandlerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
> src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
> src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
>
> Diff: https://reviews.apache.org/r/25529/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> Jacoco reports 98% line coverage, 96% branch coverage for the updater package.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 25529: Add a controller for job updates.
Posted by Bill Farner <wf...@apache.org>.
> On Sept. 11, 2014, 6:51 p.m., Zameer Manji wrote:
> > build.gradle, line 262
> > <https://reviews.apache.org/r/25529/diff/1/?file=685001#file685001line262>
> >
> > Is this needed? If so please break it out of this change.
Context: this has code coverage run without adding -Pq
Happy to pull this out for discussion in another review if you'd like. Basically, Maxim and i concluded that jacoco is a small overhead when compared to findbugs, and is a really important part of running unit tests. When trying to converge a unit test to 100% coverage, running the extra checks is a big burden.
> On Sept. 11, 2014, 6:51 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 109
> > <https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line109>
> >
> > Would it be a bad idea to encapsulate this into a InstanceName class that contains the logic for constructing one from JobKey + instanceId and how to render it to a String?
> >
> > It seems like a bit of overkill but I dislike the + "/" in the code.
Maybe a method rather than a class? The "/" is orthogonal to extracting a helper. I don't care about formatting - just that logs have necessary context.
> On Sept. 11, 2014, 6:51 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 94
> > <https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line94>
> >
> > Just to be clear, currently-active updaters are also persisted in storage right?
Yes.
> On Sept. 11, 2014, 6:51 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 357
> > <https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line357>
> >
> > Shouldn't this just be FAILED?
There's another static import for another FAILED.
> On Sept. 11, 2014, 6:51 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java, line 93
> > <https://reviews.apache.org/r/25529/diff/1/?file=685018#file685018line93>
> >
> > How are these errors going to be surfaced to the user?
Maxim and i stepped on toes here, so this validation moves up the stack with https://reviews.apache.org/r/25481/
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/#review53075
-----------------------------------------------------------
On Sept. 11, 2014, 4:53 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25529/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2014, 4:53 a.m.)
>
>
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji.
>
>
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
>
> The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
>
> As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
>
>
> Diffs
> -----
>
> build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
> src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
> src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
> src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
> src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 8befecaf4f13c0b890b452bc7b9c0618725b8c41
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java b894a71348d2d71c077f35bb21a80ba88a6b4c42
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 599dbd8bb762d051b75aa1a1e4d6a4c90ca6eb87
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java ec9b37ccaf03651e986aab9b4541f17ff0540008
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
> src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
> src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
> src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
> src/main/thrift/org/apache/aurora/gen/api.thrift c00f94371a27ffab41188b22f81bb1c8ec7a57e9
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 0be4d78dd737b34f8a58276be9ffd7aeaa7f95bb
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceActionHandlerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
> src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
> src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
>
> Diff: https://reviews.apache.org/r/25529/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> Jacoco reports 98% line coverage, 96% branch coverage for the updater package.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 25529: Add a controller for job updates.
Posted by Zameer Manji <zm...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/#review53075
-----------------------------------------------------------
This matches the logic previously explained to me, but the diff is pretty large. Once my questions are answered I'll take another look before giving a ship it.
build.gradle
<https://reviews.apache.org/r/25529/#comment92465>
Is this needed? If so please break it out of this change.
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java
<https://reviews.apache.org/r/25529/#comment92477>
Would it be a bad idea to encapsulate this into a InstanceName class that contains the logic for constructing one from JobKey + instanceId and how to render it to a String?
It seems like a bit of overkill but I dislike the + "/" in the code.
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92481>
Just to be clear, currently-active updaters are also persisted in storage right?
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92483>
Shouldn't this just be FAILED?
src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java
<https://reviews.apache.org/r/25529/#comment92484>
How are these errors going to be surfaced to the user?
- Zameer Manji
On Sept. 10, 2014, 9:53 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25529/
> -----------------------------------------------------------
>
> (Updated Sept. 10, 2014, 9:53 p.m.)
>
>
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, and Zameer Manji.
>
>
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There's a lot going on here, i'm happy to break this up if you'd perfer, but the bulk of the change (JobUpdateControllerImpl+Test) would remain.
>
> The remaining required piece here is to record JobInstanceUpdateEvents when actions are taken.
>
> As you try to follow JobUpdateControllerImplTest, take some care to understand how FakeScheduledExecutor works. Ultimately, it accepts work added to a mock, and executes that work when FakeClock is advanced past the scheduled time. It may not be obvious at first, but be glad it's there - this kind of async test would be a nightmare without it.
>
>
> Diffs
> -----
>
> build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3
> src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java aaa3513ae99c70ffa51fec0a241d67fb815b6d3f
> src/main/java/org/apache/aurora/scheduler/base/Jobs.java 7b10cf876d4424fab06113aa3e2989a6bef4d346
> src/main/java/org/apache/aurora/scheduler/base/Query.java d8572bb21a92025e7a51cf18d5bdf00fc1281078
> src/main/java/org/apache/aurora/scheduler/base/Tasks.java 1be16374aeebaee59063aec982690ffa1fbdcf0f
> src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 8befecaf4f13c0b890b452bc7b9c0618725b8c41
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java b894a71348d2d71c077f35bb21a80ba88a6b4c42
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 599dbd8bb762d051b75aa1a1e4d6a4c90ca6eb87
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java ec9b37ccaf03651e986aab9b4541f17ff0540008
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java d725bc356178f51464b4d8ea896f1bf76815e1b2
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 39bdca02295706714cf720545ffc291f9042702a
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 3f542ce3e45ec4561238736f4ce84b69f7e41219
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java 7be792f0bc9c5ec14c7001cb243a17d643f9607f
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 037602d6aabe6dda978cad008075c053e2c042eb
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java 80baa7f9360cc8b2bf7910c26d119d443d6cbbf9
> src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 028cb079316ca48bb93a35913ae25ace78088fb4
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 8298ea2283298daa71de4d958ca6bb0898d47531
> src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java a3e666e85993b833a4765ce0ad5f4825dc9253e8
> src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 0cf36839dbf64ad755383bef829e14fd94a97e30
> src/main/thrift/org/apache/aurora/gen/api.thrift c00f94371a27ffab41188b22f81bb1c8ec7a57e9
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 0be4d78dd737b34f8a58276be9ffd7aeaa7f95bb
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 0d51f7dc367081f72090736e36605bf363f3395e
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceActionHandlerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java f38e6a3e6a798b1c78d73c6d19404156eb6d8c91
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java 41274421aaae30b43abc3da15a63276a941094f9
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java 6eed641895123d21ed760fa755ce7b30cec3fd44
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java f0b68350ea39e5925a911e46532982c3d61ee0d6
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java ae654625370fd3ba8be59b9a37ba343ec7115cba
> src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java e6742dcbe101389710c11e9b75a508f7b61ffe49
> src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc
>
> Diff: https://reviews.apache.org/r/25529/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> Jacoco reports 98% line coverage, 96% branch coverage for the updater package.
>
>
> Thanks,
>
> Bill Farner
>
>