Re: Review Request 25529: Add a controller for job updates.

[Kevin Sweeney]

---
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 
 87b773ba80784e4af8cd78790c373c08e26e 
   
 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 
   

Re: Review Request 25529: Add a controller for job updates.

[Zameer Manji]

---
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 
 87b773ba80784e4af8cd78790c373c08e26e 
   
 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 
   

Re: Review Request 25529: Add a controller for job updates.

[Bill Farner]

---
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 
87b773ba80784e4af8cd78790c373c08e26e 
  
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 
  

Re: Review Request 25529: Add a controller for job updates.

[Bill Farner]

 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 
 87b773ba80784e4af8cd78790c373c08e26e 
   
 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 
   

Re: Review Request 25529: Add a controller for job updates.

[Joshua Cohen]

---
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 
 87b773ba80784e4af8cd78790c373c08e26e 
   
 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 
 

Re: Review Request 25529: Add a controller for job updates.

[Kevin Sweeney]

---
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 
 87b773ba80784e4af8cd78790c373c08e26e 
   
 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 
   

Re: Review Request 25529: Add a controller for job updates.

[Bill Farner]

 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 
 87b773ba80784e4af8cd78790c373c08e26e 
   
 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 
   
 

Re: Review Request 25529: Add a controller for job updates.

[Bill Farner]

 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 
 87b773ba80784e4af8cd78790c373c08e26e 
   
 

Re: Review Request 25529: Add a controller for job updates.

[Bill Farner]

 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 
 87b773ba80784e4af8cd78790c373c08e26e 
   
 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 
   

Re: Review Request 25529: Add a controller for job updates.

[Bill Farner]

---
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 
87b773ba80784e4af8cd78790c373c08e26e 
  
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 
  

Re: Review Request 25529: Add a controller for job updates.

[Bill Farner]

 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 
   
 

Re: Review Request 25529: Add a controller for job updates.

[Bill Farner]

 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 `FunctionJobUpdateStatus, 
  OptionalJobUpdateStatus` 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 

Re: Review Request 25529: Add a controller for job updates.

[Bill Farner]

---
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 
  

Re: Review Request 25529: Add a controller for job updates.

[Maxim Khutornenko]

 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 
   

Re: Review Request 25529: Add a controller for job updates.

[Zameer Manji]

---
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 
   
 

Re: Review Request 25529: Add a controller for job updates.

[Joshua Cohen]

---
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 `FunctionJobUpdateStatus, 
OptionalJobUpdateStatus` 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 
   
 

Re: Review Request 25529: Add a controller for job updates.

[Bill Farner]

 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 
   

Re: Review Request 25529: Add a controller for job updates.

[Maxim Khutornenko]

---
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 

Review Request 25529: Add a controller for job updates.

[Bill Farner]

---
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