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 AAA7711C9D for ; Fri, 12 Sep 2014 22:36:24 +0000 (UTC) Received: (qmail 56143 invoked by uid 500); 12 Sep 2014 22:36:24 -0000 Delivered-To: apmail-aurora-reviews-archive@aurora.apache.org Received: (qmail 56097 invoked by uid 500); 12 Sep 2014 22:36:24 -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 56085 invoked by uid 99); 12 Sep 2014 22:36:23 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 12 Sep 2014 22:36:23 +0000 X-ASF-Spam-Status: No, hits=-1999.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; Fri, 12 Sep 2014 22:36:21 +0000 Received: (qmail 55917 invoked by uid 99); 12 Sep 2014 22:36:01 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 12 Sep 2014 22:36:01 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 8C6E11DD751; Fri, 12 Sep 2014 22:35:59 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============7241202074834670794==" 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: Fri, 12 Sep 2014 22:35:59 -0000 Message-ID: <20140912223559.23929.35964@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: <20140911234117.23923.30312@reviews.apache.org> In-Reply-To: <20140911234117.23923.30312@reviews.apache.org> Reply-To: "Bill Farner" X-ReviewRequest-Repository: aurora X-Virus-Checked: Checked by ClamAV on apache.org --===============7241202074834670794== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/base/Query.java, line 104 > > > > > > typo Better yet - addressed TODO :-) > On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1428 > > > > > > Consider discarding in favor of https://reviews.apache.org/r/25481/ Negatory - i still need the exception to flag when 'updateOnlyTheseInstances' is invalid. Though, i think all this validation should live together. Added a TODO. > On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 112 > > > > > > 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). Sure, done. > On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 113 > > > > > > Replace with instanceName. Obviated by other changes. > On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 118 > > > > > > Will quota checking come later or you'd rather see AURORA-686 fixed instead? Good question. I've added a TODO, we should discuss later which should come first. > On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, lines 123-124 > > > > > > This will throw unnecesserily if the instance is not in active state. I'd rather see an inactive instance get killed. That's a precondition - this action should not be taken if the instance is not active. > On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java, line 131 > > > > > > 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)? I think a lower bound when validating is reasonable, but i don't like the idea of changing it silently. > On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java, line 79 > > > > > > Missing docs. Added. > On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 122 > > > > > > typo Fixed. > On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 187 > > > > > > Why not just inline Functions.forMap(ImmutableMap.of(ABORTED, ABORTED)) instead? Because the output should _always_ be ABORTED. That map would mean "you can only go from aborted to aborted." > On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 398 > > > > > > 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()? I wasn't able to follow your request, can you give some pseudocode to illustrate? > On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 408 > > > > > > Mind leaving a TODO here to create a getJobUpdate() storage API instead? Done. > On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 444 > > > > > > Is active() deliberate here? If so, perhaps rename the function to "getActiveInstance" to clearly state its purpose? It's critical, we don't want to evaluate exited tasks. Changed the name. > On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 493 > > > > > > This seems unconditional. Where does rollbackOnFailure get evaluated? This changed with a comment above, likely obviating the comment. > On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java, line 496 > > > > > > Would it make sense to have a catch-all else block here to log in case actions is empty? SGTM, done. > On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java, line 83 > > > > > > Logging try...catch around similar to taskChangedState()? Done. > On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriber.java, line 97 > > > > > > Same here. System resume is notoriously hard to troubleshoot and we probably don't want to let it out uncaught. Done. > On Sept. 11, 2014, 11:41 p.m., Maxim Khutornenko wrote: > > src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java, line 137 > > > > > > s/time.s/times. Fixed. - Bill ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25529/#review53051 ----------------------------------------------------------- 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 > > --===============7241202074834670794==--