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

Reply via email to