> On April 5, 2014, 4:20 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java, line 54
> > <https://reviews.apache.org/r/19767/diff/6/?file=549800#file549800line54>
> >
> >     s/persisted //
> >     
> >     (not really relevant)

fixed


> On April 5, 2014, 4:20 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java, line 65
> > <https://reviews.apache.org/r/19767/diff/6/?file=549800#file549800line65>
> >
> >     s/Consdier/Consider/

fixed


> On April 5, 2014, 4:20 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java, line 63
> > <https://reviews.apache.org/r/19767/diff/6/?file=549800#file549800line63>
> >
> >     s/ in storage//

fixed


> On April 5, 2014, 4:20 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/CronJobManager.java, line 73
> > <https://reviews.apache.org/r/19767/diff/6/?file=549800#file549800line73>
> >
> >     s/ from the storage//

fixed.


> On April 5, 2014, 4:20 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java, 
> > line 60
> > <https://reviews.apache.org/r/19767/diff/6/?file=549808#file549808line60>
> >
> >     Looks like the doc needs a refresh since there is only a single global 
> > Job instance now.

It's stateless and called once per job run.


> On April 5, 2014, 4:20 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java, 
> > line 62
> > <https://reviews.apache.org/r/19767/diff/6/?file=549808#file549808line62>
> >
> >     Use <p> if you want this break to show in rendered javadoc.

done.


> On April 5, 2014, 4:20 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java, 
> > line 109
> > <https://reviews.apache.org/r/19767/diff/6/?file=549808#file549808line109>
> >
> >     Can you leave a comment explaining the reason this check is needed?

done.


> On April 5, 2014, 4:20 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java, 
> > line 195
> > <https://reviews.apache.org/r/19767/diff/6/?file=549808#file549808line195>
> >
> >     It's worth noting that we can also wind up dogpiling here if the kill 
> > takes too long.  This is reasonably likely for hard-to-kill minutely crons.

that's why I noted it. do you suggest any followup language?


> On April 5, 2014, 4:20 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java, 
> > line 212
> > <https://reviews.apache.org/r/19767/diff/6/?file=549808#file549808line212>
> >
> >     you'll need to follow this by "throw e".  the above will only set the 
> > interrupted flag.

done.


> On April 5, 2014, 4:20 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobFactory.java,
> >  line 33
> > <https://reviews.apache.org/r/19767/diff/6/?file=549809#file549809line33>
> >
> >     Is there anything preventing you from using a single AuroraJob instance 
> > for all cron jobs?

It's stateless so there's not. Added a Singleton binding.


> On April 5, 2014, 4:20 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronModule.java, line 
> > 104
> > <https://reviews.apache.org/r/19767/diff/6/?file=549812#file549812line104>
> >
> >     What is XXX?

Changed to NOTE


> On April 5, 2014, 4:20 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronModule.java, line 
> > 129
> > <https://reviews.apache.org/r/19767/diff/6/?file=549812#file549812line129>
> >
> >     Does the indirection of another binding help here (at the expense of 
> > exposing the binding globally)?  Consider inlining this in provideScheduler.

Inlined.


> On April 5, 2014, 4:20 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronSchedulerImpl.java,
> >  line 62
> > <https://reviews.apache.org/r/19767/diff/6/?file=549814#file549814line62>
> >
> >     A short comment on why first() is used as opposed to something like 
> > Iterables.getOnlyElement would be helpful for future context.

Aesthetics. Changed to getOnlyElement.


> On April 5, 2014, 4:20 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/cron/quartz/CronIT.java, line 58
> > <https://reviews.apache.org/r/19767/diff/6/?file=549831#file549831line58>
> >
> >     It would be nice to see test coverage for a dogpiling trigger:
> >     
> >     - KILL_EXISTING cron triggers
> >     - job running, back off while killing
> >     - same cron triggers again

Looking at the Quartz code, the @DisableConcurrentExecution annotation should 
prevent this (and I don't see an easy way to test this). Looking at the code it 
appears that the job is placed into a BLOCKED state that prevents new triggers 
from firing (that's cleared when execution completes). The documentation [1] 
sorta alludes to this but I didn't see a concrete example.

[1] http://quartz-scheduler.org/documentation/quartz-2.2.x/examples/Example5


> On April 5, 2014, 4:20 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/cron/quartz/CronIT.java, line 101
> > <https://reviews.apache.org/r/19767/diff/6/?file=549831#file549831line101>
> >
> >     Is this still needed now that you're not injecting the injector?  I'd 
> > love to avoid Modules.override.

Using a real AuroraCronJob instance makes this test pretty hairy, and its logic 
is unit tested very well separately. Dropped a TODO to move to using the 
production class here.


> On April 5, 2014, 4:20 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/cron/quartz/CronIT.java, line 205
> > <https://reviews.apache.org/r/19767/diff/6/?file=549831#file549831line205>
> >
> >     Drop

Dropped.


> On April 5, 2014, 4:20 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImplTest.java,
> >  line 141
> > <https://reviews.apache.org/r/19767/diff/6/?file=549832#file549832line141>
> >
> >     Prefer assertEquals here:
> >     
> >       assertEquals(emptySet, cronJobManager.getJobs());
> >     
> >     This allows junit to give a more useful error message (showing the 
> > contents rather than just true/false).

Done.


> On April 5, 2014, 4:20 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImplTest.java,
> >  line 170
> > <https://reviews.apache.org/r/19767/diff/6/?file=549832#file549832line170>
> >
> >     assertEquals(Optional.<IJobConfiguration >absent(), fetchFromStorage());
> >     
> >     Here and elsewhere.

Done.


- Kevin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19767/#review39634
-----------------------------------------------------------


On April 4, 2014, 6:31 p.m., Kevin Sweeney wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19767/
> -----------------------------------------------------------
> 
> (Updated April 4, 2014, 6:31 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
> 
>

Reply via email to