aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David McLaughlin <da...@dmclaughlin.com>
Subject Re: Review Request 61249: Cron timezone should be configurable per job
Date Wed, 20 Sep 2017 17:22:53 GMT


> On Sept. 20, 2017, 4:50 p.m., Bill Farner wrote:
> > This seems to be of dubious value, and likely error-prone.  There are two perspectives
here - display, and time scheduling.  Per-job configuration for TZ display seems unequivocally
misguided, and better suited for API consumers to handle (e.g. javascript/browser, and the
CLI client).  TZ configuration for time scheduling seems less bad, but error-prone due to
inconsistency within a cluster.  Perhaps a cluster-wide setting makes more sense here, but
to be honest i'm inclined to suggest that UTC is still the most sane way to eliminate time
ambiguity.
> > 
> > Overall this makes me a -1 on the patch as it stands, but i wouldn't stand in the
way if others disagree.

Note that we already have the ability to allow operators to define a cluster-wide setting:
https://github.com/apache/aurora/blob/master/src/main/java/org/apache/aurora/scheduler/cron/quartz/CronModule.java#L64

In general I agree with your sentiments.


- David


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


On Sept. 12, 2017, 8:38 a.m., Jing Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61249/
> -----------------------------------------------------------
> 
> (Updated Sept. 12, 2017, 8:38 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Joshua Cohen, and Stephan Erb.
> 
> 
> Bugs: AURORA-1348
>     https://issues.apache.org/jira/browse/AURORA-1348
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Cron timezone should be configurable per job
> 
> 
> Diffs
> -----
> 
>   RELEASE-NOTES.md fd2618fee8ef05091849e177bd99fc321548be90 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 3749531b5412d7ca217736aa85eed8e6606225ad

>   docs/features/cron-jobs.md 3a3edeabe23553f12074a853c2567b2b62748226 
>   docs/reference/configuration.md bc7e098bde5da1442f7ce1b0f793fa4495e21d9a 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 186fa1b3a4780c0536fb486d50a33133258110cd

>   src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java 754fde0fdc976b673d78ae15d8ccd8c85b792373

>   src/main/java/org/apache/aurora/scheduler/cron/CronPredictor.java 8957b3a04e681c653884fdc9134d74ae766677ae

>   src/main/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImpl.java 90399f22c50ba3218fa3f392c3dbf24e6ea524a1

>   src/main/java/org/apache/aurora/scheduler/cron/quartz/CronLifecycle.java f18e1dcd6d34d6376d267359d2aaedff1d0c0202

>   src/main/java/org/apache/aurora/scheduler/cron/quartz/CronPredictorImpl.java e937667dc73c432dc0934a18f28274913ec5640d

>   src/main/java/org/apache/aurora/scheduler/storage/db/views/DbJobConfiguration.java
40a5013b62d459d9c766c765f9e536f7042757e1 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java bba1161a48738e19f10fcf72395f8d70b481ed13

>   src/main/python/apache/aurora/client/cli/jobs.py b79ae56bee0e5692cacf1e66f4a4126b06aaffdc

>   src/main/python/apache/aurora/config/schema/base.py 18ce826363009e1cc0beac5cce4edf42610d487e

>   src/main/python/apache/aurora/config/thrift.py bedf8cd6641e1b1a930602791b758d584af4891c

>   src/main/resources/org/apache/aurora/scheduler/storage/db/CronJobMapper.xml 1434f45ca0bc188bfb0f2ef3c25fbcd102a3ccb1

>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 7a86f47af67adb3e488381d30ddf424549deefbc

>   src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
50d7499f4332a3feb0e2301cb707f2cea6bb2e98 
>   src/test/java/org/apache/aurora/scheduler/cron/quartz/CronIT.java 8556253fc11f6027316871eb9dc66d8627a77fe6

>   src/test/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImplTest.java 81440f5689f9538a4c7a9e6700bf03ca89c4ba85

>   src/test/java/org/apache/aurora/scheduler/cron/quartz/CronPredictorImplTest.java 0f8c9839000e2e379a75fd9d10a7f22fc01f3b18

>   src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzTestUtil.java 3c5ecd698557cafdf8eeacdc472589a379018896

>   src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java 3ec6e2bf28db20f1cda26dd9862af83d479ec4bf

>   src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java 7f41430975d46440a404ac58395582f66fd902d4

>   src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 43e32eede27bbf26363a3fd1ca34ffe6f8c01a73

>   src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java 39456cfe8b8c8e198a6f8c82e09769bd75db2adc

>   src/test/python/apache/aurora/client/cli/test_inspect.py ecefc1845350242fe5113e5c85c4e3db09937ec1

>   src/test/python/apache/aurora/config/test_thrift.py 7a1567a9b67917072bb0ba3eea5857e968375f4d

> 
> 
> Diff: https://reviews.apache.org/r/61249/diff/3/
> 
> 
> Testing
> -------
> 
> timezone infomation can be configured via configuration file, aka .aurora file. A new
entry **time_zone** will be introduced.
> 
> Time zone information would be validated and populated in configuration manager.
>  * It accpets the value as long as the value is accepted by java.util.TimeZone. Otherwise,
an exception would be thrown indicating that timezone is invalid.
>  * If timezone is missing from configuration, an scheduler wide timezone will be applied.
> 
> The cron job is trigged at the time that is calculated from the given timezone.
> 
> From the UI, there are two times being showed
>  * local next run time which has been converted from the given timezone
>  * next run time in timezone UTC
> 
> 
> Run two tests here
> =====
> 
> Provided my time zone is Pacific time, and the cron schedule is `* 17 * * FRI`
> 
> 1. Configuration file 1, whose time zone is set to **Berlin**:
> ```
>  jobs = [
>   Job(
>     cluster = 'devcluster',
>     role = 'www-data',
>     environment = 'devel',
>     name = 'cron_job_berlin',
>     cron_schedule = '* 17 * * FRI',
>     cron_collision_policy='KILL_EXISTING',
>     time_zone='Europe/Berlin',
>     task = Task(
>       name="cron_task",
>       processes=[Process(name="cron_process",
>            cmdline="echo 'cron job in berlin timezone runs'; date")],
>       resources=Resources(cpu=1, ram=1*MB, disk=8*MB)
>     )
>   )
> ]
> ```
> 
> The _next cron run_ is scheduled at `08/04 08:00:00 LOCAL, 08/04 15:00:00 UTC`
> 
> 2. Configuration file 2, whose time zone is **missing**:
> ```
> jobs = [
>   Job(
>     cluster = 'devcluster',
>     role = 'www-data',
>     environment = 'devel',
>     name = 'cron_job_no_tz',
>     cron_schedule = '* 17 * * FRI',
>     cron_collision_policy='KILL_EXISTING',
>     task = Task(
>       name="cron_task",
>       processes=[Process(name="cron_process",
>            cmdline="echo 'cron job no tz runs'; date")],
>       resources=Resources(cpu=1, ram=1*MB, disk=8*MB)
>     )
>   )
> ]
> ```
> The _next cron run_ is scheduled at `08/04 10:00:00 LOCAL, 08/04 17:00:00 UTC`
> 
> 
> Thanks,
> 
> Jing Chen
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message