> 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 `Function<JobUpdateStatus, > > Optional<JobUpdateStatus>>` 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 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 > >
