----------------------------------------------------------- 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 <https://reviews.apache.org/r/19767/#comment72027> Revert. src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java <https://reviews.apache.org/r/19767/#comment71982> incomplete? src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java <https://reviews.apache.org/r/19767/#comment71983> Missing description. src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java <https://reviews.apache.org/r/19767/#comment71992> Missing param. src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java <https://reviews.apache.org/r/19767/#comment71991> Missing jobKey definition src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java <https://reviews.apache.org/r/19767/#comment71990> Missing params, return. src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java <https://reviews.apache.org/r/19767/#comment71989> Consider? src/main/java/org/apache/aurora/scheduler/cron/CronScheduler.java <https://reviews.apache.org/r/19767/#comment71987> 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 <https://reviews.apache.org/r/19767/#comment72000> s/cronjob/cron job src/main/java/org/apache/aurora/scheduler/cron/SanitizedCronJob.java <https://reviews.apache.org/r/19767/#comment71998> Missing comments for public methods? src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java <https://reviews.apache.org/r/19767/#comment72008> 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 <https://reviews.apache.org/r/19767/#comment72007> This does not seem to be used anywhere. src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java <https://reviews.apache.org/r/19767/#comment72018> 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 <https://reviews.apache.org/r/19767/#comment71984> s/2013/2014/g src/main/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImpl.java <https://reviews.apache.org/r/19767/#comment71994> s/2013/2014 src/main/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImpl.java <https://reviews.apache.org/r/19767/#comment71985> Move down below the constructor. src/main/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImpl.java <https://reviews.apache.org/r/19767/#comment71997> 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 <https://reviews.apache.org/r/19767/#comment72002> Consider string.format() src/main/java/org/apache/aurora/scheduler/cron/quartz/CronLifecycle.java <https://reviews.apache.org/r/19767/#comment72003> A bit terse :) src/main/java/org/apache/aurora/scheduler/cron/quartz/CronModule.java <https://reviews.apache.org/r/19767/#comment72023> Move this down after the @CmdLine block. src/main/java/org/apache/aurora/scheduler/cron/quartz/CronModule.java <https://reviews.apache.org/r/19767/#comment72014> Shouldn't this be "...a previous cron run to end"? src/main/java/org/apache/aurora/scheduler/cron/quartz/CronModule.java <https://reviews.apache.org/r/19767/#comment72015> 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 > >
