> On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/base/JobKeys.java, line 203 > > <https://reviews.apache.org/r/19767/diff/5/?file=548474#file548474line203> > > > > Revert.
Fixed. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java, > > line 171 > > <https://reviews.apache.org/r/19767/diff/5/?file=548475#file548475line171> > > > > incomplete? Fixed. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java, line 53 > > <https://reviews.apache.org/r/19767/diff/5/?file=548476#file548476line53> > > > > Missing description. Fixed. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java, line 65 > > <https://reviews.apache.org/r/19767/diff/5/?file=548476#file548476line65> > > > > Missing param. Fixed. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java, line 71 > > <https://reviews.apache.org/r/19767/diff/5/?file=548476#file548476line71> > > > > Missing jobKey definition Fixed. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java, line 79 > > <https://reviews.apache.org/r/19767/diff/5/?file=548476#file548476line79> > > > > Missing params, return. Fixed. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java, line 85 > > <https://reviews.apache.org/r/19767/diff/5/?file=548476#file548476line85> > > > > Consider? Fixed. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/cron/CronScheduler.java, line 54 > > <https://reviews.apache.org/r/19767/diff/5/?file=548478#file548478line54> > > > > I can't find any usage of this method. Is this abstraction still in use? Good catch - removed it and implementation. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/cron/SanitizedCronJob.java, line > > 48 > > <https://reviews.apache.org/r/19767/diff/5/?file=548480#file548480line48> > > > > s/cronjob/cron job Fixed. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/cron/SanitizedCronJob.java, line > > 70 > > <https://reviews.apache.org/r/19767/diff/5/?file=548480#file548480line70> > > > > Missing comments for public methods? Documented or reduced visibility for all methods in that class. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java, > > line 142 > > <https://reviews.apache.org/r/19767/diff/5/?file=548484#file548484line142> > > > > This does not seem to be used anywhere. It wasn't - I should pay more attention to grey in intellij. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java, > > line 194 > > <https://reviews.apache.org/r/19767/diff/5/?file=548484#file548484line194> > > > > I would expect a task scoped + state scoped query here as a killed task > > would not be deleted until a HistoryPruner kicks in. Good catch - updated. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImpl.java, > > line 2 > > <https://reviews.apache.org/r/19767/diff/5/?file=548486#file548486line2> > > > > s/2013/2014 got it. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImpl.java, > > line 2 > > <https://reviews.apache.org/r/19767/diff/5/?file=548486#file548486line2> > > > > s/2013/2014/g fixed. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImpl.java, > > line 56 > > <https://reviews.apache.org/r/19767/diff/5/?file=548486#file548486line56> > > > > Move down below the constructor. Fixed. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImpl.java, > > line 104 > > <https://reviews.apache.org/r/19767/diff/5/?file=548486#file548486line104> > > > > 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. Refactored a bit so the order of essential operations is clearer and log messages are more uniform. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronLifecycle.java, > > line 69 > > <https://reviews.apache.org/r/19767/diff/5/?file=548487#file548487line69> > > > > A bit terse :) Matches convention used in other event handlers. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImpl.java, > > line 162 > > <https://reviews.apache.org/r/19767/diff/5/?file=548486#file548486line162> > > > > Consider string.format() Done. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronModule.java, line > > 59 > > <https://reviews.apache.org/r/19767/diff/5/?file=548488#file548488line59> > > > > Move this down after the @CmdLine block. Done. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronModule.java, line > > 65 > > <https://reviews.apache.org/r/19767/diff/5/?file=548488#file548488line65> > > > > Shouldn't this be "...a previous cron run to end"? Fixed. > On April 4, 2014, 1:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronModule.java, line > > 70 > > <https://reviews.apache.org/r/19767/diff/5/?file=548488#file548488line70> > > > > Same here. Fixed. - Kevin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19767/#review39569 ----------------------------------------------------------- On April 3, 2014, 6:16 p.m., Kevin Sweeney wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19767/ > ----------------------------------------------------------- > > (Updated April 3, 2014, 6:16 p.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 > >
