> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java,
> >  line 398
> > <https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line398>
> >
> >     Spent quite a bit of time reading this method. Would definitely benefit 
> > from refactoring. How about at least moving everything down from here to 
> > something like maybeEvaluateUpdater()?
> 
> Bill Farner wrote:
>     I wasn't able to follow your request, can you give some pseudocode to 
> illustrate?

Everything down from here deals with processing a MonitorAction acquired on 
this line and can technically be moved into its own method. That would reduce 
the recordAndChangeJobUpdateStatus() length and make it a bit more readable 
(i.e. easier to reason about the recursive way this funciton may be invoked).


> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java,
> >  line 131
> > <https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line131>
> >
> >     This value is user-controlled and might be set too low for the kill to 
> > succeed. Would it rather make sense to fix it internally (as it is now on 
> > the client)?
> 
> Bill Farner wrote:
>     I think a lower bound when validating is reasonable, but i don't like the 
> idea of changing it silently.

A lower bound validation would not quite solve the problem as it still may not 
be enough to kill a task. I have routinely observed task kills taking longer 
than the default watch_secs value of 45 seconds. The current client timeout is 
set to 2.5 minutes of binary backoff countdown. I feel we should follow the 
same approach on the scheduler and perhaps expose an Arg to make it 
configurable.


> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java,
> >  lines 123-124
> > <https://reviews.apache.org/r/25529/diff/1/?file=685011#file685011line123>
> >
> >     This will throw unnecesserily if the instance is not in active state. 
> > I'd rather see an inactive instance get killed.
> 
> Bill Farner wrote:
>     That's a precondition - this action should not be taken if the instance 
> is not active.

Is there anything upstream that ensures the instance is still active though? Is 
that check part of the same transaction? I am concerned about the race between 
instance state and this query. Perhaps it's just safer to proceed with instance 
kill, which will be a no-op in this case.


> On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java,
> >  line 493
> > <https://reviews.apache.org/r/25529/diff/1/?file=685014#file685014line493>
> >
> >     This seems unconditional. Where does rollbackOnFailure get evaluated?
> 
> Bill Farner wrote:
>     This changed with a comment above, likely obviating the comment.

I meant to point that ROLLING_BACK should be conditional upon the 
user-specified rollbackOnFailure config value. I could not find where it is 
honored.


- Maxim


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


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

Reply via email to