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 93D701035A for ; Fri, 4 Apr 2014 20:29:00 +0000 (UTC) Received: (qmail 93277 invoked by uid 500); 4 Apr 2014 20:28:59 -0000 Delivered-To: apmail-aurora-reviews-archive@aurora.apache.org Received: (qmail 93236 invoked by uid 500); 4 Apr 2014 20:28:56 -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 93172 invoked by uid 99); 4 Apr 2014 20:28:53 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 04 Apr 2014 20:28:53 +0000 X-ASF-Spam-Status: No, hits=-1998.3 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, 04 Apr 2014 20:28:50 +0000 Received: (qmail 89903 invoked by uid 99); 4 Apr 2014 20:28:23 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 04 Apr 2014 20:28:23 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 3B4BA1D5D51; Fri, 4 Apr 2014 20:28:17 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============7430913384077661185==" MIME-Version: 1.0 Subject: Re: Review Request 19767: CronScheduler based on Quartz From: "Maxim Khutornenko" To: "Bill Farner" , "Maxim Khutornenko" Cc: "Aurora" , "Kevin Sweeney" Date: Fri, 04 Apr 2014 20:28:17 -0000 Message-ID: <20140404202817.17825.97214@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Maxim Khutornenko" X-ReviewGroup: Aurora X-ReviewRequest-URL: https://reviews.apache.org/r/19767/ X-Sender: "Maxim Khutornenko" References: <20140404011646.18229.41774@reviews.apache.org> In-Reply-To: <20140404011646.18229.41774@reviews.apache.org> Reply-To: "Maxim Khutornenko" X-ReviewRequest-Repository: aurora X-Virus-Checked: Checked by ClamAV on apache.org --===============7430913384077661185== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19767/#review39569 ----------------------------------------------------------- src/main/java/org/apache/aurora/scheduler/base/JobKeys.java Revert. src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java incomplete? src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java Missing description. src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java Missing param. src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java Missing jobKey definition src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java Missing params, return. src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java Consider? src/main/java/org/apache/aurora/scheduler/cron/CronScheduler.java I can't find any usage of this method. Is this abstraction still in use? src/main/java/org/apache/aurora/scheduler/cron/SanitizedCronJob.java s/cronjob/cron job src/main/java/org/apache/aurora/scheduler/cron/SanitizedCronJob.java Missing comments for public methods? src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java Consider splitting logic in this block into a set of private helper methods for improved readability. src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java This does not seem to be used anywhere. src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java I would expect a task scoped + state scoped query here as a killed task would not be deleted until a HistoryPruner kicks in. src/main/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImpl.java s/2013/2014/g src/main/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImpl.java s/2013/2014 src/main/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImpl.java Move down below the constructor. src/main/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImpl.java This is a departure from the current implementation where the old job is removed and the new one is created. Consider following the same approach. Otherwise, we may end up in an unpredictable state if scheduler.deleteJob() throws. src/main/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImpl.java Consider string.format() src/main/java/org/apache/aurora/scheduler/cron/quartz/CronLifecycle.java A bit terse :) src/main/java/org/apache/aurora/scheduler/cron/quartz/CronModule.java Move this down after the @CmdLine block. src/main/java/org/apache/aurora/scheduler/cron/quartz/CronModule.java Shouldn't this be "...a previous cron run to end"? src/main/java/org/apache/aurora/scheduler/cron/quartz/CronModule.java Same here. - Maxim Khutornenko On April 4, 2014, 1:16 a.m., Kevin Sweeney wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19767/ > ----------------------------------------------------------- > > (Updated April 4, 2014, 1:16 a.m.) > > > Review request for Aurora, Maxim Khutornenko and Bill Farner. > > > Bugs: AURORA-132 > https://issues.apache.org/jira/browse/AURORA-132 > > > Repository: aurora > > > Description > ------- > > This introduces a new CronScheduler based on Quartz and removes the NoopCronScheduler. > > > Diffs > ----- > > build.gradle 109c193da3324bd5534b409bfabb6aeb0adda7b1 > src/main/java/org/apache/aurora/scheduler/MesosTaskFactory.java 86bbc29c64dd62037ad6bc51b8daa30115eaf74c > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java ec56c649116c03ef148bac916bd6691a94685bc3 > src/main/java/org/apache/aurora/scheduler/async/TaskGroups.java 6c95c6f7695cb8126105818528900cb09887ad3e > src/main/java/org/apache/aurora/scheduler/base/JobKeys.java db1bec4f508c8908f212aa541fb86e041a8c471c > src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java 82034e008e5dbaa3124dc154cdc6c5e9767ca87f > src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/cron/CronPredictor.java df0c37839c5da54795404c18ff9fc93084cd32e4 > src/main/java/org/apache/aurora/scheduler/cron/CronScheduler.java 56e9950fd94ae1e3dbd96baec00b7e6b262fbe34 > src/main/java/org/apache/aurora/scheduler/cron/CrontabEntryTest.java 2bb848a7f5f096b1c85596e4130f0656e9a4401e > src/main/java/org/apache/aurora/scheduler/cron/SanitizedCronJob.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/cron/noop/NoopCronModule.java e0935f5eab8a101f4ce1831f260f9a23137124ce > src/main/java/org/apache/aurora/scheduler/cron/noop/NoopCronPredictor.java 7b25152c0258e10be21b801cae1444c518367fa7 > src/main/java/org/apache/aurora/scheduler/cron/noop/NoopCronScheduler.java a31551c77818c17ee0f9f71b5ab458a3b853dc6a > src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobFactory.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImpl.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronLifecycle.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronModule.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronPredictorImpl.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronSchedulerImpl.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/cron/quartz/Quartz.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/cron/testing/AbstractCronIT.java 61b01d2575d6cfce069e77c39bfa8f8680cf4298 > src/main/java/org/apache/aurora/scheduler/http/Cron.java 80a398a5f297558a25c0a0c63afcb049a9558e44 > src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java 2ccc6f367b9715a0abb3e0673069289ae4860087 > src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java e2f9ed0ea846c570de11b7dd85bc90aee6bc3342 > src/main/java/org/apache/aurora/scheduler/http/ServletModule.java e3ff2571d95effcf72b2047cc5840d56143a180c > src/main/java/org/apache/aurora/scheduler/http/StructDump.java efea75f3d5a5f4c538c63cc15d5a004d891c2a4a > src/main/java/org/apache/aurora/scheduler/state/CronJobManager.java fa39e2b901bdc764d802a05d26ee73d77ef7604d > src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 5696485e5beb9b7bf4ccee8b6189f25db51aff39 > src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 2f6800043839dbb586bc774ccccb13fdd16ee09c > src/main/java/org/apache/aurora/scheduler/state/StateModule.java 7d26082b74f62f35865e0343f9ba8b475e075d62 > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 84151a5308c12b3bee7cf5fd662776e574e8fadf > src/main/resources/org/apache/aurora/scheduler/cron/testing/cron-schedule-predictions.json > src/test/java/org/apache/aurora/scheduler/cron/ExpectedPrediction.java PRE-CREATION > src/test/java/org/apache/aurora/scheduler/cron/noop/NoopCronIT.java a9b85d0983dcfee89101a5e774ba86ee11708c68 > src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java PRE-CREATION > src/test/java/org/apache/aurora/scheduler/cron/quartz/CronIT.java PRE-CREATION > src/test/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImplTest.java PRE-CREATION > src/test/java/org/apache/aurora/scheduler/cron/quartz/CronPredictorImplTest.java PRE-CREATION > src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzTestUtil.java PRE-CREATION > src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java f834bfb8c2339585214b0512e7df98dc75f931c7 > src/test/java/org/apache/aurora/scheduler/state/CronJobManagerTest.java fa9cb757936542c483699b3fc6bba944d717abac > src/test/java/org/apache/aurora/scheduler/state/LockManagerImplTest.java c8ad55d8d48f7e96180846ab515dd4df3d8ed79e > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java fae2de11235dd059718e1023fdcfb0e8fc4deadd > src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java e212174ed089fdcf28fa679318fe216917a40b99 > > Diff: https://reviews.apache.org/r/19767/diff/ > > > Testing > ------- > > ./gradlew build > > > File Attachments > ---------------- > > Coverage report. > https://reviews.apache.org/media/uploaded/files/2014/04/03/6b5f24f5-86a5-43d2-8a0e-d69fd24f7d2a__Screenshot_from_2014-04-02_203443.png > > > Thanks, > > Kevin Sweeney > > --===============7430913384077661185==--