aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Farner" <wfar...@apache.org>
Subject Re: Review Request 25529: Add a controller for job updates.
Date Mon, 15 Sep 2014 23:52:34 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message