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

2017-08-02 Thread David McLaughlin 


> On Aug. 2, 2017, 8:38 p.m., David McLaughlin wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java
> > Lines 266 (patched)
> > 
> >
> > 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

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

2017-08-02 Thread Jing Chen 


> 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)
> > 
> >
> > 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)
> > 
> >
> > 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

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

2017-08-02 Thread David McLaughlin 

---
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)


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)


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)


Can we combine both these messages together?



src/main/java/org/apache/aurora/scheduler/cron/quartz/CronPredictorImpl.java
Line 41 (original), 41 (patched)


Is this called anymore?



src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java
Lines 266 (patched)


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
>