Return-Path: X-Original-To: apmail-aurora-reviews-archive@minotaur.apache.org Delivered-To: apmail-aurora-reviews-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id C109B11576 for ; Mon, 15 Sep 2014 23:19:49 +0000 (UTC) Received: (qmail 55381 invoked by uid 500); 15 Sep 2014 23:19:49 -0000 Delivered-To: apmail-aurora-reviews-archive@aurora.apache.org Received: (qmail 55335 invoked by uid 500); 15 Sep 2014 23:19:49 -0000 Mailing-List: contact reviews-help@aurora.incubator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: reviews@aurora.incubator.apache.org Delivered-To: mailing list reviews@aurora.incubator.apache.org Received: (qmail 55323 invoked by uid 99); 15 Sep 2014 23:19:49 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 15 Sep 2014 23:19:49 +0000 X-ASF-Spam-Status: No, hits=-1998.5 required=5.0 tests=ALL_TRUSTED,HTML_MESSAGE,RP_MATCHES_RCVD X-Spam-Check-By: apache.org Received: from [140.211.11.3] (HELO mail.apache.org) (140.211.11.3) by apache.org (qpsmtpd/0.29) with SMTP; Mon, 15 Sep 2014 23:19:47 +0000 Received: (qmail 53862 invoked by uid 99); 15 Sep 2014 23:19:27 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 15 Sep 2014 23:19:27 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 475851DB832; Mon, 15 Sep 2014 23:19:25 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============0428311662886068054==" MIME-Version: 1.0 Subject: Re: Review Request 25529: Add a controller for job updates. From: "Bill Farner" To: "Kevin Sweeney" , "Maxim Khutornenko" , "Zameer Manji" , "Joshua Cohen" Cc: "Bill Farner" , "Aurora" Date: Mon, 15 Sep 2014 23:19:25 -0000 Message-ID: <20140915231925.7800.66868@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Bill Farner" X-ReviewGroup: Aurora X-ReviewRequest-URL: https://reviews.apache.org/r/25529/ X-Sender: "Bill Farner" References: <20140915230131.7803.61754@reviews.apache.org> In-Reply-To: <20140915230131.7803.61754@reviews.apache.org> Reply-To: "Bill Farner" X-ReviewRequest-Repository: aurora X-Virus-Checked: Checked by ClamAV on apache.org --===============0428311662886068054== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > On Sept. 15, 2014, 11:01 p.m., Joshua Cohen wrote: > > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1368 > > > > > > Is this a leftover from some intermediate work? Not sure how that made it in. Removed. > On Sept. 15, 2014, 11:01 p.m., Joshua Cohen wrote: > > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 81 > > > > > > Some more context to this log message might be helpful (akin to the KillTask log? `"Adding " + instance + " while " + status` Done. > On Sept. 15, 2014, 11:01 p.m., Joshua Cohen wrote: > > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, lines 123-124 > > > > > > Does it make sense to separate the config validation logic from the UpdateFactory creation? I'm slightly hesitatnt here. There's non-trivial relationships between the fields in JobUpdateConfiguration, and i'm failing to extract the validation out in a way that doesn't leave me with 3 helper functions, some redundancy, and less certainty that the gates are in place. I'd also like to see if the TODO about moving the logic to SchedulerThriftInterface pans out. - Bill ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25529/#review53425 ----------------------------------------------------------- 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 > > --===============0428311662886068054==--