Re: Review Request 61249: Cron timezone should be configurable per job

2017-07-31 Thread Aurora ReviewBot

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


Ship it!




Master (cdc5b8e) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On July 31, 2017, 11: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, 11: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 * * F

Review Request 61249: Cron timezone should be configurable per job

2017-07-31 Thread Jing Chen

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

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 jo