History | Log In     View a printable version of the current page.  
Issue Details (XML | Word | Printable)

Key: QUARTZ-579
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: James House
Reporter: quartz12h
Votes: 0
Watchers: 1
Operations

If you were logged in you would be able to see more operations.
Quartz Scheduler

regression on timezone support of crontrigger

Created: 01/May/07 12:55 PM   Updated: 19/Aug/08 11:16 PM
Component/s: Triggers
Affects Version/s: 1.6
Fix Version/s: 1.6.1

File Attachments: 1. Text File QUARTZ-579-fix.patch (0.6 kb)
2. Text File QUARTZ-579-test.patch (1 kb)


Flags: Important


 Description  « Hide
My local pc timezone is EDT, (-5 with dst)
My crontrigger timezone is suppoed to be pacific time pdt (-8 with dst).

When I do this simple setup that worked with 1.5.1,

CronTrigger trigger = new CronTrigger();
trigger.setName(.....);
trigger.setGroup(Scheduler.DEFAULT_GROUP);
trigger.setTimeZone(tz);
trigger.setCronExpression(expression);


The crontrigger.setCronExpression() is setting the expression timezone to itself (edt, not pdt)
see CronTrigger line 444

line 444:
    public void setCronExpression(String cronExpression) throws ParseException {
        this.cronEx = new CronExpression(cronExpression);
        this.cronEx.setTimeZone(getTimeZone()); ////////// the bug is here
    }

line 562:
    public TimeZone getTimeZone() {
       
        if(cronEx != null) {
            return cronEx.getTimeZone();
        }
        
        if (timeZone == null) {
            timeZone = TimeZone.getDefault();
        }
        return timeZone;
    }



 All   Comments   Change History      Sort Order:
James House - [01/May/07 12:57 PM ]
Yikes!

Henri Yandell - [04/May/07 03:32 PM ]
Attaching a unit test showing this problem.

Henri Yandell - [04/May/07 03:41 PM ]
The fix seems easy enough, and I'll attach a patch for this:

    public void setCronExpression(String cronExpression) throws ParseException {
        TimeZone tmp = getTimeZone();
        this.cronEx = new CronExpression(cronExpression);
        this.cronEx.setTimeZone(tmp);
    }

Henri Yandell - [23/May/07 06:59 PM ]
Chatting on dev@, James suggests the following avenues of investigation:

* Remove the timezone variable from CronTrigger; delegate all timezone work into the CronExpression.

* Make sure serialization is happy with the variable removal. Possibly need to include the cronExpression's timezone in the serialization.

* Make sure to do:

   public void setCronExpression(String cronExpression) throws ParseException {
      TimeZone otz = this.cronEx.getTimeZone();
       this.cronEx = new CronExpression(cronExpression);
       this.cronEx.setTimeZone(otz);
   }

ie) if a String expression pased in, then use the current timezone.

* Investigate CronCalendar; it has the same problems.

James House - [19/Aug/08 11:16 PM ]

In studying the problem out futher.... due to the fact that CronTrigger.setTimeZone() has existed for years, and there is a lot of code calling it, we cannot really take the improvements so far as to remove the timezone property from CronTrigger.

Thus the immediate problem reported as part of the bug (not preserving the timezone when the cron expression is changed) has been fixed as originally proposed.