-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25529/#review53051
-----------------------------------------------------------



src/main/java/org/apache/aurora/scheduler/base/Query.java
<https://reviews.apache.org/r/25529/#comment92433>

    typo



src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
<https://reviews.apache.org/r/25529/#comment92482>

    Consider discarding in favor of https://reviews.apache.org/r/25481/



src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java
<https://reviews.apache.org/r/25529/#comment92439>

    Should it rather be named ADD_TASK...? It's used for both update (replace) 
and add new instance but since KILL is separate feels like "ADD" should be more 
appropriate here (i.e. it only inserts pending and does not do any killing).



src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java
<https://reviews.apache.org/r/25529/#comment92438>

    Replace with instanceName.



src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java
<https://reviews.apache.org/r/25529/#comment92488>

    Will quota checking come later or you'd rather see AURORA-686 fixed instead?



src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java
<https://reviews.apache.org/r/25529/#comment92440>

    This will throw unnecesserily if the instance is not in active state. I'd 
rather see an inactive instance get killed.



src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java
<https://reviews.apache.org/r/25529/#comment92463>

    This value is user-controlled and might be set too low for the kill to 
succeed. Would it rather make sense to fix it internally (as it is now on the 
client)?



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java
<https://reviews.apache.org/r/25529/#comment92473>

    Missing docs.



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92480>

    typo



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92496>

    Why not just inline Functions.forMap(ImmutableMap.of(ABORTED, ABORTED)) 
instead?



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92495>

    Spent quite a bit of time reading this method. Would definitely benefit 
from refactoring. How about at least moving everything down from here to 
something like maybeEvaluateUpdater()?



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92527>

    Mind leaving a TODO here to create a getJobUpdate() storage API instead?



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92528>

    +1



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92529>

    Is active() deliberate here? If so, perhaps rename the function to 
"getActiveInstance" to clearly state its purpose?



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92571>

    I a bit concerned about this circular dependency between evaluateUpdater 
and changeJobUpdateStatus  but not sure what would be the best way to break 
it... Hopefully there is enough checking in place to prevent looping.



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92569>

    This seems unconditional. Where does rollbackOnFailure get evaluated?



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
<https://reviews.apache.org/r/25529/#comment92570>

    Would it make sense to have a catch-all else block here to log in case 
actions is empty?



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java
<https://reviews.apache.org/r/25529/#comment92497>

    Logging try...catch around similar to taskChangedState()?



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java
<https://reviews.apache.org/r/25529/#comment92498>

    Same here. System resume is notoriously hard to troubleshoot and we 
probably don't want to let it out uncaught.



src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java
<https://reviews.apache.org/r/25529/#comment92575>

    s/time.s/times.


- Maxim Khutornenko


On Sept. 11, 2014, 4:53 a.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25529/
> -----------------------------------------------------------
> 
> (Updated Sept. 11, 2014, 4:53 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, Maxim Khutornenko, 
> and Zameer Manji.
> 
> 
> Bugs: AURORA-613
>     https://issues.apache.org/jira/browse/AURORA-613
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> There's a lot going on here, i'm happy to break this up if you'd perfer, but 
> the bulk of the change (JobUpdateControllerImpl+Test) would remain.
> 
> The remaining required piece here is to record JobInstanceUpdateEvents when 
> actions are taken.
> 
> As you try to follow JobUpdateControllerImplTest, take some care to 
> understand how FakeScheduledExecutor works.  Ultimately, it accepts work 
> added to a mock, and executes that work when FakeClock is advanced past the 
> scheduled time.  It may not be obvious at first, but be glad it's there - 
> this kind of async test would be a nightmare without it.
> 
> 
> Diffs
> -----
> 
>   build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3 
>   src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java 
> aaa3513ae99c70ffa51fec0a241d67fb815b6d3f 
>   src/main/java/org/apache/aurora/scheduler/base/Jobs.java 
> 7b10cf876d4424fab06113aa3e2989a6bef4d346 
>   src/main/java/org/apache/aurora/scheduler/base/Query.java 
> d8572bb21a92025e7a51cf18d5bdf00fc1281078 
>   src/main/java/org/apache/aurora/scheduler/base/Tasks.java 
> 1be16374aeebaee59063aec982690ffa1fbdcf0f 
>   src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 
> 8befecaf4f13c0b890b452bc7b9c0618725b8c41 
>   src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 
> b894a71348d2d71c077f35bb21a80ba88a6b4c42 
>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
> 599dbd8bb762d051b75aa1a1e4d6a4c90ca6eb87 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 
> ec9b37ccaf03651e986aab9b4541f17ff0540008 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  a43e5d7748c22d60f56f03a8a3d52949faebeff2 
>   
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 
> d725bc356178f51464b4d8ea896f1bf76815e1b2 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> 39bdca02295706714cf720545ffc291f9042702a 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  3f542ce3e45ec4561238736f4ce84b69f7e41219 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java
>  7be792f0bc9c5ec14c7001cb243a17d643f9607f 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 
> 037602d6aabe6dda978cad008075c053e2c042eb 
>   src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java 
> d6a1e4b2c916b1c4dbcc73fe79bd672fca9b3189 
>   
> src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactory.java
>  80baa7f9360cc8b2bf7910c26d119d443d6cbbf9 
>   
> src/main/java/org/apache/aurora/scheduler/updater/UpdateConfigurationException.java
>  PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/updater/UpdaterModule.java 
> 028cb079316ca48bb93a35913ae25ace78088fb4 
>   
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java
>  8298ea2283298daa71de4d958ca6bb0898d47531 
>   
> src/main/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategy.java 
> a3e666e85993b833a4765ce0ad5f4825dc9253e8 
>   
> src/main/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategy.java 
> 0cf36839dbf64ad755383bef829e14fd94a97e30 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> c00f94371a27ffab41188b22f81bb1c8ec7a57e9 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  0be4d78dd737b34f8a58276be9ffd7aeaa7f95bb 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  0d51f7dc367081f72090736e36605bf363f3395e 
>   
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/updater/InstanceActionHandlerTest.java
>  PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java 
> f38e6a3e6a798b1c78d73c6d19404156eb6d8c91 
>   
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java
>  41274421aaae30b43abc3da15a63276a941094f9 
>   
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java
>  6eed641895123d21ed760fa755ce7b30cec3fd44 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdateControllerTest.java
>  f0b68350ea39e5925a911e46532982c3d61ee0d6 
>   
> src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterFactoryImplTest.java
>  ae654625370fd3ba8be59b9a37ba343ec7115cba 
>   
> src/test/java/org/apache/aurora/scheduler/updater/strategy/BatchStrategyTest.java
>  e6742dcbe101389710c11e9b75a508f7b61ffe49 
>   
> src/test/java/org/apache/aurora/scheduler/updater/strategy/QueueStrategyTest.java
>  f9a2806708ea1a06a4f0ebe118c777fd9dbe2dcc 
> 
> Diff: https://reviews.apache.org/r/25529/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew build -Pq
> 
> Jacoco reports 98% line coverage, 96% branch coverage for the updater package.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Reply via email to