You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Kevin Sweeney <ke...@apache.org> on 2014/02/05 21:42:26 UTC
Review Request 17750: CronScheduler implementation based on Quartz.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17750/
-----------------------------------------------------------
Review request for Aurora, Maxim Khutornenko and Bill Farner.
Bugs: AURORA-132
https://issues.apache.org/jira/browse/AURORA-132
Repository: aurora
Description
-------
CronScheduler implementation based on Quartz.
This adds an implementation of CronScheduler using Quartz [1], allowing Aurora to execute cron jobs without external triggering. The quartz library is Apache 2 licensed and thus including it in Aurora complies with the ASF's third-party licensing policy.
[1] http://quartz-scheduler.org
TODO (Subsequent reviews):
* Drop .noop package; make QuartCronModule the default
* Delete .testing package; merge QuartzCronIT and AbstractCronIT
* Delete thrift testing fixtures.
* Add examples of cron jobs to examples/jobs
* Add e2e test coverage for new examples
* Document cron schedule syntax in config reference.
* Create epic for reliable cron execution.
Diffs
-----
build.gradle 2f355d3528fb4fd5d41ce021ae3330b3af9142f5
src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronModule.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronPredictor.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronScheduler.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronIT.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronPredictorTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronSchedulerTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/17750/diff/
Testing
-------
./gradlew build
Thanks,
Kevin Sweeney
Re: Review Request 17750: CronScheduler implementation based on Quartz.
Posted by Kevin Sweeney <ke...@apache.org>.
> On Feb. 7, 2014, 1:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronPredictor.java, line 34
> > <https://reviews.apache.org/r/17750/diff/2/?file=470391#file470391line34>
> >
> > s/public//
fixed.
> On Feb. 7, 2014, 1:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronModule.java, line 70
> > <https://reviews.apache.org/r/17750/diff/2/?file=470390#file470390line70>
> >
> > Should it rather take "aurora-cron" and UUID.randomUUID() here? I am afraid any messages coming from quartz scheduler would duplicate the long name.
>
> Kevin Sweeney wrote:
> Configuration is stored in a mutable global singleton associated with a name and initialization fails if a scheduler with the same name has already been initialized. Maybe I should use a shorter string here (like a static counter in the module)?
Added a counter.
> On Feb. 7, 2014, 1:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronScheduler.java, line 58
> > <https://reviews.apache.org/r/17750/diff/2/?file=470392#file470392line58>
> >
> > Mixing fields with methods.
Moved down.
> On Feb. 7, 2014, 1:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronScheduler.java, line 66
> > <https://reviews.apache.org/r/17750/diff/2/?file=470392#file470392line66>
> >
> > Please, move into constructor for better consistency and visibility.
Moved.
> On Feb. 7, 2014, 1:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronScheduler.java, line 70
> > <https://reviews.apache.org/r/17750/diff/2/?file=470392#file470392line70>
> >
> > s/@Override/@Override\n/g
Done. That's gonna take me a while to get used to.
> On Feb. 7, 2014, 1:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronScheduler.java, line 89
> > <https://reviews.apache.org/r/17750/diff/2/?file=470392#file470392line89>
> >
> > + scheduler.getSchedulerName()?
Done.
> On Feb. 7, 2014, 1:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronScheduler.java, line 112
> > <https://reviews.apache.org/r/17750/diff/2/?file=470392#file470392line112>
> >
> > String.format() seems to be used only here. All other places use concatenation. Converge on one approach?
> >
> > Also, e should be on a separate line.
Converged.
> On Feb. 7, 2014, 1:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronScheduler.java, line 129
> > <https://reviews.apache.org/r/17750/diff/2/?file=470392#file470392line129>
> >
> > Move this outside try..catch?
Moved.
> On Feb. 7, 2014, 1:48 p.m., Maxim Khutornenko wrote:
> > src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronIT.java, line 31
> > <https://reviews.apache.org/r/17750/diff/2/?file=470393#file470393line31>
> >
> > Drop.
Dropped.
> On Feb. 7, 2014, 1:48 p.m., Maxim Khutornenko wrote:
> > src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronPredictorTest.java, line 38
> > <https://reviews.apache.org/r/17750/diff/2/?file=470394#file470394line38>
> >
> > remove
ok
> On Feb. 7, 2014, 1:48 p.m., Maxim Khutornenko wrote:
> > src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronSchedulerTest.java, line 100
> > <https://reviews.apache.org/r/17750/diff/2/?file=470395#file470395line100>
> >
> > Same.
Fixed.
> On Feb. 7, 2014, 1:48 p.m., Maxim Khutornenko wrote:
> > src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronSchedulerTest.java, line 73
> > <https://reviews.apache.org/r/17750/diff/2/?file=470395#file470395line73>
> >
> > Assert.fail()?
Done.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17750/#review33957
-----------------------------------------------------------
On Feb. 5, 2014, 3:23 p.m., Kevin Sweeney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17750/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2014, 3:23 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
> -------
>
> CronScheduler implementation based on Quartz.
>
> This adds an implementation of CronScheduler using Quartz [1], allowing Aurora to execute cron jobs without external triggering. The quartz library is Apache 2 licensed and thus including it in Aurora complies with the ASF's third-party licensing policy.
>
> [1] http://quartz-scheduler.org
>
> TODO (Subsequent reviews):
> * Drop .noop package; make QuartCronModule the default
> * Delete .testing package; merge QuartzCronIT and AbstractCronIT
> * Delete thrift testing fixtures.
> * Add examples of cron jobs to examples/jobs
> * Add e2e test coverage for new examples
> * Document cron schedule syntax in config reference.
> * Create epic for reliable cron execution.
>
>
> Diffs
> -----
>
> build.gradle 5b98659b9cb93a25f0f196285910cf6afb416072
> src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronModule.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronPredictor.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronScheduler.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronPredictorTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronSchedulerTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17750/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> Kevin Sweeney
>
>
Re: Review Request 17750: CronScheduler implementation based on Quartz.
Posted by Kevin Sweeney <ke...@apache.org>.
> On Feb. 7, 2014, 1:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronModule.java, line 70
> > <https://reviews.apache.org/r/17750/diff/2/?file=470390#file470390line70>
> >
> > Should it rather take "aurora-cron" and UUID.randomUUID() here? I am afraid any messages coming from quartz scheduler would duplicate the long name.
Configuration is stored in a mutable global singleton associated with a name and initialization fails if a scheduler with the same name has already been initialized. Maybe I should use a shorter string here (like a static counter in the module)?
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17750/#review33957
-----------------------------------------------------------
On Feb. 5, 2014, 3:23 p.m., Kevin Sweeney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17750/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2014, 3:23 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
> -------
>
> CronScheduler implementation based on Quartz.
>
> This adds an implementation of CronScheduler using Quartz [1], allowing Aurora to execute cron jobs without external triggering. The quartz library is Apache 2 licensed and thus including it in Aurora complies with the ASF's third-party licensing policy.
>
> [1] http://quartz-scheduler.org
>
> TODO (Subsequent reviews):
> * Drop .noop package; make QuartCronModule the default
> * Delete .testing package; merge QuartzCronIT and AbstractCronIT
> * Delete thrift testing fixtures.
> * Add examples of cron jobs to examples/jobs
> * Add e2e test coverage for new examples
> * Document cron schedule syntax in config reference.
> * Create epic for reliable cron execution.
>
>
> Diffs
> -----
>
> build.gradle 5b98659b9cb93a25f0f196285910cf6afb416072
> src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronModule.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronPredictor.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronScheduler.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronPredictorTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronSchedulerTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17750/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> Kevin Sweeney
>
>
Re: Review Request 17750: CronScheduler implementation based on Quartz.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17750/#review33957
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronModule.java
<https://reviews.apache.org/r/17750/#comment63789>
Should it rather take "aurora-cron" and UUID.randomUUID() here? I am afraid any messages coming from quartz scheduler would duplicate the long name.
src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronPredictor.java
<https://reviews.apache.org/r/17750/#comment63796>
s/public//
src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronScheduler.java
<https://reviews.apache.org/r/17750/#comment63797>
Mixing fields with methods.
src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronScheduler.java
<https://reviews.apache.org/r/17750/#comment63799>
Please, move into constructor for better consistency and visibility.
src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronScheduler.java
<https://reviews.apache.org/r/17750/#comment63802>
s/@Override/@Override\n/g
src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronScheduler.java
<https://reviews.apache.org/r/17750/#comment63803>
+ scheduler.getSchedulerName()?
src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronScheduler.java
<https://reviews.apache.org/r/17750/#comment63807>
String.format() seems to be used only here. All other places use concatenation. Converge on one approach?
Also, e should be on a separate line.
src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronScheduler.java
<https://reviews.apache.org/r/17750/#comment63804>
Move this outside try..catch?
src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronIT.java
<https://reviews.apache.org/r/17750/#comment63809>
Drop.
src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronPredictorTest.java
<https://reviews.apache.org/r/17750/#comment63810>
remove
src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronSchedulerTest.java
<https://reviews.apache.org/r/17750/#comment63811>
Assert.fail()?
src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronSchedulerTest.java
<https://reviews.apache.org/r/17750/#comment63813>
Same.
- Maxim Khutornenko
On Feb. 5, 2014, 11:23 p.m., Kevin Sweeney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17750/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2014, 11:23 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
> -------
>
> CronScheduler implementation based on Quartz.
>
> This adds an implementation of CronScheduler using Quartz [1], allowing Aurora to execute cron jobs without external triggering. The quartz library is Apache 2 licensed and thus including it in Aurora complies with the ASF's third-party licensing policy.
>
> [1] http://quartz-scheduler.org
>
> TODO (Subsequent reviews):
> * Drop .noop package; make QuartCronModule the default
> * Delete .testing package; merge QuartzCronIT and AbstractCronIT
> * Delete thrift testing fixtures.
> * Add examples of cron jobs to examples/jobs
> * Add e2e test coverage for new examples
> * Document cron schedule syntax in config reference.
> * Create epic for reliable cron execution.
>
>
> Diffs
> -----
>
> build.gradle 5b98659b9cb93a25f0f196285910cf6afb416072
> src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronModule.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronPredictor.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronScheduler.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronPredictorTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronSchedulerTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17750/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> Kevin Sweeney
>
>
Re: Review Request 17750: CronScheduler implementation based on Quartz.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17750/#review38805
-----------------------------------------------------------
This is obsoleted by https://reviews.apache.org/r/17750/ so I'm discarding
- Kevin Sweeney
On Feb. 13, 2014, 3:50 p.m., Kevin Sweeney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17750/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2014, 3:50 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
> -------
>
> CronScheduler implementation based on Quartz.
>
> This adds an implementation of CronScheduler using Quartz [1], allowing Aurora to execute cron jobs without external triggering. The quartz library is Apache 2 licensed and thus including it in Aurora complies with the ASF's third-party licensing policy.
>
> [1] http://quartz-scheduler.org
>
> TODO (Subsequent reviews):
> * Drop .noop package; make QuartCronModule the default
> * Delete .testing package; merge QuartzCronIT and AbstractCronIT
> * Delete thrift testing fixtures.
> * Add examples of cron jobs to examples/jobs
> * Add e2e test coverage for new examples
> * Document cron schedule syntax in config reference.
> * Create epic for reliable cron execution.
>
>
> Diffs
> -----
>
> build.gradle 4ba72c109b92d05581dbfe78194518653f20e12d
> src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronModule.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronPredictor.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronScheduler.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronIT.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronPredictorTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronSchedulerTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17750/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> Kevin Sweeney
>
>
Re: Review Request 17750: CronScheduler implementation based on Quartz.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17750/
-----------------------------------------------------------
(Updated Feb. 13, 2014, 3:50 p.m.)
Review request for Aurora, Maxim Khutornenko and Bill Farner.
Changes
-------
Incorporate Maxim's feedback.
Bugs: AURORA-132
https://issues.apache.org/jira/browse/AURORA-132
Repository: aurora
Description
-------
CronScheduler implementation based on Quartz.
This adds an implementation of CronScheduler using Quartz [1], allowing Aurora to execute cron jobs without external triggering. The quartz library is Apache 2 licensed and thus including it in Aurora complies with the ASF's third-party licensing policy.
[1] http://quartz-scheduler.org
TODO (Subsequent reviews):
* Drop .noop package; make QuartCronModule the default
* Delete .testing package; merge QuartzCronIT and AbstractCronIT
* Delete thrift testing fixtures.
* Add examples of cron jobs to examples/jobs
* Add e2e test coverage for new examples
* Document cron schedule syntax in config reference.
* Create epic for reliable cron execution.
Diffs (updated)
-----
build.gradle 4ba72c109b92d05581dbfe78194518653f20e12d
src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronModule.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronPredictor.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronScheduler.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronIT.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronPredictorTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronSchedulerTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/17750/diff/
Testing
-------
./gradlew build
Thanks,
Kevin Sweeney
Re: Review Request 17750: CronScheduler implementation based on Quartz.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17750/
-----------------------------------------------------------
(Updated Feb. 5, 2014, 3:23 p.m.)
Review request for Aurora, Maxim Khutornenko and Bill Farner.
Changes
-------
Rebase to master.
Bugs: AURORA-132
https://issues.apache.org/jira/browse/AURORA-132
Repository: aurora
Description
-------
CronScheduler implementation based on Quartz.
This adds an implementation of CronScheduler using Quartz [1], allowing Aurora to execute cron jobs without external triggering. The quartz library is Apache 2 licensed and thus including it in Aurora complies with the ASF's third-party licensing policy.
[1] http://quartz-scheduler.org
TODO (Subsequent reviews):
* Drop .noop package; make QuartCronModule the default
* Delete .testing package; merge QuartzCronIT and AbstractCronIT
* Delete thrift testing fixtures.
* Add examples of cron jobs to examples/jobs
* Add e2e test coverage for new examples
* Document cron schedule syntax in config reference.
* Create epic for reliable cron execution.
Diffs (updated)
-----
build.gradle 5b98659b9cb93a25f0f196285910cf6afb416072
src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronModule.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronPredictor.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronScheduler.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronIT.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronPredictorTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzCronSchedulerTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/17750/diff/
Testing
-------
./gradlew build
Thanks,
Kevin Sweeney