You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by David McLaughlin <da...@dmclaughlin.com> on 2017/08/02 20:38:05 UTC
Re: Review Request 61249: Cron timezone should be configurable per job
-----------------------------------------------------------
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
>
>
Re: Review Request 61249: Cron timezone should be configurable per job
Posted by Stephan Erb <se...@apache.org>.
> On Aug. 2, 2017, 10:38 p.m., David McLaughlin wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
> > Lines 185-186 (patched)
> > <https://reviews.apache.org/r/61249/diff/1/?file=1785799#file1785799line185>
> >
> > 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.
I believe a more obvious implementation could be to check if `timeZoneId` is contained in `TimeZone.getAvailableIDs()`.
- 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
>
>
Re: Review Request 61249: Cron timezone should be configurable per job
Posted by Jing Chen <mi...@gmail.com>.
> On Aug. 2, 2017, 8: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.
>
> Stephan Erb wrote:
> 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.
>
> David McLaughlin wrote:
> Here: https://github.com/apache/aurora/blob/master/src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java#L171
>
> David McLaughlin wrote:
> Actually, you are correct in that the data returned to the user and visible in the UI will be incorrect.
>
> I left my comment in the wrong place - the problem I was worried about was on the cron scheduling side. Which should be fixed by the sanitization.
>
> Stephan Erb wrote:
> Jing, I believe this issue still stands. Do you think you can address it?
>
> (Sorry for the epic delay in answering!)
Just wondering if it is good idea to sanitize crons when they are fetched from storage.
- Jing
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61249/#review182031
-----------------------------------------------------------
On Aug. 5, 2017, 12:31 p.m., Jing Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61249/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2017, 12:31 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
> -----
>
> 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/2/
>
>
> 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
>
>
Re: Review Request 61249: Cron timezone should be configurable per job
Posted by David McLaughlin <da...@dmclaughlin.com>.
> On Aug. 2, 2017, 8: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.
>
> Stephan Erb wrote:
> 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.
>
> David McLaughlin wrote:
> Here: https://github.com/apache/aurora/blob/master/src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java#L171
Actually, you are correct in that the data returned to the user and visible in the UI will be incorrect.
I left my comment in the wrong place - the problem I was worried about was on the cron scheduling side. Which should be fixed by the sanitization.
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61249/#review182031
-----------------------------------------------------------
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
>
>
Re: Review Request 61249: Cron timezone should be configurable per job
Posted by David McLaughlin <da...@dmclaughlin.com>.
> On Aug. 2, 2017, 8: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
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.
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61249/#review182031
-----------------------------------------------------------
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
>
>
Re: Review Request 61249: Cron timezone should be configurable per job
Posted by David McLaughlin <da...@dmclaughlin.com>.
> On Aug. 2, 2017, 8:38 p.m., David McLaughlin wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift
> > Lines 328 (patched)
> > <https://reviews.apache.org/r/61249/diff/1/?file=1785797#file1785797line328>
> >
> > Needs to be marked optional to support your backwards-compatible default strategy.
>
> Stephan Erb wrote:
> I believe it is also risky to re-use a previously used ID. Some very old client or integration could could potentially still expect another field by that ID. David, please correct me if I am wrong here.
I had a similar question but couldn't see anything in the Thrift docs. Unfortunately I'm not a Thrift expert. Was hoping someone else could chime in :)
> On Aug. 2, 2017, 8: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.
>
> Stephan Erb wrote:
> 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.
Here: https://github.com/apache/aurora/blob/master/src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java#L171
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61249/#review182031
-----------------------------------------------------------
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
>
>
Re: Review Request 61249: Cron timezone should be configurable per job
Posted by Jing Chen <mi...@gmail.com>.
> On Aug. 2, 2017, 8:38 p.m., David McLaughlin wrote:
> > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronPredictorImpl.java
> > Line 41 (original), 41 (patched)
> > <https://reviews.apache.org/r/61249/diff/1/?file=1785803#file1785803line41>
> >
> > Is this called anymore?
most likely no, I will double check and remove the method once I could confirm
> On Aug. 2, 2017, 8: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.
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
- Jing
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61249/#review182031
-----------------------------------------------------------
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
>
>
Re: Review Request 61249: Cron timezone should be configurable per job
Posted by Stephan Erb <se...@apache.org>.
> 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.
>
> Stephan Erb wrote:
> 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.
>
> David McLaughlin wrote:
> Here: https://github.com/apache/aurora/blob/master/src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java#L171
>
> David McLaughlin wrote:
> Actually, you are correct in that the data returned to the user and visible in the UI will be incorrect.
>
> I left my comment in the wrong place - the problem I was worried about was on the cron scheduling side. Which should be fixed by the sanitization.
Jing, I believe this issue still stands. Do you think you can address it?
(Sorry for the epic delay in answering!)
- Stephan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61249/#review182031
-----------------------------------------------------------
On Aug. 5, 2017, 2:31 p.m., Jing Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61249/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2017, 2:31 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
> -----
>
> 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/2/
>
>
> 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
>
>
Re: Review Request 61249: Cron timezone should be configurable per job
Posted by Stephan Erb <se...@apache.org>.
> On Aug. 2, 2017, 10:38 p.m., David McLaughlin wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift
> > Lines 328 (patched)
> > <https://reviews.apache.org/r/61249/diff/1/?file=1785797#file1785797line328>
> >
> > Needs to be marked optional to support your backwards-compatible default strategy.
I believe it is also risky to re-use a previously used ID. Some very old client or integration could could potentially still expect another field by that ID. David, please correct me if I am wrong here.
- 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
>
>
Re: Review Request 61249: Cron timezone should be configurable per job
Posted by Stephan Erb <se...@apache.org>.
> 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
>
>