----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61249/#review182031 -----------------------------------------------------------
Thanks for the comprehensive patch! Left some comments below. api/src/main/thrift/org/apache/aurora/gen/api.thrift Lines 328 (patched) <https://reviews.apache.org/r/61249/#comment257841> Needs to be marked optional to support your backwards-compatible default strategy. src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java Lines 185-186 (patched) <https://reviews.apache.org/r/61249/#comment257848> Can you add a comment to this check? This was initially confusing to me until I checked the javadoc for TimeZone::getTimeZone and realized it defaulted to GMT. src/main/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImpl.java Line 158 (original), 161-164 (patched) <https://reviews.apache.org/r/61249/#comment257849> Can we combine both these messages together? src/main/java/org/apache/aurora/scheduler/cron/quartz/CronPredictorImpl.java Line 41 (original), 41 (patched) <https://reviews.apache.org/r/61249/#comment257850> Is this called anymore? src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java Lines 266 (patched) <https://reviews.apache.org/r/61249/#comment257851> 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. - David McLaughlin On July 31, 2017, 10:29 a.m., Jing Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61249/ > ----------------------------------------------------------- > > (Updated July 31, 2017, 10:29 a.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 > >