aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stephan Erb <s...@apache.org>
Subject Re: Review Request 61249: Cron timezone should be configurable per job
Date Thu, 03 Aug 2017 21:15:35 GMT


> On Aug. 2, 2017, 10:38 p.m., David McLaughlin wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java
> > Lines 266 (patched)
> > <https://reviews.apache.org/r/61249/diff/1/?file=1785805#file1785805line266>
> >
> >     This will be null for all crons scheduled before this patch landed, and below
it will default to "GMT", possibly conflicting with the scheduler default. 
> >     
> >     So you can either do a null check here and call predictNextRun when it's null,
or do a storage backfill and add timezone to all existing crons.
> 
> Jing Chen wrote:
>     In ConfigurationManager::validateAndPopulate, a cron job's timezone is set to the
scheduler's timezone if its timezone is not set.
>     So I assume every cron job's timezone must be populated since they are all validated
via ConfigurationManager.
>     
>     Please correct me if i was wrong
> 
> David McLaughlin wrote:
>     Ah, you're right. I thought validateAndPopulate was only called when the config was
sent to the API. But it's also called implicitly via the SanitizedCronJob class.

Where is the spot this is called? The way I am reading it at the moment, `getJobs` is directly
calling into `Storage.Util.fetchCronJobs(storage)` and that would be un-sanitized.


- Stephan


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


On July 31, 2017, 12:29 p.m., Jing Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61249/
> -----------------------------------------------------------
> 
> (Updated July 31, 2017, 12:29 p.m.)
> 
> 
> Review request for Aurora, 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
> -----
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 3749531b5412d7ca217736aa85eed8e6606225ad

>   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/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/config/test_thrift.py 7a1567a9b67917072bb0ba3eea5857e968375f4d

> 
> 
> Diff: https://reviews.apache.org/r/61249/diff/1/
> 
> 
> 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